GNOME Bugzilla – Bug 688499
work towards not linking against gtk
Last modified: 2021-06-14 18:21:45 UTC
It is not really right for a session manager to link against a toolkit. The http://git.gnome.org/browse/gnome-session/log/?h=wip/cleanups branch has a bunch of work towards dropping that. Among other things, it: - drops the fallback warning dialog - uses zenity for init warnings - turn the fail whale into a standalone binary - turn the fallback inhibit+logout dialogs into a standalone binary that implements the EndSessionDialog dbus api - ports GsmAutostartApp to GDesktopAppInfo
"It is not really right" isn't a really good motivation. Not saying it's wrong, but... One thing I think you're losing here is correct focus stealing timestamp handling, where we pass around the event timestamp down into startup-notification. If we link with GTK+, that's provided GdkAppLaunchContext. It probably doesn't matter too much...but if we were to say add a button "Restore session" somewhere, which caused gnome-session to launch saved apps, we should be passing the timestamp of that click down to the apps. Also, it looks like your spawn handling is doing G_SPAWN_DO_NOT_REAP_CHILD, but is not installing a child watch, so you'll collect zombies. Those are the two things I noticed in a quick look at the top of the patch stack. I can look in more detail later.
right, so the motivations are: - detangle the gsm-manager state machine by moving the fallback code behind the same dbus api as the shell - get the fallback code out of process, so we can easily drop it - make it easier to test ui code like the fail whale by making it standalone - gain (a little bit) of login speed (if we get to the point where w avoid gtk initialization)
Most of the changes looks fine to me. But I'd like to start using GnomeIdleMonitor (bug 688841). gnome-desktop currently requires GTK+ for the bg crossfade code though, so we'd need to move the bg code somewhere else, then I can use gnome-desktop without GTK+, and make gnome-session use GnomeIdleMonitor. Clear? :)
Yes, very clear :-) To reiterate, I'm not super-concerned about dropping the gtk dependency in the short term. In the medium-term, background rendering moves to the shell.
I've pushed most of the easy patches; the remaining interesting ones are below
Created attachment 229699 [details] [review] Make the fallback end session dialog a separate program
Created attachment 229701 [details] [review] Make the fallback end session dialog a separate program
Created attachment 229702 [details] [review] drop the user of eggdesktopfile
Created attachment 229739 [details] [review] drop use of eggdesktopfile
Review of attachment 229701 [details] [review]: Any chance you could split that up in 2 patches, one adding the new program, and another converting to use that new tool.
(In reply to comment #10) > Review of attachment 229701 [details] [review]: > > Any chance you could split that up in 2 patches, one adding the new program, > and another converting to use that new tool. I can try - it will require some gymnastics, because I am reusing some source files and just move them over from being compiled into gnome-session to being compiled into gnome-session-failed.
Review of attachment 229739 [details] [review]: Seems to work with the below changes. I'll post an updated patch. ::: gnome-session/gsm-autostart-app.c @@ +138,3 @@ current_desktop = gsm_util_get_current_desktop (); + g_desktop_app_info_set_desktop_env (current_desktop); + if (!g_app_info_should_show (G_APP_INFO (priv->app_info))) { should_show isn't right here. A lot of the desktop files are NoDisplay, also it doesn't take current_desktop into account, but that's more minor. Should be current_desktop != NULL && g_desktop_app_info_get_show_in (..., current_desktop) @@ +141,2 @@ if (current_desktop) { g_debug ("app %s not installed or not for %s", TryExec is checked when the AppInfo is created, so no installed isn't a potential failure case. @@ +698,3 @@ + app->priv->app_info = g_desktop_app_info_new_from_filename (desktop_filename); + if (app->priv->app_info == NULL) { + g_warning ("Could not parse desktop file %s", desktop_filename); this could fail for parse error or TryExec not finding the executable now. @@ +1033,3 @@ g_assert (startup_id != NULL); env[0] = g_strdup_printf ("DESKTOP_AUTOSTART_ID=%s", startup_id); this is unused now and should get dropped @@ +1054,1 @@ g_free (env[0]); this line can be dropped @@ +1272,3 @@ + provides_str = g_desktop_app_info_get_string (aapp->priv->app_info, + GSM_AUTOSTART_APP_PROVIDES_KEY); + need to handle null provides_str here
Created attachment 230665 [details] [review] Drop use of EggDesktopfile in GsmAutostartApp This requires a few additions to the GDesktopAppInfo API, which have landed in GLib 2.35.3.
Those changes look good to me.
Comment on attachment 230665 [details] [review] Drop use of EggDesktopfile in GsmAutostartApp Attachment 230665 [details] pushed as a0741c8 - Drop use of EggDesktopfile in GsmAutostartApp
Review of attachment 229701 [details] [review]: This patch is pretty old, do we still want it?
Review of attachment 229701 [details] [review]: We'd still want something close to it, if it still applies.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of gnome-session, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-session/-/issues/ Thank you for your understanding and your help.