GNOME Bugzilla – Bug 741666
should make gnome-shell dbus activatable
Last modified: 2021-07-05 14:41:47 UTC
this will require 4 things as i understand it 1) renaming gnome-shell.desktop to org.gnome.Shell.desktop 2) adding DBusActivatable=true 3) Dropping gnome-shell-classic.desktop (already done see bug 741660 ) 4) Dropping gnome-shell-wayland.desktop . This requires "doing the right thing" based on the logind session type (see bug 741658 for the GDM work that was required)
Why?
For more details see: https://mail.gnome.org/archives/desktop-devel-list/2014-January/msg00081.html tl;dr the plan is to make all gnome components dbus activatable, then use the glib APIs for activating those components instead of spawning them directly. They'll get run inside the systemd user session this way and we'll be one step closer to our utopian future of flying cars.
i need to test these, but here's my first draft...
Created attachment 292921 [details] [review] core: move backend setting to helper function This paves the way for making the backend setting be more automatic.
Created attachment 292922 [details] [review] core: start as wayland display server if run when there's a wayland session registered This commit makes gets rid of the need for --display-server and --wayland when mutter detects that a wayland session is registered.
Created attachment 292924 [details] [review] data: drop mutter-wayland.desktop It's not needed anymore, now that we can automatically figure things out based on logind.
Created attachment 292925 [details] [review] data: drop gnome-shell-wayland.desktop We can autodetect what to do, based on the user session, so drop the separate desktop file
Review of attachment 292925 [details] [review]: ::: data/gnome-shell-wayland.desktop.in.in @@ -11,3 @@ -OnlyShowIn=GNOME; -NoDisplay=true -X-GNOME-Autostart-Phase=DisplayServer So one problem is gnome-shell.desktop uses the Autostart-Phase=WindowManager not DisplayServer.
Given we already start the window manager before the initialization phase in wayland, i guess it's probably okay to start the window manager before the initialization phase in X too (since any ordering problems would crop up in wayland as well), so we can probably use DisplayServer for both.
another idea is we could have two bus names... org.gnome.DisplayServer and org.gnome.Shell. the former would be required for the wayland session definition and the latter would be required by the non-wayland session definition.
Review of attachment 292922 [details] [review]: ::: src/core/main.c @@ +303,3 @@ + char **sessions = NULL; + char *session_id = NULL; + int result, i; I usually use "ret" for this variable. Style preference. @@ +307,3 @@ + result = sd_pid_get_session (0, &session_id); + + if (result == 0 && session_id != NULL) I usually prefer: if (ret < 0) goto out; @@ +354,3 @@ + } + + free (sessions); g_strfreev ? @@ +369,3 @@ + +#ifdef HAVE_NATIVE_BACKEND + session_id = find_logind_session (&session_type); Do we actually need both the session ID and the session type? You could make find_logind_session much simpler if you don't. @@ +376,3 @@ + is_wayland = g_strcmp0 (session_type, "wayland") == 0; + free (session_id); + free (session_type); You should free in every case, in case it's possible to fill in the session type without filling in the session ID. @@ +389,3 @@ + +#if defined(HAVE_WAYLAND) && defined(HAVE_NATIVE_BACKEND) + session_type_is_wayland = check_for_wayland_session_type (); Hm. This is guarded with HAVE_WAYLAND && HAVE_NATIVE_BACKEND, but check_for_wayland_session_type(); is only guarded with HAVE_WAYLAND_BACKEND, leading to an unused warning in one case. You should be able to simplify by using the same guards in both places.
Review of attachment 292921 [details] [review]: ::: src/core/main.c @@ +305,3 @@ + meta_set_is_wayland_compositor (opt_wayland); +#endif + Extra whitespace.
(In reply to comment #11) > Review of attachment 292922 [details] [review]: > > ::: src/core/main.c > @@ +303,3 @@ > + char **sessions = NULL; > + char *session_id = NULL; > + int result, i; > > I usually use "ret" for this variable. Style preference. sure, I actually usually use ret too, and even wrote ret this time and changed it to result at the last minute...not sure why. > > @@ +307,3 @@ > + result = sd_pid_get_session (0, &session_id); > + > + if (result == 0 && session_id != NULL) > > I usually prefer: > > if (ret < 0) > goto out; This isn't actually a case of "if fail, quit". the code first looks if you're in a session and if so uses it. Otherwise, if you're running outside of a logind session (as would be the case with systemd --user) then it looks for the session. what i could do is if (ret == 0) goto out; and then do the sd_session_get_type code in the out: path. > > @@ +354,3 @@ > + } > + > + free (sessions); > > g_strfreev ? systemd doesn't use g_malloc. not that it matters in practice, since g_malloc by default calls malloc, but we generally try to pretend like it matters. > Do we actually need both the session ID and the session type? You could make > find_logind_session much simpler if you don't. we need the session id to get the session type. We don't need to return the session id. I can stop doing that. I'm a little worried we're going to need the session id down the line and this hairy code will get duplicated, but we can cross that bridge when we get to it. > > @@ +376,3 @@ > + is_wayland = g_strcmp0 (session_type, "wayland") == 0; > + free (session_id); > + free (session_type); > > You should free in every case, in case it's possible to fill in the session > type without filling in the session ID. That would be non-conventional. most apis that have out args, leave those out args "undefined" if the call fails. Of course this is all moot, since i'm going to change it to session_type = find_logind_session_type ();
Created attachment 293093 [details] [review] ui: disable GL in gdk backend initializing GL leads to deadlock, since it calls into xwayland, which calls back into mutter. This commit disables it. Longer term we should probably stop using gtk inside mutter.
Created attachment 293094 [details] [review] core: move backend setting to helper function This paves the way for making the backend setting be more automatic.
Created attachment 293095 [details] [review] core: start as wayland display server if run when there's a wayland session registered This commit makes gets rid of the need for --display-server and --wayland when mutter detects that a wayland session is registered.
Created attachment 293096 [details] [review] data: drop mutter-wayland.desktop It's not needed since we can automatically figure things out based on logind.
Created attachment 293097 [details] [review] data: merge gnome-shell.desktop and gnome-shell-wayland.desktop into one file We can autodetect what to do based on the user's session, so drop the separate desktop files. This, along with some future changes, will enable us to be bus activatable.
(In reply to Ray Strode [halfline] from comment #9) > Given we already start the window manager before the initialization phase in > wayland, i guess it's probably okay to start the window manager before the > initialization phase in X too (since any ordering problems would crop up in > wayland as well), so we can probably use DisplayServer for both. So there actually are some ordering issues with wayland that we need to tackle. see bug 738205
Comment on attachment 293093 [details] [review] ui: disable GL in gdk backend this was committed some time ago.
Comment on attachment 293094 [details] [review] core: move backend setting to helper function (getting this one in now because it's just a refactor) Attachment 293094 [details] pushed as db4355b - core: move backend setting to helper function
Created attachment 315022 [details] [review] data: rename gnome-shell required component to org.gnome.Shell gnome-shell and gnome-shell-wayland have been consolidated, and now get dbus activated. The new name is org.gnome.Shell. This commit changes the session files to reference org.gnome.Shell.
unless there's any complaints in the next couple days I'll probably push these out on monday sometime
Attachment 293095 [details] pushed as f251f38 - core: start as wayland display server if run when there's a wayland session registered Attachment 293096 [details] pushed as 7b20d15 - data: drop mutter-wayland.desktop
Comment on attachment 293097 [details] [review] data: merge gnome-shell.desktop and gnome-shell-wayland.desktop into one file Attachment 293097 [details] pushed as b8e29ae - data: merge gnome-shell.desktop and gnome-shell-wayland.desktop into one file
Attachment 315022 [details] pushed as 95cc979 - data: rename gnome-shell required component to org.gnome.Shell
A patch from this series caused a regression. A filed it as bug 758658.
It seems that this changes the way to start gnome-shell wayland from the command line? What's the new way?
So I never actually did 2) in comment 0. Reopening.
note it's not enough to merely do 2) from comment 0 . We have a few dependencies on $XDG_SESSION_ID in mutter and gnome-shell
Review of attachment 293097 [details] [review]: ::: data/gnome-shell.desktop.in.in @@ +11,3 @@ OnlyShowIn=GNOME; NoDisplay=true +X-GNOME-Autostart-Phase=DisplayServer This caused a HiDpi regression on X. See bug 765196
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, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.