GNOME Bugzilla – Bug 708201
Core dumped when running on Wayland
Last modified: 2014-03-08 12:49:35 UTC
Rhythmbox still uses gdk_x11_* interfaces. In order to run on Wayland, these interfaces should be made optional and made to use for X11 sessions only or replaced with Wayland equivalents when running on a wayland compositor. lib/eggdesktopfile.c: launch_time = gdk_x11_display_get_user_time (display); lib/eggdesktopfile.c: gdk_x11_display_broadcast_startup_message (display, "new", lib/eggdesktopfile.c: gdk_x11_display_broadcast_startup_message (display, "remove", lib/rb-missing-plugins.c: xid = gdk_x11_window_get_xid (gtk_widget_get_window (GTK_WIDGET (parent_window))); plugins/brasero-disc-recorder/rb-disc-recorder-plugin.c: xid = gdk_x11_window_get_xid (window); See https://wiki.gnome.org/Wayland/#Porting_GNOME_to_Wayland
I'm not going to be testing anything on wayland any time soon. If you want this fixed you'll probably need to come up with some patches.
Armin: Those egg files should hopefully just require putting the latest version there.
Olav: I just looked at the file in git repository, it's more or less the same. My belief is that if I wrap startup notification code to run only on gdk x11, it shouldn't harm anything - plus I don't think that startup notification is yet supported on wayland compositors or in gdk wayland api.
Created attachment 255093 [details] [review] First try With this patch, I've been able to start rhythmbox on weston.
Created attachment 255094 [details] [review] Second try This fixes mmkeys on x11 without gsd running. Ran comparison on an unitilized variable.
Review of attachment 255093 [details] [review]: ::: lib/eggdesktopfile.c @@ +34,2 @@ #include <gtk/gtk.h> +#ifdef GDK_WINDOWING_X11 I don't think you should modify eggdesktopfile.c. It's used by the XSMP session saving code, which is useless in Wayland, and this code would need to be "fixed" in libegg anyway. I would recommend that Rhythmbox moves away from using that code, and saves its state regularly (that's what I did in Totem for example). ::: plugins/brasero-disc-recorder/rb-disc-recorder-plugin.c @@ +177,3 @@ } + + if(xid_str) That's not needed, g_free handles NULL. ::: plugins/mmkeys/rb-mmkeys-plugin.c @@ +417,1 @@ rb_debug ("attempting old-style key grabs"); I personally think that the "old-style" mmkeys grabs should die. In the longer-term, we might add functionality to the MPRIS spec to replace the only thing it doesn't do that g-s-d does (telling us which is the last application to be focused) and remove the g-s-d code altogether.
Review of attachment 255094 [details] [review]: Same comments as for the other patch so marking as needs-work as well. (do try git-bz, available on git.gnome.org, it will make uploading patches much easier, including automatically marking old patches as obsolete for example).
Created attachment 260450 [details] [review] Remove eggdesktopfile usage
Created attachment 260453 [details] [review] Remove eggdesktopfile usage
Created attachment 260454 [details] [review] Don't try to use old mmkeys grab unless running on X11
Created attachment 260455 [details] [review] Don't run gdk_x11_* functions unless running on X11
Review of attachment 260453 [details] [review]: seems ok, I'll figure out how much I care about g_set_prgname and commit it. ::: plugins/mpris/rb-mpris-plugin.c @@ +277,3 @@ +#else + path = g_build_filename (DATADIR, "applications", "rhythmbox.desktop", NULL); +#endif I don't really like having plugins aware of this much detail, but I'll live with it. ::: shell/main.c @@ +77,3 @@ #endif + g_set_prgname ("rhythmbox"); GApplication does this for us, doesn't it?
Review of attachment 260454 [details] [review]: ::: plugins/mmkeys/rb-mmkeys-plugin.c @@ +347,3 @@ g_clear_error (&error); #ifdef HAVE_MMKEYS + if (GDK_IS_X11_DISPLAY (gdk_display_get_default ())) { is this how gtk/wayland people recommend doing this?
(In reply to comment #13) > Review of attachment 260454 [details] [review]: > > ::: plugins/mmkeys/rb-mmkeys-plugin.c > @@ +347,3 @@ > g_clear_error (&error); > #ifdef HAVE_MMKEYS > + if (GDK_IS_X11_DISPLAY (gdk_display_get_default ())) { > > is this how gtk/wayland people recommend doing this? Yes. Though it's wrapped in #ifdef GDK_WINDOWING_X11 (which HAVE_MMKEYS is more or less equal to).
(In reply to comment #13) > Review of attachment 260454 [details] [review]: > > ::: plugins/mmkeys/rb-mmkeys-plugin.c > @@ +347,3 @@ > g_clear_error (&error); > #ifdef HAVE_MMKEYS > + if (GDK_IS_X11_DISPLAY (gdk_display_get_default ())) { > > is this how gtk/wayland people recommend doing this? It is necessarry when GTK+3 is compiled with both X11 and Wayland backends. #ifdef only checks if code is to be built, but it will run on non-X11 platform in case GTK+3 is built with multiple backends. That's why runtime check was necessarry, else I get core dumps which I traced to legacy mmkeys grabbing.
(In reply to comment #12) > Review of attachment 260453 [details] [review]: > > seems ok, I'll figure out how much I care about g_set_prgname and commit it. > > ::: plugins/mpris/rb-mpris-plugin.c > @@ +277,3 @@ > +#else > + path = g_build_filename (DATADIR, "applications", "rhythmbox.desktop", > NULL); > +#endif > > I don't really like having plugins aware of this much detail, but I'll live > with it. > > ::: shell/main.c > @@ +77,3 @@ > #endif > > + g_set_prgname ("rhythmbox"); > > GApplication does this for us, doesn't it? I am not familiar with GApplication. If you say so, you can just remove the line before you apply the patch. I didn't notice any problems either at build time or runtime with that line present.
Pushed as 7066bd2, f4b18d4 and d97dcd0. Is there more to do here?
No, I don't think so.
OK, I'll close this bug. If you have more patches for wayland issues, please open new bugs.