GNOME Bugzilla – Bug 705917
wayland: implement support for plugin modality
Last modified: 2013-08-30 08:39:25 UTC
Let's get modals working in gnome-shell, for dialogs, menus and most important the activities overview. (Currently they fail because ShellGlobal uses a fake X window, which is never mapped, so we get GrabNotViewable)
Created attachment 251508 [details] [review] wayland: implement support for plugin modality Calling XIGrabDevice has no effect under wayland, because the xserver is getting events from us. Instead, we need to use our own interfaces for grabs. At the same time, we can simplify the public API, as plugins should always listen for events using clutter.
Created attachment 251509 [details] [review] ShellGlobal: update for Mutter API changes meta_plugin_begin_modal() was simplified to always grab on the stage, which is what we want.
Review of attachment 251508 [details] [review]: Just so I'm clear, this doesn't fix meta_display_begin_grab_op. Are we going to continue making that go through XWayland? ::: src/compositor/compositor.c @@ +516,3 @@ + + if ((options & META_MODAL_POINTER_ALREADY_GRABBED) == 0) + { No braces. @@ +521,3 @@ + } + if ((options & META_MODAL_KEYBOARD_ALREADY_GRABBED) == 0) + { No braces. @@ +532,3 @@ + meta_wayland_pointer_end_modal (&compositor->seat->pointer); + if (keyboard_grabbed) + meta_wayland_keyboard_end_modal (&compositor->seat->keyboard); pointer_grabbed / keyboard_grabbed are never set. ::: src/wayland/meta-wayland-keyboard.c @@ +573,3 @@ + uint32_t state) +{ +} I can't help but wonder if it would not be easier to just check for NULL function pointers when calling grab->interface->key. Do we ever want to move anything into handlers like this (for window moves like Weston does, etc.) or will we just want to go through Clutter/X always? (And if not, why not just replace the GrabInterface stuff with a boolean for is_grabbed that prevents normal delivery?)
Review of attachment 251509 [details] [review]: OK.
Ok, so... I don't like the current grab interface (it feels foreign and bolt-on in the existing code base), but I don't like the Central Grand Station with the giant switch. This is the smallest patch that I could think of, to handle what we need for gnome-shell and still be runtime choosable. As you know, I'm in favor of a fork, which would allow us to rewrite the input handling entirely. In the my Brave New World, it would work like this: - All input events come from clutter, and we never call clutter_grab() or the like - Global keybindings are handled in a captured-event on the stage - Mouse grab operations are also handled in captured-event (and they prevent normal delivery of event by returning TRUE) - Input events are converted into wayland events by MetaWindowActor/MetaShapedTexture - We never passively or actively X grab - When we need modality for the shell we either hide the window group, add a reactive pane or install another captured-event handler - Gdk events on our windows are synthetized, and don't go through xwayland at all - Other X events (ConfigureRequest/Notify, MapRequest/Notify, FocusIn/Out, etc.) get handled as usual through the Gdk event filter It's simple, it's cleaner, but it's totally incompatible with mutter as an X11 wm, so it something for 3.12/3.14.
(In reply to comment #5) > - Mouse grab operations are also handled in captured-event (and they prevent > normal delivery of event by returning TRUE) Considering multiple captured-event handlers stacking is causing a lot of pain when trying to manage GrabHelper, I'm not sure this is the greatest idea. We certainly don't want to pop up an event blocker when dragging a scrollbar around, and we need to make sure it's all integrated so that its captured-event handler is at the start, otherwise scrollbars in menus won't work properly. We're effectively building our own stacked grab system. If you want to manage that, then go for it.
(In reply to comment #6) > (In reply to comment #5) > We're effectively building our own stacked grab system. If you want to manage > that, then go for it. Well, none of the two other systems stack, so... any better ideas? :)
Created attachment 251749 [details] [review] wayland: implement support for plugin modality Calling XIGrabDevice has no effect under wayland, because the xserver is getting events from us. Instead, we need to use our own interfaces for grabs. At the same time, we can simplify the public API, as plugins should always listen for events using clutter.
Review of attachment 251749 [details] [review]: I assume you're going to make the same change on the X11 branch as well? ::: src/compositor/compositor.c @@ +528,3 @@ + goto fail; + + keyboard_grabbed = TRUE; Issue with the existing X11 code, but there's no way for this to ever become TRUE in the fail path. Feel free to commit with this in here, and I'll make a cleanup removing it later for both X11 and Wayland. ::: src/core/display.c @@ +1912,2 @@ if (display->focus_xwindow == xwindow && + display->focus_type == type && Where is this added? It's not on the current 'wayland' branch as far as I can tell. ::: src/wayland/meta-wayland.c @@ +537,3 @@ meta_window_unmanage (surface->window, timestamp); } + else if (!surface->window) Seems unrelated?
(In reply to comment #9) > Review of attachment 251749 [details] [review]: > > ::: src/core/display.c > @@ +1912,2 @@ > if (display->focus_xwindow == xwindow && > + display->focus_type == type && > > Where is this added? It's not on the current 'wayland' branch as far as I can > tell. Bug 706364
Created attachment 252339 [details] [review] MetaPlugin: simplify the modal API Remove grab window and cursor from the API, and just grab always on the stage window with no cursor. This is mainly to remove the X11 usage in the public API, in preparation for implementing this in wayland. Ok, this is for the X11 branch.
Review of attachment 252339 [details] [review]: Looks good.
Comment on attachment 252339 [details] [review] MetaPlugin: simplify the modal API I'm going to push this now so it makes 3.9.90, and we don't need to break API later on, which can be problematic in that we'll need mutter, mutter-wayland and gnome-shell all in sync. Attachment 252339 [details] pushed as 3f2dcf1 - MetaPlugin: simplify the modal API
Comment on attachment 251509 [details] [review] ShellGlobal: update for Mutter API changes Attachment 251509 [details] pushed as f7c3cf5 - ShellGlobal: update for Mutter API changes
Created attachment 253404 [details] [review] wayland: implement support for plugin modality Calling XIGrabDevice has no effect under wayland, because the xserver is getting events from us. Instead, we need to use our own interfaces for grabs. At the same time, we can simplify the public API, as plugins should always listen for events using clutter. Reattaching after the rebase.
Review of attachment 253404 [details] [review]: ::: src/wayland/meta-wayland.c @@ +522,3 @@ meta_window_unmanage (surface->window, timestamp); } + else if (!surface->window) Again; seems unrelated?
Created attachment 253408 [details] [review] wayland: implement support for plugin modality Calling XIGrabDevice has no effect under wayland, because the xserver is getting events from us. Instead, we need to use our own interfaces for grabs. At the same time, we can simplify the public API, as plugins should always listen for events using clutter.
Created attachment 253409 [details] [review] wayland: don't free surfaces that have a window associated After a MetaWaylandSurface is associated with a MetaWindow, it should be freed only when the MetaWindow is unmanaged. For wayland clients, the window is unmanaged when the resource is destroyed, but for X11 clients we want to wait for the unmap event.
Review of attachment 253409 [details] [review]: OK.
Review of attachment 253408 [details] [review]: looks good
Attachment 253408 [details] pushed as 7eb4bfb - wayland: implement support for plugin modality Attachment 253409 [details] pushed as deeb1db - wayland: don't free surfaces that have a window associated