XdpAppInfo refactoring, Part 2

[swick]

This is a continuation of #1366.

This moves the app info kind specific code to subclasses, the pid mapping stuff to utils and reduces the scope of XdpAppInfo.

[swick]

Rebased on top of the changes from Part 1.

[swick]

Rebased and added one more commit.

[GeorgesStavracas]

I think constructors like this should always return the object they're constructing:

XdpAppInfo *
xdp_app_info_flatpak_new (int      pid,
                          int      pidfd,
                          GError **error);

[swick]

That's not possible because this is basically a tri-state. While creating it, errors can occur, in which case no further attempt must be made to create another XdpAppInfo (because that might create a host XdpAppInfo for a flatpak app). It however can also be possible that the app just isn't e.g. a flatpak app and that snap and then host should be tried later.

The API is as good as it gets with those semantics because it follows the "returns FALSE when error is set" convention while also allowing to either create or not create an XdpAppInfo.

[ebassi]

That's not possible because this is basically a tri-state. While creating it, errors can occur, in which case no further attempt must be made to create another XdpAppInfo (because that might create a host XdpAppInfo for a flatpak app).

That's what GInitable is for.

The API is as good as it gets with those semantics because it follows the "returns FALSE when error is set"

You can return NULL and set the error. Typical constructors cannot return NULL, but fallible constructors (like anything that implements GInitable) can do that.

[swick]

I don't get it. xdp_app_info_flatpak_new either creates a XdpAppInfo GObject in which case there won't be an error, or figures out that the process isn't a flatpak process, or runs into an error while trying to figure out if the process is a flatpak process or not.

GInitiable doesn't help here at all because the GObject initialization always succeeds.

You can return NULL and set the error.

I also can do this here, and that's what the code did before, but then NULL can be returned and it either means there was an error (when error is set) or the process isn't a flatpak process (when error is not set).

[hfiguiere]

how about returning NULL and setting the GError. Then the GError can indicate the proper reason. You annotate that it is nullable (there is no associated doc at the moment it seems). Also about the "no further attempt" the code must ensure of that, to prevent API misuse.

[swick]

Returning NULL meaning either "not a flatpak process" or "there was an error" is exactly how it worked before. In glib and a lot of other GNOME components there is a convention that the success/error state is returned and the error is just an additional piece of information. This change is so that it follows this convention.

I really don't understand the problem here. Out arguments are common. The error argument itself is such a thing.

[GeorgesStavracas]

I think what I was trying to say is, a common pattern for this kind of situation is to define a custom GError, and use it. Here's an example in Boatswain.

The BS_STREAM_DECK_ERROR_UNRECOGNIZED is defined specifically for the purpose of saying: this device is not a recognized Elgato device. Creating an object will fail (the purpose of GInitable is for failable constructors after all), and we use the extra bit of information - the error itself - to decide how to proceed.

[swick]

Sure, I'm now using a new XDG_DESKTOP_PORTAL_ERROR_WRONG_APP_KIND error to signal that the process isn't the of the app kind which was attempted to be created.

[jsparber]

Wouldn't it make sense to validate_desktop_file_id() from the DynamicLauncher to be part of xdp_app_info_validate_dynamic_launcher()?

But maybe it could also be a separate vfunc so it can be reused by the notification spec v2

[swick]

Wanted to leave that for a future cleanup but I've done that now (last two commits).

[swick]

Removed the last few commits and moved them to a part 3 MR. The commits which are left should not be split any further because that might leave the code in a suboptimal state.