GNOME Bugzilla – Bug 707467
Merge the wip/wayland display branch
Last modified: 2013-09-12 08:37:51 UTC
So, it seems stuff doesn't move forward unless it's in bugzilla... Patch "Don't pass on X events to Clutter" is a bit iffy, IIRC Rob and Neil wanted to change MetaPluginManager to handle this internally, but it hasn't happened and to me it's late now to change the API in a way that needs coordination between mutter, mutter-wayland and gnome-shell. Patch "ShellGlobal: don't use Gdk to query the pointer position under wayland" relies on clutter bug 706494.
Created attachment 254073 [details] [review] wayland: Add a --display-server option This makes Gnome Shell run as its own display server.
Created attachment 254074 [details] [review] wayland: Create a dummy stage window when running as a display server When Mutter is running as a display server then Clutter won't be using the X11 backend so there won't be a window for the stage. The shell global keeps track of this stage window to be used in various places. This patch adds a dummy window that is never mapped to use instead of the stage window so that the various places that are using it might be able to continue working without generating X errors.
Created attachment 254075 [details] [review] wayland: Don't set properties on the overlay window on Wayland When Mutter is running as a display server it won't create an overlay window. Therefore we shouldn't need to set the DND proxy on the window. This was previously causing X errors when it tried to set a property on an invalid window.
Created attachment 254076 [details] [review] Don't pass on X events to Clutter The Mutter plugin manager has now been changed so that it itself will pass on the events through Clutter so there is no need to do this in Gnome Shell anymore.
Created attachment 254077 [details] [review] ShellGlobal: avoid one use of ClutterX11 clutter_x11_get_stage_window() will abort if clutter is not using the x11 backend (such as when running under DRM). We already have a fake X window in ShellGlobal, let's use that instead.
Created attachment 254079 [details] [review] StEntry: don't call ClutterX11 methods if Clutter is not using x11 It causes an assert and crashes.
Created attachment 254080 [details] [review] ShellGlobal: don't use Gdk to query the pointer position under wayland Gdk uses Xwayland, so it only sees the events we forward to X11 clients. Instead, under wayland we can use clutter and be sure that the value it provides reflect the actual pointer position.
Created attachment 254081 [details] [review] Add support for running wayland under gnome-session Include a .desktop file that autostarts us as a wayland compositor.
Review of attachment 254073 [details] [review]: I really don't like '--display-server' at all. Can we just have '--wayland' ?
Review of attachment 254074 [details] [review]: This is a disgusting hack, and I'd want to verify all the places we use the stage window to make sure that this does what we want. Tentatively marking reviewed until I can double-check.
Review of attachment 254075 [details] [review]: How exactly is XDnD going to work under Wayland? Are we going to provide fake X windows for the panel? Are we just going to drop support for it. I'm fine with this as a short-term thing, but only if we have a long-term plan going forward.
Review of attachment 254076 [details] [review]: So the mutter plugin manager interface differs between wayland and not-wayland? Fix it to be consistent, and if we need to adapt here, file a different patch.
Review of attachment 254077 [details] [review]: Can we investigate if this IBus hack is actually still needed? I thought it was fixed upstream. ACN if it's not, and we don't have any other short-term solutions.
Review of attachment 254079 [details] [review]: Do we have an idea how input methods will work under Wayland? I understand we're running under GDK with the X11 backend, but has GTK+ implemented the new input method protocol? This is really a "don't crash" fix more than anything; I'd rather see us adapt to a new input method API if GTK+ has been updated for Wayland support. ACN for short-term if we have no better solution.
Review of attachment 254080 [details] [review]: MetaCursorTracker...
Review of attachment 254081 [details] [review]: needs update for command line option change, otherwise fine
(In reply to comment #14) > Review of attachment 254079 [details] [review]: > > Do we have an idea how input methods will work under Wayland? I understand > we're running under GDK with the X11 backend, but has GTK+ implemented the new > input method protocol? > > This is really a "don't crash" fix more than anything; I'd rather see us adapt > to a new input method API if GTK+ has been updated for Wayland support. > > ACN for short-term if we have no better solution. The st_entry_set_cursor(), yes, is pretty much a "don't crash" fix, because MetaCursorTracker has a giant FIXME for set_cursor() in the wayland case, and because we can't easily call into MetaCursorTracker from StEntry. OTOH, the IM fix is a full one. We need the event window as tag that the event was generated by IBus and it is meant for the stage window, and it works well. Also, well, the new input method API is client side, we're the compositor, we can't use that...
Created attachment 254165 [details] [review] wayland: Add a --wayland option This makes Gnome Shell run as a wayland compositor
Created attachment 254166 [details] [review] wayland: Create a dummy stage window when running as a display server When Mutter is running as a display server then Clutter won't be using the X11 backend so there won't be a window for the stage. The shell global keeps track of this stage window to be used in various places. This patch adds a dummy window that is never mapped to use instead of the stage window so that the various places that are using it might be able to continue working without generating X errors.
Created attachment 254167 [details] [review] wayland: Don't set properties on the overlay window on Wayland When Mutter is running as a display server it won't create an overlay window. Therefore we shouldn't need to set the DND proxy on the window. This was previously causing X errors when it tried to set a property on an invalid window.
Created attachment 254168 [details] [review] Don't pass on X events to Clutter The Mutter plugin manager has now been changed so that it itself will pass on the events through Clutter so there is no need to do this in Gnome Shell anymore.
Created attachment 254169 [details] [review] ShellGlobal: avoid one use of ClutterX11 clutter_x11_get_stage_window() will abort if clutter is not using the x11 backend (such as when running under DRM). We already have a fake X window in ShellGlobal, let's use that instead.
Created attachment 254170 [details] [review] StEntry: don't call ClutterX11 methods if Clutter is not using x11 It causes an assert and crashes.
Created attachment 254171 [details] [review] ShellGlobal: use MetaCursorTracker to query the pointer position Gdk uses Xwayland, so it only sees the events we forward to X11 clients. Instead, we can use the abstraction API provided by mutter and get the right value automatically. Also, we need to use MetaCursorTracker to handle the cursor visibility too.
Created attachment 254172 [details] [review] Add support for running wayland under gnome-session Include a .desktop file that autostarts us as a wayland compositor.
Review of attachment 254165 [details] [review]: OK.
Review of attachment 254172 [details] [review]: OK.
Review of attachment 254169 [details] [review]: OK.
Review of attachment 254171 [details] [review]: Only other thing I'm worried about is _fakePointerEvent in overview.js. I'm not sure why we have that code, though, so maybe it isn't it worth it for now. Everything else looks good.
Review of attachment 254170 [details] [review]: So, IMText is very X11-specific. What I think makes sense as a shorter-term hack is to have StEntry create an StIMText under X11, and a plain ClutterText under other platforms, and long-term we'll have StIMTextX11 and StIMTextWayland, or use GTK+ in StIMText natively or something. ::: src/st/st-entry.c @@ +688,3 @@ + if (!clutter_check_windowing_backend (CLUTTER_WINDOWING_X11)) + return; Could we rewrite this code using GDK instead to avoid the issue? This is fine as a short-term hack for now.
Review of attachment 254168 [details] [review]: Makes sense.
(In reply to comment #31) > Review of attachment 254170 [details] [review]: > > So, IMText is very X11-specific. What I think makes sense as a shorter-term > hack is to have StEntry create an StIMText under X11, and a plain ClutterText > under other platforms, and long-term we'll have StIMTextX11 and > StIMTextWayland, or use GTK+ in StIMText natively or something. IMText is not X11-specific. Indeed, in the ibus backend, all communication happens through DBus... > ::: src/st/st-entry.c > @@ +688,3 @@ > > + if (!clutter_check_windowing_backend (CLUTTER_WINDOWING_X11)) > + return; > > Could we rewrite this code using GDK instead to avoid the issue? It wouldn't help, Xwayland is not pointer-focused when you hover an StEntry
Comment on attachment 254168 [details] [review] Don't pass on X events to Clutter Pushing this one first, to coordinate with the mutter change. Attachment 254168 [details] pushed as 9658846 - Don't pass on X events to Clutter
Review of attachment 254166 [details] [review]: I dislike this patch. It's not like stage_gdk_window / stage_window / clutter_x11_get_stage_window are used in one consistent way over a ton of places. They are used in a very limited number of places in very different ways: * stage_gdk_window is used *only* to set this cursor via GDK API. This makes no sense for wayland * stage_xwindow is used as an event handling window for Clutter events - this just needs some distinct X window * stage_xwindow is used for grabbing in shell_global_freeze_keyboard() - this just needs some distinct X window * stage_xwindow is used for Xdnd handling. Xdnd the current way is not going to work unless we have a X window with the shape of the shell's input region mapped - which the dummy window doesn't work at all for. I think we're probably better just dropping Xdnd support in Wayland until we get the Xdnd => wayland DND bridging going, which will require similar things. So my suggestion would be for the things that this dummy window works for, add a new global->event_xwindow - which can be the same as global->stage_xwindow in the non-Wayland build. And for the code that it doesn't make sense for, don't enable that code when building gnome-shell-wayland. I don't think that's any more complex than this patch. The API for passing the event window to St shoudln't be st_im_text_set_fake_window, since there's nothing really "fake" about that window it's a real window - probably set_event_window()
Review of attachment 254167 [details] [review]: The Xdnd code should just be disabled when running in Wayland unless we are preserving the idea of using an X window for deciding which events get delivered to shell chrome, which, AFAIK, we aren't.
Review of attachment 254170 [details] [review]: Seems to be multiple things here. - The cursor code should be using common code with shell_global_set_cursor() (can be hacked out for the moment if we have code in progress to handle that) - The IMText stuff is 100% fine and will work as long as we are using the X11 backend to GTK+ inside Mutter. The only real alternative would be to rewrite StIMContext to talk to the IBus daemon directly.
(In reply to comment #37) > Review of attachment 254170 [details] [review]: > > Seems to be multiple things here. > > - The cursor code should be using common code with shell_global_set_cursor() > (can be hacked out for the moment if we have code in progress to handle that) Technically, that would be breaking a boundary. St doesn't link against Shell right now -- Shell links against St.
(In reply to comment #38) > (In reply to comment #37) > > Review of attachment 254170 [details] [review] [details]: > > > > Seems to be multiple things here. > > > > - The cursor code should be using common code with shell_global_set_cursor() > > (can be hacked out for the moment if we have code in progress to handle that) > > Technically, that would be breaking a boundary. St doesn't link against Shell > right now -- Shell links against St. We can add a hook function to St that the shell fills in, for example.
Created attachment 254708 [details] [review] ShellGlobal: remove cursor manipulation functions Use the new API in MetaScreen instead, which is automatically routed to MetaCursorTracker as appropriate.
Created attachment 254709 [details] [review] ShellGlobal: repurpose the stage_gdk_window as the IBus window And use it directly in the IBus event handling (by pushing it down to StIMText)
Created attachment 254710 [details] [review] Disable XDND when running as a wayland compositor We can't do xdnd on wayland easily, so let's disable this for 3.10
Created attachment 254711 [details] [review] ShellGlobal: use a different window for IBus when running on wayland When running as a wayland compositor, the clutter stage doesn't have an usable window for IPC, so just create another one. Also, disable freezing the keyboard when running on wayland, as we can't do it really.
Created attachment 254712 [details] [review] StEntry: add a hook to set the cursor from libgnome-shell We need to call into MetaScreen to set the cursor, but we can't do that from libst, so add a hook that libgnome-shell can fill, and remove more ClutterX11 usage.
Created attachment 254713 [details] [review] ShellGlobal: use MetaCursorTracker to query the pointer position Gdk uses Xwayland, so it only sees the events we forward to X11 clients. Instead, we can use the abstraction API provided by mutter and get the right value automatically. Also, we need to use MetaCursorTracker to handle the cursor visibility too.
Created attachment 254714 [details] [review] Add support for running wayland under gnome-session Include a .desktop file that autostarts us as a wayland compositor.
Reworked heavily to address Owen's comments, now depends on mutter bug 707919 for meta_screen_set_cursor() and the new cursor types. Tested on x11 and wayland-kms.
Review of attachment 254713 [details] [review]: OK.
Review of attachment 254708 [details] [review]: OK.
Review of attachment 254708 [details] [review]: I'll trust you got the translation right; looks good. ::: src/shell-global.c @@ +28,3 @@ #include <meta/util.h> #include <meta/meta-shaped-texture.h> +#include <meta/meta-cursor-tracker.h> Seems to be unused?
Review of attachment 254709 [details] [review]: OK.
Review of attachment 254710 [details] [review]: OK.
Review of attachment 254711 [details] [review]: Can you always make gnome-shell use the IBus window, instead of just when running as a Wayland compositor?
Review of attachment 254712 [details] [review]: Not overly happy with it, but it works, so OK.
In the end I kept reusing the stage GDK window for IBus. It shouldn't matter, but as I said in IRC, we should aim for minimizing changes in the non-wayland paths. Anyway, we can fix it later if we absolutely want to. Attachment 254708 [details] pushed as 11c2933 - ShellGlobal: remove cursor manipulation functions Attachment 254709 [details] pushed as 9d1f789 - ShellGlobal: repurpose the stage_gdk_window as the IBus window Attachment 254710 [details] pushed as 4db6e70 - Disable XDND when running as a wayland compositor Attachment 254711 [details] pushed as ba9c1d9 - ShellGlobal: use a different window for IBus when running on wayland Attachment 254712 [details] pushed as 8ae0f1a - StEntry: add a hook to set the cursor from libgnome-shell Attachment 254713 [details] pushed as c584488 - ShellGlobal: use MetaCursorTracker to query the pointer position Attachment 254714 [details] pushed as 135727c - Add support for running wayland under gnome-session