Transition host apps to registered state if necessary #1614
Conversation
|
I have been testing this now with Firefox. The issue I hit with the original code is that when running Firefox from a command line, which is obvious since I've been testing a new feature. It fails to register, because it already has an entry it guessed from the PID, but it ends up with an empty app ID. For that reason I tried to apply this change which fixes the issue for me, but I had to fix the two issues I mentioned. Edit: can be ignored, it fails to register, because I was registering it too late as there is a code in FF that asks the settings portal for color scheme. Still the suggestions to the code apply. |
src/xdp-app-info-host.c
Outdated
| @@ -200,6 +202,7 @@ xdp_app_info_host_new_full (const char *app_id, | |||
| /* supports_opath */ TRUE, | |||
| /* has_network */ TRUE, | |||
| /* requires_pid_mapping */ FALSE); | |||
| app_info_host->registered = registered; | |||
There was a problem hiding this comment.
The XdpAppInfoTest also can be registered which is needed for the tests and they now have divergent behavior.
There was a problem hiding this comment.
Maybe we should take the opportunity to rework the XdpAppInfoTest and XdpAppInfo testing in general. Currently we can't do integration tests with anything but XdpAppInfoHosts and that has caused regressions.
There was a problem hiding this comment.
I've reworked XdpAppInfo internals quite intensely in this MR, and I think that's about as far as I can go without turning this PR into a monstrosity. Should make it easier to do any follow-up if necessary.
Purely cosmetics.
They're not public.
* Add space before '('
* gchar → char
* Remove stray newline
It's unused.
Makes it slightly easier to read.
We'll start shuffling things around, so that XdpAppInfo & subclasses operate with the mechanics proposed by GInitable.
And pass it during object construction (i.e. g_initable_new())
In the future it'll be mutable, e.g. when registering a host app, but for now it's construct-only.
And pass it during object construction (i.e. g_initable_new()). Since xdp_app_info_initialize() is now empty, remove it.
And pass it during object construction (i.e. g_initable_new())
Generate flag type from the private enum, and make it a construct-only property.
And pass it during object construction (i.e. g_initable_new()). This was the last value passed to xdp_app_info_initialize() so remove this function as well.
Now that the constructor is a simple g_initable_new(), the xdp_app_info_host_new_full() constructor seems unnecessary. Next commits will introduce different flags for each path so just separate them now.
Right now it's somewhat useless, and just serves as a safeguard, but it will be useful for sharing the GAppInfo creation code a little more.
Instead of making each constructor create a GAppInfo in almost exactly equal ways, add an overridable vfunc, and implement the common path in XdpAppInfo. XdpAppInfoSnap needs a little bit of handholding, since the desktop file does not match the app id, so add a construct-only property to XdpAppInfoSnap and use it for a Snap-specific GAppInfo construction.
No functional changes (hopefully).
It indicates if an XdpAppInfo is "final" - as in, cannot be registered again - or not. Flapaks, Snaps, and registered host apps have this flag set. Unregistered host apps and test apps don't.
This will be used for transitioning test apps to registered state.
GeorgesStavracas
force-pushed
the
gbsneto/host-transition-registered
branch
2 times, most recently
from
0178915 to
e9a3b5f
Compare
|
CI is happy now but leaving as a draft as it's not clear if this approach is desired. |
There was a problem hiding this comment.
The first few commits can land independently I guess, but the last few that moves towards allowing transitioning is what needs consideration.
The portal service going away is an annoying race to deal with, and with multiple entities sharing the same connection (GApplication, app and libportal) I guess it'll be hard to handle registration when the name disappears and reappears, so with that in mind, this "best effort" might be the best we can do.
What is a different but related concern is that it might paper over incorrect implementations where an app does portal call, then registers, when starting up. A bit tricky to distinguish a broken app, from an app having trouble racing to win recovering from the portal service restarting.
Opened #1619 with them.
Yeah, this is the main reason I'm not sure about this PR. I thought about adding a warning to scare app developers slightly, but that will probably be insufficient. |
If a host app is created due an early D-Bus, then the registration happens, transition it from "guessed" to registered. But only do it once. This allows smoother recovery of apps if the portal service disappears for some reason, e.g. crashes or restarts. Adjust the registry tests to exercise this code path. Closes: flatpak#1612
GeorgesStavracas
force-pushed
the
gbsneto/host-transition-registered
branch
from
e9a3b5f to
ea52415
Compare
Allow transitioning a host app from "guessed" to registered, once.
See #1612