GNOME Bugzilla – Bug 783342
Wayland: RFC - add xwayland keyboard grab / shortcut inhibitor support
Last modified: 2018-01-25 15:37:24 UTC
Description: The following series of patch aims at implementing support for Xwayland keyboard grab and shortcut inhibitor support in mutter. These two protocols have been previously discussed here: https://lists.freedesktop.org/archives/wayland-devel/2017-March/033526.html https://lists.freedesktop.org/archives/wayland-devel/2017-April/033638.html https://lists.freedesktop.org/archives/wayland-devel/2017-April/033800.html https://lists.freedesktop.org/archives/wayland-devel/2017-May/034130.html https://lists.freedesktop.org/archives/wayland-devel/2017-June/034223.html Patch for Xwayland can be found here: https://lists.freedesktop.org/archives/xorg-devel/2017-June/053830.html Note: These two protocols are still under review/discussion on wayland-devel mailing and may be subject to change before they are approved in wyland-protocol, the patches posted here are for reference and preliminary discussions.
Created attachment 353059 [details] [review] [PATCH 1/4] configure.ac: update wayland-server requirement Raise wayland-server requirement to 1.13.0 to use the new API to control wl_global visibility, so that we can hide Xwayland specific protocols from other regular clients.
Created attachment 353060 [details] [review] [PATCH 2/4] wayland: add inhibit shortcut mechanism Add a mechanism to MetaWaylandSurface that inhibits compositor's own shortcuts when the surface has input focus, so that clients can receive all key events regardless of the compositor own shortcuts. This will help with implementing "fake" active grabs in Wayland and XWayland clients.
Created attachment 353061 [details] [review] [PATCH 3/4] wayland: add Xwayland grab keyboard support his protocol is limited to Xwayland only and is not visible/usable by any other client.
Created attachment 353062 [details] [review] [PATCH 4/4] wayland: add keyboard shortcuts inhibitor protocol
Created attachment 353064 [details] [review] [PATCH 4/4 v2] wayland: add keyboard shortcuts inhibitor protocol v2: add a surface listener to avoid a crash if the surface is gone when restorign shortcuts (e.g. menu)
Created attachment 353078 [details] [review] [PATCH 3/4 v2] wayland: add Xwayland grab keyboard support v2: fix null-pointer deref when surface is gone in xwayland protocol
Created attachment 353391 [details] [review] [PATCH 2/4 v2] wayland: add inhibit shortcut mechanism adds new keybinding flag ("non_maskable") and signals for surfaces
Created attachment 353393 [details] [review] [PATCH 4/4 v3] wayland: add keyboard shortcuts inhibitor protocol Connect signals to send active/inactive events, few bug fixes
Created attachment 353405 [details] [review] [PATCH 2/4 v3] wayland: add inhibit shortcut mechanism
Created attachment 353880 [details] [review] [PATCH 1/9] configure.ac: update wayland-server requirement Raise wayland-server requirement to 1.13.0 to use the new API to control wl_global visibility, so that we can hide Xwayland specific protocols from other regular clients.
Created attachment 353881 [details] [review] [PATCH 2/8] wayland: add inhibit shortcut mechanism Add a mechanism to MetaWaylandSurface that inhibits compositor's own shortcuts when the surface has input focus, so that clients can receive all key events regardless of the compositor own shortcuts. This will help with implementing "fake" active grabs in Wayland and XWayland clients.
Created attachment 353882 [details] [review] [PATCH 3/8] wayland: add Xwayland grab keyboard support This protocol is limited to Xwayland only and is not visible/usable by any other client.
Created attachment 353883 [details] [review] [PATCH 4/8] wayland: add keyboard shortcuts inhibitor protocol
Created attachment 353884 [details] [review] [PATCH 5/8] core: add MetaInhibitShortcutsDialog Add a new interface for allowing or denying shortcuts inhibit requests.
Created attachment 353885 [details] [review] [PATCH 6/8] core: implement MetaInhibitShortcutsDialogDefault this is the default implementation of the inhibit shortcuts dialog, using a notification rather than an dialog. The notification message informs the user that a shortcut inhibitor is requested, and how to force the normal shortcut processing if desired using the special key combo.
Created attachment 353886 [details] [review] [PATCH 7/8] compositor: add vmethod to override inhibit shortcut dialog A MetaPlugin implementation of the MetaInhibitShortcutsDialog can be used in place of the default inhibit shortcut.
Created attachment 353887 [details] [review] [PATCH 8/8] wayland: use the inhibit shortcuts dialog Plug the new MetaInhbitShortcutsDialog to the relevant Wayland protocol implementation.
(In reply to Olivier Fourdan from comment #15) > Created attachment 353885 [details] [review] [review] > [PATCH 6/8] core: implement MetaInhibitShortcutsDialogDefault > A typo: + line1 = g_strdup (_("Application has inhibited shortcutss")); ^^
Review of attachment 353880 [details] [review]: lgtm.
Review of attachment 353881 [details] [review]: ::: src/core/keybindings.c @@ +1733,3 @@ + goto not_found; + } +#endif Can't we have this as a keyboard shortcut state instead instead of wayland specific code here? For example it could be a list of ClutterInputDevices (which in effect will just be the core keyboard device) and check the master device of the device here? That way we don't need to have winsys concepts leaked into here. The Wayland implementations would add/remove its inhibited devices (or a table with device -> num inhibit refs) whenever the correct focus is set. ::: src/core/window-private.h @@ +774,3 @@ +void meta_window_wayland_inhibit_shortcuts (MetaWindow *window); + +void meta_window_wayland_restore_shortcuts (MetaWindow *window); These two should go into src/wayland/meta-window-wayland.h.
Review of attachment 353882 [details] [review]: ::: src/wayland/meta-wayland.c @@ +315,3 @@ +meta_xwayland_global_filter (const struct wl_client *client, + const struct wl_global *global, + void *data) nit: alignment @@ +372,3 @@ + /* Xwayland specific protocol, needs to be filtered out for all other clients */ + if (meta_xwayland_grab_keyboard_init (compositor)) + wl_display_set_global_filter(compositor->wayland_display, nit: space between _filter and ( @@ +374,3 @@ + wl_display_set_global_filter(compositor->wayland_display, + meta_xwayland_global_filter, + NULL); Could just as well pass the compositor here so you don't have to look up the singleton. ::: src/wayland/meta-xwayland-grab-keyboard.c @@ +41,3 @@ + +static gboolean +meta_xwayland_grab_keyboard_key (MetaWaylandKeyboardGrab *grab, nit: A bit confusing naming: a "xwayland grab keyboard" that takes a "keyboard grab". An "xwayland keyboard grab" makes more sense IMHO. @@ +121,3 @@ +static void +meta_xwayland_grab_keyboard_surface_destroyed (struct wl_listener *listener, + void *data) nit: alignment @@ +130,3 @@ + window = active_grab->window; + if (window) + meta_screen_grab_keys (window->screen); What is this call for? @@ +157,3 @@ + + if (!meta_wayland_seat_has_keyboard (seat)) + return; We must always succeed in creating the new object. @@ +172,3 @@ + meta_wayland_surface_inhibit_shortcuts (surface, seat); + + active_grab = g_slice_new0 (MetaXwaylandKeyboardActiveGrab); personal opinion: we gain only harder debugability by using slices, so I try to avoid it. @@ +176,3 @@ + active_grab->keyboard_grab.keyboard = seat->keyboard; + active_grab->surface = surface; + active_grab->window = window; You can always fetch the window from the surface, so no need to keep a separate pointer to it right? Early in its lifetime it will also be NULL for Xwayland surfaces as there is a race regarding window<->surface association. @@ +181,3 @@ + active_grab->surface_listener.notify = meta_xwayland_grab_keyboard_surface_destroyed; + + wl_resource_add_destroy_listener (surface_resource, &active_grab->surface_listener); Just use the "destroy" GSignal on the MetaWaylandSurface instead. @@ +213,3 @@ + if (resource == NULL) + { + wl_client_post_no_memory (client); Mutter will never work on systems without overcommit, so no need for this. @@ +226,3 @@ +{ + return (wl_global_create (compositor->wayland_display, + &zwp_xwayland_keyboard_grab_manager_v1_interface, 1, You should use the META_ZWP_XWAYLAND_KEYBOARD_GRAB_V1_VERSION here instead of 1 and just pass the version variable where you use the macro when creating the child resources.
Review of attachment 353883 [details] [review]: ::: src/wayland/meta-wayland-inhibit-shortcuts.c @@ +58,3 @@ + meta_wayland_surface_restore_shortcuts (shortcut_inhibit->surface, + shortcut_inhibit->seat); + Stray newline. @@ +82,3 @@ +static void +shortcuts_inhibited_cb (MetaWaylandSurface *surface, + gpointer user_data) Can just have correct type here instead of gpointer. We G_CALLBACK () cast these function pointers anyway. (same below) @@ +86,3 @@ + MetaWaylandKeyboardShotscutsInhibit *shortcut_inhibit = user_data; + + zwp_keyboard_shortcuts_inhibitor_v1_send_active (shortcut_inhibit->resource); Should we not only send these when the client has keyboard focus? I mean there might be another client that got the shortcuts inhibited for it, and when that happens I guess we shouldn't send active to this one? @@ +118,3 @@ + + if (!meta_wayland_seat_has_keyboard (seat)) + return; We must always succeed creating the resource. @@ +128,3 @@ + if (keyboard_shortcuts_inhibit_resource == NULL) + { + wl_client_post_no_memory (client); Overcommit is required, so we'll never end up here. @@ +132,3 @@ + } + + shortcut_inhibit = g_slice_new0 (MetaWaylandKeyboardShotscutsInhibit); Slices makes debugging harder, so avoid them when there is no clear gain (are there any these days on modern systems?) @@ +149,3 @@ + + wl_resource_add_destroy_listener (surface_resource, + &shortcut_inhibit->surface_listener); Just use the "destroy" GSingal on MetaWaylandSurface. @@ +194,3 @@ +{ + return (wl_global_create (compositor->wayland_display, + &zwp_keyboard_shortcuts_inhibit_manager_v1_interface, 1, Same as with the Xwayland grab protocol and the version macro. Macro goes here, the "version" variable in the other places. ::: src/wayland/meta-wayland-inhibit-shortcuts.h @@ +37,3 @@ + +#endif /* META_WAYLAND_INHIBIT_SHORTCUTS_H */ + Stray newline.
Review of attachment 353885 [details] [review]: ::: src/core/meta-inhibit-shortcuts-dialog-default.c @@ +180,3 @@ + NULL, NULL, + NULL, 0, + NULL, NULL); Hmm. AFAIK this uses libnotify, and AFAIK plain mutter does not implement the "receiver" end of the notification API, so I don't understand how this can work. When trying to call "zenity --notification ..." in a plain mutter session zenity just fails complaining about no service being available.
Review of attachment 353883 [details] [review]: ::: src/wayland/meta-wayland-inhibit-shortcuts.c @@ +23,3 @@ +#include "config.h" + +#include <wayland-server.h> nit: empty line between system headers and non-system headers. @@ +130,3 @@ + wl_client_post_no_memory (client); + return; + } We should probably here let the resource be a dummy resource if the surface is not something we will ever inhibit. You could probably check if if the surface role is a MetaWaylandSurfaceRoleShellSurface, as only those can ever have keyboard focus.
Review of attachment 353884 [details] [review]: ::: src/core/meta-inhibit-shortcuts-dialog.c @@ +17,3 @@ + */ + +#include "config.h" Newline after including config.h. @@ +18,3 @@ + +#include "config.h" +#include "window-private.h" "core/window-private.h" @@ +22,3 @@ +#include "meta/meta-enum-types.h" + +enum { enum { @@ +39,3 @@ + "Window", + META_TYPE_WINDOW, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS ? ::: src/meta/meta-inhibit-shortcuts-dialog.h @@ +24,3 @@ + +#define META_TYPE_INHIBIT_SHORTCUTS_DIALOG (meta_inhibit_shortcuts_dialog_get_type ()) + Usually now empty line here.
Review of attachment 353886 [details] [review]: lgtm.
Review of attachment 353887 [details] [review]: Overall, it feels like the request/deny dialog interaction can be put directly in meta-wayland-inhibit-shortcuts.c. ::: src/wayland/meta-wayland-inhibit-shortcuts-dialog.c @@ +24,3 @@ +#include "meta-wayland-inhibit-shortcuts-dialog.h" + +struct inhibit_shortcuts_data WrongNamingConvention @@ +32,3 @@ +static void +inhibit_shortcuts_dialog_data_closure_notify (gpointer data, + GClosure *closure) alignment @@ +60,3 @@ + window = meta_wayland_surface_get_toplevel_window (surface); + if (!window) + return FALSE; We should rather make sure we never end up here in the first place, because we should never have a way to inhibit without getting the dialog. ::: src/wayland/meta-wayland-inhibit-shortcuts.c @@ +157,3 @@ + meta_wayland_surface_show_inhibit_shortcuts_dialog (surface, seat); + else + meta_wayland_surface_inhibit_shortcuts (surface, seat); Why are you just triggering the dialog when the surface is the toplevel one? If it's a popup, we should also query/notify about it. ::: src/wayland/meta-wayland-surface.h @@ +245,3 @@ + + /* Managed by meta-wayland-inhibit-shortcuts-dialog.c */ + MetaInhibitShortcutsDialog *inhibit_shortcuts_dialog; I tend to prefer having extension specific fields in a ad-hoc struct (using g_object_set/get_qdata) on the surface (see pointer constraints). The reason is to avoid MetaWaylandSurface ending up being a hard to comprehent catch-all data dump.
(In reply to Jonas Ådahl from comment #23) > Review of attachment 353885 [details] [review] [review]: > [...] > Hmm. AFAIK this uses libnotify, and AFAIK plain mutter does not implement > the "receiver" end of the notification API, so I don't understand how this > can work. When trying to call "zenity --notification ..." in a plain mutter > session zenity just fails complaining about no service being available. Yes, it requires a notification daemon running, but that's not much of a problem imho because gnome-shell implements it and all other desktop do as well (and running a bare mutter instance alone is pretty unusable for a regular, it doesn't even implement an alt-tab dialog either)
(In reply to Olivier Fourdan from comment #28) > (In reply to Jonas Ådahl from comment #23) > > Review of attachment 353885 [details] [review] [review] [review]: > > [...] > > Hmm. AFAIK this uses libnotify, and AFAIK plain mutter does not implement > > the "receiver" end of the notification API, so I don't understand how this > > can work. When trying to call "zenity --notification ..." in a plain mutter > > session zenity just fails complaining about no service being available. > > Yes, it requires a notification daemon running, but that's not much of a > problem imho because gnome-shell implements it and all other desktop do as > well (and running a bare mutter instance alone is pretty unusable for a > regular, it doesn't even implement an alt-tab dialog either) In that case I suggest we make the mutter implementation g_warning ("No MetaInhibitShortcutDialog implementation, falling back on allowing"); g_signal_emit (..., ALLOW); and do it properly without zenity using St in gnome-shell.
(In reply to Jonas Ådahl from comment #29) > and do it properly without zenity using St in gnome-shell. Yeah, talking to ourselves over DBus is a bit weird.
We "talk to ourselves" only under gnome-shell, there might be other desktops using mutter which have a separate notification daemon. So I'd do both, keep zenity notification (because in vast majority of the cases we do have a notification mechanism in place with gnome-shell) and also warn that there is no implementation (it's a fallback anyway).
(In reply to Olivier Fourdan from comment #31) > We "talk to ourselves" only under gnome-shell, there might be other desktops > using mutter which have a separate notification daemon. > > So I'd do both, keep zenity notification (because in vast majority of the > cases we do have a notification mechanism in place with gnome-shell) and > also warn that there is no implementation (it's a fallback anyway). Well, we shouldn't "talk to ourself" (via D-Bus) in gnome-shell, and I still don't think it makes any sense to have the zenity method in plain mutter as it'll just result in errors being logged. If other libmutter users want to use libnotify/zenity they should implement the MetaPlugin API themselves.
But then we shall have no indication for the user that a shortcut inhibitor is in effect nor how to forcibly disable it, not even in gnome-shell, until someone comes up with an implementation of MetaInhibitShortcutDialog in gnome-shell...
(In reply to Olivier Fourdan from comment #33) > But then we shall have no indication for the user that a shortcut inhibitor > is in effect nor how to forcibly disable it, not even in gnome-shell, until > someone comes up with an implementation of MetaInhibitShortcutDialog in > gnome-shell... Right, which is why we must implement it in gnome-shell and land both the mutter and gnome-shell part at roughly the same time (or at least before the UI freeze).
(In reply to Olivier Fourdan from comment #33) > until someone comes up with an implementation of MetaInhibitShortcutDialog in > gnome-shell... I don't mind being that someone. But just for clarification - the intended UI for this is an actual dialog rather than a notification, right?
(In reply to Florian Müllner from comment #35) > (In reply to Olivier Fourdan from comment #33) > > until someone comes up with an implementation of MetaInhibitShortcutDialog in > > gnome-shell... > > I don't mind being that someone. But just for clarification - the intended > UI for this is an actual dialog rather than a notification, right? That is my personal preference, but could be good to go via the design team on that one.
Created attachment 355599 [details] [review] ui: Add InhibitShortcutsDialog Quick shell implementation for testing + design feedback.
Created attachment 355723 [details] [review] ui: Add InhibitShortcutsDialog Rebased to master.
(In reply to Florian Müllner from comment #37) > Quick shell implementation for testing + design feedback. Oh that's awesome, thanks so much Florian!
I tried attachment 355723 [details] [review] but I get this error instead of the dialog: (gnome-shell:32667): Gjs-WARNING **: JS ERROR: ReferenceError: InhibitShortcutsDialog is not defined WindowManager<._createInhibitShortcutsDialog@resource:///org/gnome/shell/ui/windowManager.js:1982:9 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22
Created attachment 355840 [details] [review] ui: Add InhibitShortcutsDialog Ah yes, it was missing an import. (I only tested the raw dialog by popping it up manually, not the boilerplate that hooks it up to the underlying functionality in mutter ...)
nice! :) It works!
Created attachment 355843 [details] [review] [PATCH 2/9] wayland: add inhibit shortcut mechanism (In reply to Jonas Ådahl from comment #20) > Review of attachment 353881 [details] [review] [review]: > > ::: src/core/keybindings.c > @@ +1733,3 @@ > + goto not_found; > + } > +#endif > > Can't we have this as a keyboard shortcut state instead instead of wayland > specific code here? For example it could be a list of ClutterInputDevices > (which in effect will just be the core keyboard device) and check the master > device of the device here? That way we don't need to have winsys concepts > leaked into here. > > The Wayland implementations would add/remove its inhibited devices (or a > table with device -> num inhibit refs) whenever the correct focus is set. I did it differently, as a couple of vfunc on the MetaWindowClass, oin x11 those are just stubs. That keeps the code clean without any winsys hacks in the core. > ::: src/core/window-private.h > @@ +774,3 @@ > +void meta_window_wayland_inhibit_shortcuts (MetaWindow *window); > + > +void meta_window_wayland_restore_shortcuts (MetaWindow *window); > > These two should go into src/wayland/meta-window-wayland.h. Yeah, those were leftovers from a pretious iteration actually...
Created attachment 355844 [details] [review] [PATCH 3/9] configure.ac: update wayland-protocols requirement New patch, now that we have wayland-protocols released. We could squash it with attachment 353880 [details] [review], as you prefer.
Created attachment 355845 [details] [review] [PATCH 4/9] wayland: add Xwayland grab keyboard support
Created attachment 355846 [details] [review] [PATCH 5/9] wayland: add keyboard shortcuts inhibitor protocol
Created attachment 355847 [details] [review] [PATCH 6/9] core: add MetaInhibitShortcutsDialog
Created attachment 355848 [details] [review] [PATCH 7/9] core: implement MetaInhibitShortcutsDialogDefault
Created attachment 355849 [details] [review] [PATCH 8/9] compositor: add vmethod to override inhibit shortcut
Created attachment 355850 [details] [review] [PATCH 9/9] wayland: use the inhibit shortcuts dialog
So it works, with one problem: Using a dialog (attachment 355840 [details] [review]) instead of a notification moves focus away from the window issuing the shortcut inhibit request, and virt-manager will issue a new grab *each* time focus changes, so it causes the dialog to show up continuously.
(In reply to Olivier Fourdan from comment #51) > Using a dialog (attachment 355840 [details] [review] [review]) instead of a > notification moves focus away from the window Note that notifications may grab focus as well (<super>n or mouse hover), so that's neither a fix nor a workaround for that issue. > virt-manager will issue a new grab *each* time focus changes, > so it causes the dialog to show up continuously. That's clearly broken. Can we change meta_wayland_surface_ensure_inhibit_shortcuts_dialog() to return TRUE if there's an existing dialog rather than replacing it with a new one?
(In reply to Florian Müllner from comment #52) > > virt-manager will issue a new grab *each* time focus changes, > > so it causes the dialog to show up continuously. > > That's clearly broken. Can we change > meta_wayland_surface_ensure_inhibit_shortcuts_dialog() to return TRUE if > there's an existing dialog rather than replacing it with a new one? I doubt it would solve the issue, because the focus transition occurs after the dialog is unmapped, i.e. after the user has closed the dialog.
A solution could be a "[X] Remember my choice" option in the dialog. Would that be "Remember forever" or "Just for the current session" is still debatable (as a user, I like to be able to change my mind, as I'm sometimes too fast in dismissing dialogs)
...or we could even avoid asking, like automatically remember the user choice as long as the app is running. Would that be acceptable?
Created attachment 355927 [details] [review] [PATCH 10/9] wayland: remember last choice for inhibit shortcut (In reply to Olivier Fourdan from comment #55) > ...or we could even avoid asking, like automatically remember the user > choice as long as the app is running. Would that be acceptable? This patch remembers the choice for each surface so we don't ask again for a given surface (until the surface is destroyed).
Created attachment 356096 [details] [review] [PATCH 9/9] wayland: use the inhibit shortcuts dialog Squash attachment 355850 [details] [review] and attachment 355927 [details] [review], I reckon we don't need two different wuaraks jsut to remember the last user choice, instead we just kee pthe data until the surface gets eventually destroyed, and save the last user choice in the existing data structure.
Created attachment 356589 [details] [review] [PATCH 4/9] wayland: add Xwayland grab keyboard support Use the shortcut-restored signal to actually delete the xwayland grab so that when the user forces the shortcuts to be restored (using the special key combo) we don't keep forcing focus on the grabbed window.
Review of attachment 355843 [details] [review]: Looks good, only nits. ::: src/core/keybindings.c @@ +56,3 @@ +#ifdef HAVE_WAYLAND +#include "wayland/meta-wayland.h" +#endif Don't need this one anymore. ::: src/wayland/meta-wayland-surface.c @@ +2259,3 @@ + +gboolean +meta_wayland_surface_shortcuts_inhibited (MetaWaylandSurface *surface, nit: naming: meta_wayland_surface_*is*_shortcuts_inhibited ::: src/wayland/meta-wayland-surface.h @@ +242,3 @@ } sub; + + /* hash of seats for which shortcuts are inhibited */ nit: s/hash/table/ @@ +243,3 @@ + + /* hash of seats for which shortcuts are inhibited */ + GHashTable *inhibit_shortcuts; naming nit/suggestion: shortcut_inhibited_seats ::: src/wayland/meta-wayland.c @@ +421,3 @@ + meta_wayland_surface_restore_shortcuts (keyboard->focus_surface, + compositor->seat); +} Missing newline. @@ +423,3 @@ +} +gboolean +meta_wayland_compositor_shortcuts_inhibited (MetaWaylandCompositor *compositor, naming nit: .._is_.. is the common way of true/false checkers. ::: src/wayland/meta-window-wayland.h @@ -79,3 @@ int *height); - - Left over change. ::: src/x11/window-x11.c @@ +1509,3 @@ + ClutterInputDevice *source) +{ + /* stub */ I guess can just put a comment that it is handled by the X server directly. @@ +1520,3 @@ +} + + One newline too many.
Review of attachment 355844 [details] [review]: lgtm.
Review of attachment 355846 [details] [review]: lgtm, just very minor issues ::: src/wayland/meta-wayland-inhibit-shortcuts.c @@ +78,3 @@ +} + + nit: extra newline @@ +149,3 @@ + shortcut_inhibit, + NULL); + wl_resource_set_user_data (keyboard_shortcuts_inhibit_resource, shortcut_inhibit); no need for this call, the data is already set by _set_implementation().
Review of attachment 355847 [details] [review]: lgtm.
Review of attachment 355848 [details] [review]: lgtm, just minor things ::: po/POTFILES.in @@ +18,3 @@ src/core/main.c src/core/meta-close-dialog-default.c +src/core/meta-inhibit-shortcuts-dialog-default.c I guess this is no longer needed? ::: src/core/meta-inhibit-shortcuts-dialog-default.c @@ +52,3 @@ + /* Default to allow shortcuts inhibitor, but complain that no dialog is implemented */ + g_warning ("No MetaInhibitShortcutDialog implementation, falling back on allowing"); + g_signal_emit_by_name (dialog, "response", META_INHIBIT_SHORTCUTS_DIALOG_RESPONSE_ALLOW); Could call the meta_inhibit_shortcuts_dialog_response() helper here as well.
Review of attachment 355849 [details] [review]: lgtm.
Review of attachment 356096 [details] [review]: ::: src/wayland/meta-wayland-inhibit-shortcuts-dialog.c @@ +16,3 @@ + */ + +#include <config.h> nit :P "config.h" @@ +26,3 @@ +static GQuark quark_surface_inhibit_shortcuts_data = 0; + +struct InhibitShortcutsData typedef struct _InhibitShortcutsData { ... } InhibitShortcutsData; @@ +116,3 @@ + g_signal_connect_data (dialog, "response", + G_CALLBACK (inhibit_shortcuts_dialog_response_cb), + data, surface_inhibit_shortcuts_data_closure_notify, function naming confused me, could we call it surface_inhibit_shortcuts_data_free or something? @@ +157,3 @@ + data = surface_inhibit_shortcuts_data_get (surface); + meta_inhibit_shortcuts_dialog_hide (data->dialog); + data->dialog = NULL; Can we g_signal_handlers_disconnect_by_data() here and rely on the closure destroy callback to be called? Then we check just data not data && data->dialog ::: src/wayland/meta-wayland-inhibit-shortcuts.c @@ +75,3 @@ MetaWaylandKeyboardShotscutsInhibit *shortcut_inhibit) { + meta_wayland_surface_free_inhibit_shortcuts_dialog (shortcut_inhibit->surface); Can we move this to meta-wayland-inhibit-shortcuts-dialog.c instead? To let isolate the ownership to itself only. @@ +143,3 @@ shortcut_inhibit); + meta_wayland_surface_show_inhibit_shortcuts_dialog (surface, seat); We also need to do something like meta_wayland_surface_cancel_inhibit_shortcuts_dialog (..) for when the inhibit request is dismissed.
Created attachment 356645 [details] [review] [PATCH 2/9] wayland: add inhibit shortcut mechanism New version following review in comment 59
Created attachment 356647 [details] [review] [PATCH 9/9] wayland: use the inhibit shortcuts dialog Update after review in comment 65
Created attachment 356654 [details] [review] [PATCH 9/9] wayland: use the inhibit shortcuts dialog Oops, forgot the "typedef struct" from comment 65.
Review of attachment 356645 [details] [review]: lgtm
Review of attachment 356654 [details] [review]: lgtm
For reference, regarding the need for supporting Xwayland grab protocol with override redirect windows (which cannot be focused in mutter): - https://bugs.freedesktop.org/show_bug.cgi?id=96547 That prevents vmware installer from asking for root password, it's using custom version of gksu that maps an override redirect window and asks for the root password and won't let the user continue until this is provided. - bug 752956 Somehow more of a corner case, but if somehow someone manages to have xscreensaver running on Xwayland, the user cannot enter the password because it's using an override redirect windows. But a workaround was added in xscreensaver to not use the lock if WAYLAND_DISPLAY is set, so it's less of an issue nowadays.
Ok, I've rebased the series and I am pretty much ready to land those patches (minus the Xwayland grab support patch) in mutter, what about the gnome-shell one, Florian, you're fine with me landing your patch in gnome-shell once the ones in mutter have landed?
(In reply to Olivier Fourdan from comment #72) > Florian, you're fine with me landing your patch in > gnome-shell once the ones in mutter have landed? The patch has some CSS changes, which means it is a bit tedious to land (the style is actually generated from SASS sources in a submodule, so landing the patch involves two commits and a submodule update). I can attach the submodule patch as well, but it's probably easier for you to ping me and let me push the gnome-shell bits ...
Comment on attachment 353880 [details] [review] [PATCH 1/9] configure.ac: update wayland-server requirement attachment 353880 [details] [review] pushed to git master as commit c54377e - configure.ac: update wayland-server requirement
Comment on attachment 355844 [details] [review] [PATCH 3/9] configure.ac: update wayland-protocols requirement That one was dropped as the wayland-protocols got raised already by commit b7b5fb2 - wayland: Add zwp_linux_dmabuf_v1 support
Comment on attachment 356645 [details] [review] [PATCH 2/9] wayland: add inhibit shortcut mechanism attachment 356645 [details] [review] pushed to git master as commit dd12f56 - wayland: add inhibit shortcut mechanism
Comment on attachment 355846 [details] [review] [PATCH 5/9] wayland: add keyboard shortcuts inhibitor protocol attachment 355846 [details] [review] pushed to git master as commit 2ca0871 - wayland: add keyboard shortcuts inhibitor protocol
Comment on attachment 356589 [details] [review] [PATCH 4/9] wayland: add Xwayland grab keyboard support Need to add some "whitelist" mechanism of apps we actually care about grabs and create a MetaWindowXwayland similar to MetaWindowX11 to use inhibit shortcut vfunc that check about the surface like Wayland native ones.
Comment on attachment 355847 [details] [review] [PATCH 6/9] core: add MetaInhibitShortcutsDialog attachment 355847 [details] [review] pushed to git master as b894fbd - core: add MetaInhibitShortcutsDialog
Comment on attachment 355848 [details] [review] [PATCH 7/9] core: implement MetaInhibitShortcutsDialogDefault attachment 355848 [details] [review] pushed to git master as commit e3f76e9 - core: implement MetaInhibitShortcutsDialogDefault
Comment on attachment 355849 [details] [review] [PATCH 8/9] compositor: add vmethod to override inhibit shortcut attachment 355849 [details] [review] pushed to git as master as commit ce20c96 - compositor: add vmethod to override inhibit shortcut dialog
Comment on attachment 356654 [details] [review] [PATCH 9/9] wayland: use the inhibit shortcuts dialog attachment 356654 [details] [review] pushed ti git master as commit 46cb506 - wayland: use the inhibit shortcuts dialog
Comment on attachment 355840 [details] [review] ui: Add InhibitShortcutsDialog Attachment 355840 [details] pushed as dff3e4e - ui: Add InhibitShortcutsDialog
Created attachment 358926 [details] [review] [PATCH 1/3] xwayland: Add MetaWindowXWayland MetaWindowXWayland derives from MetaWindowX11 to allow for some XWayland specific vfunc that wouldn't apply to plain X11 windows, such as shortcut inhibit routines.
Created attachment 358927 [details] [review] [PATCH 2/3] settings: Add "xwayland-grab-granted" setting Add a new setting to control which X11 windows are allowed to issue Xwayland grabs. X11 windows whose either resource name or resource class match one the values listed in this key will be granted a grab by the Wayland compositor when requested, wheraeas grabs requested by those not matching will have no effect.
Created attachment 358928 [details] [review] [PATCH 3/3] wayland: Add Xwayland grab keyboard support This protocol is limited to Xwayland only and is not visible/usable by any other client. mutter use the setting key "xwayland-grab-granted" to control which X11 windows are granted the right to issue grabs on their toplevel windows.
Review of attachment 358926 [details] [review]: ::: src/wayland/meta-window-xwayland.c @@ +39,3 @@ +meta_window_xwayland_init (MetaWindowXWayland *window_xwayland) +{ + /* Nothing in particular to do here */ An empty function definition would also say this :P ::: src/wayland/meta-window-xwayland.h @@ +20,3 @@ +#define META_WINDOW_XWAYLAND_H + +#include <meta/window.h> It's not a system include, so just do "meta/window.h" @@ +34,3 @@ +GType meta_window_xwayland_get_type (void); + +typedef struct _MetaWindowXWayland MetaWindowXWayland; Given how naming stuff works, it should be MetaWindowXwayland and not MetaWindowXWayland, as each capitalization means a new word. @@ +35,3 @@ + +typedef struct _MetaWindowXWayland MetaWindowXWayland; +typedef struct _MetaWindowXWaylandClass MetaWindowXWaylandClass; Everything inside G_BEGIN/END_DECLS can be shortened to #define META_TYPE_WINDOW_XWAYLAND G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, META, WINDOW_XWAYLAND, GObject)
Review of attachment 358927 [details] [review]: ::: data/org.gnome.mutter.gschema.xml.in @@ +128,3 @@ </key> + <key name="xwayland-grab-granted" type="as"> Maybe "xwayland-grab-whitelist"? I first read "granted" as the past tense verb. Also, we have a .wayland. namespace, maybe we should put it there? @@ +148,3 @@ + This is by no mean a security feature, any application is able to + add itself to this list, the purpose of this key is to selectively + allow keyboard grabs in Xwayland to a limited set of X11 applications. Don't give application developers any ideas by adding the part about them being able to add themself :P @@ +151,3 @@ + + This key has no effect on Wayland native applications nor on X11 + when running natively (without a Wayland compositor). A thing to consider adding is that its possible to "abort" the grab from mutters / wayland clients point of view. @@ +153,3 @@ + when running natively (without a Wayland compositor). + </description> + </key> A problem with using a gsetting is that we can't install updates to this when the user has configured their own whitelist. What would be best is a system config and a local config some how, but I don't have any great ideas and we don't have any precedence where we do something like this. ::: src/backends/meta-settings.c @@ +313,3 @@ + g_ptr_array_free (settings->xwayland_grab_granted_patterns, TRUE); + settings->xwayland_grab_granted_patterns = + g_ptr_array_new_with_free_func ((GDestroyNotify) g_pattern_spec_free); Incorrect indentation. @@ +319,3 @@ + while (g_variant_iter_loop (&granted_iter, "s", &granted)) + g_ptr_array_add (settings->xwayland_grab_granted_patterns, + (gpointer) g_pattern_spec_new (granted)); Is this cast really needed? @@ +374,3 @@ g_clear_object (&settings->mutter_settings); g_clear_object (&settings->interface_settings); + g_ptr_array_free (settings->xwayland_grab_granted_patterns, TRUE); You'll need to set the variable to NULL to, as this is dispose(), not finalize(). I think you can for example change it to g_clear_pointer (&settinsg->xwayland_grab_granted_patterns, g_ptr_array_unref); which will call _free with TRUE. ::: src/meta/meta-settings.h @@ +29,3 @@ int meta_settings_get_font_dpi (MetaSettings *settings); +GPtrArray* meta_settings_get_xwayland_grab_granted_patterns (MetaSettings *settings); Coding style: space between 'GPtrArray' and '*'. Also, this should be added to meta-settings-private.h, as I assume this wont be fetched by gnome-shell?
Review of attachment 358928 [details] [review]: ::: src/Makefile.am @@ +84,3 @@ keyboard-shortcuts-inhibit-unstable-v1-server-protocol.h \ + xwayland-keyboard-grab-unstable-v1-protocol.c \ + xwayland-keyboard-grab-unstable-v1-server-protocol.h \ Add these to .gitignore. ::: src/wayland/meta-wayland.c @@ +320,3 @@ } +static bool gboolean @@ +334,3 @@ + + /* All others are visible to all clients */ + return true; TRUE @@ +386,3 @@ + wl_display_set_global_filter (compositor->wayland_display, + meta_xwayland_global_filter, + (void *) compositor); is (void *) really needed here? ::: src/wayland/meta-xwayland-grab-keyboard.c @@ +23,3 @@ +#include "config.h" + +#include <wayland-server.h> Empty newline between system includes and local includes. @@ +30,3 @@ +#include "meta-wayland-seat.h" +#include "meta-window-wayland.h" +#include "meta-xwayland-grab-keyboard.h" Use full paths to include files. @@ +38,3 @@ + gulong surface_destroyed_handler; + gulong shortcuts_restored_handler; + struct wl_resource *resource; We don't usually align struct fields this way. It tends to cause problems when introducing some long variable type name. @@ +58,3 @@ + +static void +zwp_xwayland_keyboard_grab_manager_destructor (struct wl_resource *resource) I guess you mean zwp_xwayland_keyboard_grab_destructor here? @@ +63,3 @@ + + active_grab = wl_resource_get_user_data (resource); + meta_xwayland_keyboard_grab_end (active_grab); This will cause warnings if the user ungrabbed, as you don't unset the surface (thus don't early out) in .._grab_end(). @@ +75,3 @@ +} + +static const struct zwp_xwayland_keyboard_grab_manager_v1_interface This should be zwp_xwayland_keyboard_grab_v1_interface @@ +76,3 @@ + +static const struct zwp_xwayland_keyboard_grab_manager_v1_interface + meta_grab_keyboard_interface = { Just call it 'xwayland_keyboard_grab_interface' or something. I'm not fond of prefixing variables with meta_. @@ +105,3 @@ +{ + MetaBackend *meta_backend; + MetaSettings *meta_settings; Don't prefix variable names of our own types with 'meta_'. It should be just "backend" and "settings". @@ +107,3 @@ + MetaSettings *meta_settings; + GPtrArray *pattern_array; + GPatternSpec *pattern; 'pattern' can be local to the for loop block. ::: src/wayland/meta-xwayland-grab-keyboard.h @@ +24,3 @@ +#define META_XWAYLAND_GRAB_KEYBOARD_H + +#include <wayland-server.h> Empty line between system includes and local includes. @@ +38,3 @@ + +#endif /* META_XWAYLAND_GRAB_KEYBOARD_H */ + Stray newline
(In reply to Jonas Ådahl from comment #89) > Review of attachment 358928 [details] [review] [review]: > ::: src/wayland/meta-wayland.c > @@ +320,3 @@ > } > > +static bool > > gboolean > > @@ +334,3 @@ > + > + /* All others are visible to all clients */ > + return true; > > TRUE > > @@ +386,3 @@ > + wl_display_set_global_filter (compositor->wayland_display, > + meta_xwayland_global_filter, > + (void *) compositor); > > is (void *) really needed here? Changing bool/true to gboolean/TRUE means I'll have to cast meta_xwayland_global_filter() as (wl_display_global_filter_func_t) though, otherwise gcc will raise a warning.
(In reply to Olivier Fourdan from comment #90) > (In reply to Jonas Ådahl from comment #89) > > Review of attachment 358928 [details] [review] [review] [review]: > > ::: src/wayland/meta-wayland.c > > @@ +320,3 @@ > > } > > > > +static bool > > > > gboolean > > > > @@ +334,3 @@ > > + > > + /* All others are visible to all clients */ > > + return true; > > > > TRUE > > > > @@ +386,3 @@ > > + wl_display_set_global_filter (compositor->wayland_display, > > + meta_xwayland_global_filter, > > + (void *) compositor); > > > > is (void *) really needed here? > > Changing bool/true to gboolean/TRUE means I'll have to cast > meta_xwayland_global_filter() as (wl_display_global_filter_func_t) though, > otherwise gcc will raise a warning. Good point. Didn't think about that this wasn't plain internal API but libwayland API, so makes sense to leave it as bool.
(In reply to Jonas Ådahl from comment #88) > Review of attachment 358927 [details] [review] [review]: > [...] > > Everything inside G_BEGIN/END_DECLS can be shortened to > > #define META_TYPE_WINDOW_XWAYLAND > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > META, WINDOW_XWAYLAND, GObject) Odd, if I use G_DECLARE_FINAL_TYPE() in place of the old style definition, it builds OK but crashes at run time, 100% reproducible. Reverting to the old style works fine...
I think this is because: GLib-GObject-CRITICAL **: g_object_new_with_properties: assertion 'G_TYPE_IS_OBJECT (object_type)' failed and after creation as a META_TYPE_WINDOW_XWAYLAND in _meta_window_shared_new(), the resulting window is null
(In reply to Jonas Ådahl from comment #88) > Review of attachment 358927 [details] [review] [review]: > A problem with using a gsetting is that we can't install updates to this > when the user has configured their own whitelist. What would be best is a > system config and a local config some how, but I don't have any great ideas > and we don't have any precedence where we do something like this. We could have two settings, "xwayland-grab-user-whitelist" and "xwayland-grab-system-whitelist", the former being empty by default and the latter being populated as needs arise (ie as we install updates). Users and/or Sysadmins would add new apps to the "xwayland-grab-user-whitelist" for specific needs (like home made apps needing grabs) without touching the "xwayland-grab-system-whitelist" unless they want to revoke grabs for some of the defaults apps. How does that sound? overkill?..
Created attachment 360171 [details] [review] [PATCH v2 1/3] xwayland: Add MetaWindowXwayland
Created attachment 360172 [details] [review] [PATCH v2 2/3] settings: Add "xwayland-grab-*-whitelist" settings
Created attachment 360173 [details] [review] [PATCH v2 3/3] wayland: Add Xwayland grab keyboard support
(In reply to Olivier Fourdan from comment #94) > We could have two settings, "xwayland-grab-user-whitelist" and > "xwayland-grab-system-whitelist", the former being empty by default and the > latter being populated as needs arise (ie as we install updates). > > Users and/or Sysadmins would add new apps to the > "xwayland-grab-user-whitelist" for specific needs (like home made apps > needing grabs) without touching the "xwayland-grab-system-whitelist" unless > they want to revoke grabs for some of the defaults apps. What's the point of having the system whitelist be a gsetting? We could just hardcode it if it's only going to be update by us, and make the whitelist gsetting able to also remove things from the hardcoded default by prefixing the string with something unlikely to be used in WM_CLASS like '!' .
(In reply to Rui Matos from comment #98) > What's the point of having the system whitelist be a gsetting? We could just Simplicity. > hardcode it if it's only going to be update by us, and make the whitelist > gsetting able to also remove things from the hardcoded default by prefixing > the string with something unlikely to be used in WM_CLASS like '!' . For now, the whitelist uses GPatternSpec for wildcards and jokers matching, I wouldn't want to go too much into a complex parser or regular expression matching.
Also, using a gsetting for system whitelist allows users and sysadmins to check what is allowed without digging into the code (if it were to be hardcoded)
Unfortunately since gsettings lacks a true multi tiered lookup mechanism, I'm worried that users are able to change the "system" gsetting we won't be able to update it anyway. (In reply to Olivier Fourdan from comment #100) > Also, using a gsetting for system whitelist allows users and sysadmins to > check what is allowed without digging into the code (if it were to be > hardcoded) We could show the default in the gsetting key description, ideally with a bit of build system glue to have the definition in a single place and get it replaced in the schema and the code.
OK, I see how we could do it...
Created attachment 360260 [details] [review] [PATCH v3 2/3] settings: Add "xwayland-grab-whitelist" setting Add whitelist/blacklist (entries prefixed with "!") along with a configure option to set the default (system) whitelist.
Created attachment 360261 [details] [review] [PATCH v3 3/3] wayland: Add Xwayland grab keyboard support Use whitelist/blacklist
Anyone to redo a review of these new iteration? I hope it does everything that was requested previously.
As an application developer, what are the criteria to be added to that whitelist? :) We are based on TigerVNC, but have changed WM_CLASS to "thinlinc-client" to separate things.
apps developpers can add the the wm_class to the gsettings
So the recommendation is to add ourselves automatically as part of the application startup? I got the impression that wasn't that welcome from comment #88. :)
Hehe, err, well, my comment 107 wasn't a recommendation per se, merely stating what would possible with the proposed solution :)
(In reply to Olivier Fourdan from comment #109) > Hehe, err, well, my comment 107 wasn't a recommendation per se, merely > stating what would possible with the proposed solution :) Err, applications poking at gstreamer seems like a pretty nasty thing to do. Maybe we should add a custom window property instead? Something like, "Hi, I'm actually not a popup menu, I actually want the grab, givf it to me!".
(In reply to Jonas Ådahl from comment #110) > Err, applications poking at gstreamer seems like a pretty nasty thing to do. You mean gsettings, right? > Maybe we should add a custom window property instead? Something like, "Hi, > I'm actually not a popup menu, I actually want the grab, givf it to me!". That would brought us back to the initial discussion when decided on a white list/black list, no?
(In reply to Olivier Fourdan from comment #111) > (In reply to Jonas Ådahl from comment #110) > > Err, applications poking at gstreamer seems like a pretty nasty thing to do. > > You mean gsettings, right? Heh, correct, interesting typo. > > > Maybe we should add a custom window property instead? Something like, "Hi, > > I'm actually not a popup menu, I actually want the grab, givf it to me!". > > That would brought us back to the initial discussion when decided on a white > list/black list, no? True. Comment 107 somewhat stated that applications should/could poke at the gsetting though, which I think we must strongly advice against. If we want to have any way to allow applications to "whitelist" themselves, that must be done some other way (like a window property).
> (In reply to Jonas Ådahl from comment #110) > > That would brought us back to the initial discussion when decided on a white > list/black list, no? Is that discussion elsewhere, because I don't see much of a rationale on this bug? Are Wayland applications also subjected to such a whitelist if they want to disable window manager hotkeys? If not, why are X11 applications? We'd like to be good citizens and follow recommended behaviour, but we need to have a method that all users can understand and use.(In reply to Olivier Fourdan from comment #111)
(In reply to Jonas Ådahl from comment #112) > True. Comment 107 somewhat stated that applications should/could poke at the > gsetting though, which I think we must strongly advice against. If we want > to have any way to allow applications to "whitelist" themselves, that must > be done some other way (like a window property). The problem I see with this approach is that's leaking Wayland design decisions/limitations onto X11 apps, which may work for newer apps but not for older or proprietary ones. The main use of Xwayland, IMHO, is for legacy applications that cannot be ported to Wayland.
(In reply to Pierre Ossman from comment #113) > Is that discussion elsewhere, because I don't see much of a rationale on > this bug? That was discussed mostly at GUADEC. My initial implementation was granting grabs to all X11 apps, which would stop alt-tab from working whenever a menu was opened in legacy X11 apps such as gitk, which was perceived as a regression. To limit the use of this grab "emulation", it was decided it would be better to have a white list of applications allowed to issue such a grab, and then eventually turned out as a black list/white list. Another alternative is status quo, ie. no grab for Xwayland apps. Anyhow, any Wayland compositor is free to implement this the way it wants (or not at all) so there is no one solution fit all. > Are Wayland applications also subjected to such a whitelist if > they want to disable window manager hotkeys? If not, why are X11 > applications? My guess is because these are legacy applications with a long history of abusing grabs in all sort of ways. > We'd like to be good citizens and follow recommended behaviour, but we need > to have a method that all users can understand and use. Ideally, application should be ported to Wayland native.
(In reply to Pierre Ossman from comment #113) > > (In reply to Jonas Ådahl from comment #110) > > > > That would brought us back to the initial discussion when decided on a white > > list/black list, no? > > Is that discussion elsewhere, because I don't see much of a rationale on > this bug? Are Wayland applications also subjected to such a whitelist if > they want to disable window manager hotkeys? If not, why are X11 > applications? The problem is that mutter can't tell the difference from X11 applications that simply opens a popup menu and applications such as remote desktop/virtual machine viewers. Thinking some more about this, maybe we should allow good citizen X11 clients that want to "just work" to whitelist certain windows, which would make mutter trigger the Wayland client path (i.e. show a dialog). What I mean is: X11 client adds property "good grab citizen" on its main window. X11 client does grab on said window. Mutter shows inhibit shortcuts dialog thing. We'd then have the whitelist thing, that requires manual user intervention. Speaking of which, should we consider disabling the 'whitelist' by default, hiding it behind a 'allow legacy x11 grabs' gsetting? That way we make it possible for "well meaning" X11 clients to do the right thing (described above), while "punishing" bad ones.
(In reply to Olivier Fourdan from comment #115) > > That was discussed mostly at GUADEC. My initial implementation was granting > grabs to all X11 apps, which would stop alt-tab from working whenever a menu > was opened in legacy X11 apps such as gitk, which was perceived as a > regression. > Ah. That would indeed be a problem. I take it not all of these are override redirect? Any other heuristics that could be applied? E.g. only blacklist undecorated windows. > > Ideally, application should be ported to Wayland native. Agreed, but at least in our case that is non-trival so we'll be stuck with just X11 for the foreseeable future. :/
(In reply to Pierre Ossman from comment #117) > Ah. That would indeed be a problem. I take it not all of these are override > redirect? > > Any other heuristics that could be applied? E.g. only blacklist undecorated > windows. I think any kind of heuristic for this is deemed to fail given the variety of X11 applications, e.g. it would fail with fdo bug 96547 [1] which is where it all started (that's using an O-R window, thus undecorated) [1] https://bugs.freedesktop.org/show_bug.cgi?id=96547
(In reply to Jonas Ådahl from comment #116) > Thinking some more about this, maybe we should allow good citizen X11 > clients that want to "just work" to whitelist certain windows, which would > make mutter trigger the Wayland client path (i.e. show a dialog). > > What I mean is: > > X11 client adds property "good grab citizen" on its main window. Not the main window, the window on which the grab is to be issued. > X11 client does grab on said window. > Mutter shows inhibit shortcuts dialog thing. > > We'd then have the whitelist thing, that requires manual user intervention. > > Speaking of which, should we consider disabling the 'whitelist' by default, > hiding it behind a 'allow legacy x11 grabs' gsetting? That way we make it > possible for "well meaning" X11 clients to do the right thing (described > above), while "punishing" bad ones. That could work, but what about the default white list and the ability to blacklist applications from it (comment 98)?
(In reply to Olivier Fourdan from comment #118) > > I think any kind of heuristic for this is deemed to fail given the variety > of X11 applications, e.g. it would fail with fdo bug 96547 [1] which is > where it all started (that's using an O-R window, thus undecorated) > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=96547 That bug seems to suggest that O-R windows can't be supported anyway?
(In reply to Pierre Ossman from comment #120) > That bug seems to suggest that O-R windows can't be supported anyway? The whole point of this bug here (and the related Wayland protocols additions) is precisely to make such case work with Xwayland/Wayland.
Alright. I figured that you might be settling for just windowed and _NET_WM_FULLSCREEN scenarios. But O-R covering the entire screen might be a sufficient addition to the heuristic in that case. It would exclude menus.
Created attachment 362070 [details] [review] [PATCH 1/4] xwayland: Add MetaWindowXwayland MetaWindowXwayland derives from MetaWindowX11 to allow for some Xwayland specific vfunc that wouldn't apply to plain X11 windows, such as shortcut inhibit routines.
Created attachment 362072 [details] [review] [PATCH 2/4] xwayland: add _XWAYLAND_MAY_GRAB_KEYBOARD property Add a new client message "_XWAYLAND_MAY_GRAB_KEYBOARD" that X11 clients can use to tell mutter this is a well behaving X11 client so it may grant the keyboard grabs when requested. An X11 client wishing to be granted Xwayland grabs by gnome-shell/mutter must send a ClientMessage to the root window with: - message_type set to "_XWAYLAND_MAY_GRAB_KEYBOARD" - window set to the xid of the window on which the grab is to be issued - data.l[0] to a non-zero value Note: Sending this client message when running a plain native X11 environment would have no effect.
Created attachment 362073 [details] [review] [PATCH 3/4] settings: Add xwayland grab settings Add new settings to control which X11 windows are allowed to issue Xwayland grabs.
Created attachment 362074 [details] [review] [PATCH 4/4] wayland: Add Xwayland grab keyboard support This protocol is limited to Xwayland only and is not visible/usable by any other client. Mutter uses the following mechanisms to determine if an X11 client should be granted a grab: - is "xwayland-allow-grabs" set? - if set, is the client blacklisted? - otherwise, has the client set the X11 window property _XWAYLAND_MAY_GRAB_KEYBOARD on the window using a client message? - if not, is it a client white-listed either via the default system list or the settings "xwayland-grab-whitelist"?
This new series does: - Add "xwayland-allow-grabs" settings (boolean) to enable or disable Xwayland grabs altogether (comment 116) - Keep the whitelist/blacklist, including the default built-time white list (comment 98) - Restore the meta-keyboard grab so grabs on O-R work What this series does _not_ do: - Use the inhibit-shortcut dialog (comment 116) because: * Grabs in X11 are a lot more common that shortcut inhibition would be in Wayland * We already add several mechanism to let the user control which app can issue grabs (whitelist/blacklist) * Grabs are not enabled by default, so if enabled by the user that is necessarily on purpose. * When dealing with O-R windows, those show up on top of the inhibit shortcut dialog, meh... * Xwayland grabs are different from (i.e. more than) Wayland shortcut inhibition in the sense that it also grabs the focus (just like real X11 grabs).
Review of attachment 362070 [details] [review]: LGTM, just comment about whether we can use the declare macro. ::: src/wayland/meta-window-xwayland.h @@ +35,3 @@ + +typedef struct _MetaWindowXwayland MetaWindowXwayland; +typedef struct _MetaWindowXwaylandClass MetaWindowXwaylandClass; Can just replace all this with G_DECLARE_FINAL_TYPE (..) right?
Review of attachment 362072 [details] [review]: lgtm.
Review of attachment 362073 [details] [review]: I think this seems like the best solution so far. ::: configure.ac @@ +106,3 @@ xcb-res " +XWAYLAND_GRAB_DEFAULT_WHITELIST="gnome-boxes,remote-viewer,virt-viewer,virt-manager,vinagre,vncviewer,Xephyr" Isn't gnome-boxes using the Wayland backend? I guess the question is, should we limit these to applications that doesn't have Wayland backends? ::: data/org.gnome.mutter.wayland.gschema.xml.in @@ +65,3 @@ + <description> + Allow keyboard grabs issued by X11 applications running in Xwayland + to be taken into account. Should we blurb about the conditions for it to happen? I.e. either the "I'm well behaved" and the whitelist. ::: src/backends/meta-settings-private.h @@ +63,3 @@ + GPtrArray **blacklist_patterns); + +gboolean meta_settings_get_xwayland_allow_grabs (MetaSettings *settings); grammar nit: meta_settings_are_xwayland_grabs_allowed() reads better gramatically. (or _is_?) style nit: just one space between MetaSettings and *settings. ::: src/backends/meta-settings.c @@ +42,3 @@ FONT_DPI_CHANGED, EXPERIMENTAL_FEATURES_CHANGED, + XWAYLAND_GRAB_WHITELIST_CHANGED, Since this is not a white list, name suggestion: 'XWAYLAND_GRAB_ACCESS_RULES_CHANGED', with the API meta_settings_get_xwayland_grab_access_rules() and s/whitelist/access_rules/ where whitelist now refers to both lists.
Review of attachment 362074 [details] [review]: ::: src/wayland/meta-xwayland-grab-keyboard.c @@ +91,3 @@ + + g_signal_handler_disconnect (active_grab->surface, + active_grab->shortcuts_restored_handler); Shouldn't we also disconnect the window-associated handler? As in, couldn't this happen: 1. Create wl_surface 2. Grab 3. Ungrab 4. Window associated @@ +96,3 @@ + active_grab->seat); + + if (active_grab->has_keyboard_grab) All our grab APIs are terrible, but I think the most "correct" way to check whether we actually still have the grab is to check whether the keyboard->grab->key == meta_xwayland_keyboard_grab_key. (see https://bugzilla.gnome.org/show_bug.cgi?id=785218 for a brighter future) @@ +263,3 @@ + active_grab); + else + meta_warning ("Cannot grant Xwayland grab to surface %p", surface); Use g_warning() and you get fancy colors. (FWIW, I think meta_warning can be deprecated)
(In reply to Jonas Ådahl from comment #128) > Review of attachment 362070 [details] [review] [review]: > > LGTM, just comment about whether we can use the declare macro. > > ::: src/wayland/meta-window-xwayland.h > @@ +35,3 @@ > + > +typedef struct _MetaWindowXwayland MetaWindowXwayland; > +typedef struct _MetaWindowXwaylandClass MetaWindowXwaylandClass; > > Can just replace all this with > > G_DECLARE_FINAL_TYPE (..) right? Err, nope, I tried hard, promise, to understand why it would build okay but fail hard art runtime when using G_DECLARE_FINAL_TYPE (comment 92), but the truth is it works with the old style definition and not with G_DECLARE_FINAL_TYPE and I have no idea why... So I'd rather stick to what works for now.
(In reply to Jonas Ådahl from comment #130) > +XWAYLAND_GRAB_DEFAULT_WHITELIST="gnome-boxes,remote-viewer,virt-viewer,virt- > manager,vinagre,vncviewer,Xephyr" > > Isn't gnome-boxes using the Wayland backend? Yes, it is, just like virt-viewer, virt-manager, etc. but... > I guess the question is, should we limit these to applications that doesn't > have Wayland backends? ... when used over ssh tunnelling, gtk+ will fall back to plain good old X11 when Wayland isn;t available, and it seems plenty of sysadmins just do that (don ask me for the exact figures, I have none), ssh to some other VM host and run the VM viewer of their choice (whatever it is) via ssh tunnelling, so for those it makes sense (imho) to whitelist the well known ones.
(In reply to Olivier Fourdan from comment #132) > (In reply to Jonas Ådahl from comment #128) > > Review of attachment 362070 [details] [review] [review] [review]: > > > > LGTM, just comment about whether we can use the declare macro. > > > > ::: src/wayland/meta-window-xwayland.h > > @@ +35,3 @@ > > + > > +typedef struct _MetaWindowXwayland MetaWindowXwayland; > > +typedef struct _MetaWindowXwaylandClass MetaWindowXwaylandClass; > > > > Can just replace all this with > > > > G_DECLARE_FINAL_TYPE (..) right? > > Err, nope, I tried hard, promise, to understand why it would build okay but > fail hard art runtime when using G_DECLARE_FINAL_TYPE (comment 92), but the > truth is it works with the old style definition and not with > G_DECLARE_FINAL_TYPE and I have no idea why... So I'd rather stick to what > works for now. From comment 92: > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > META, WINDOW_XWAYLAND, GObject) I think this should have been G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, META, WINDOW_XWAYLAND, MetaWindowX11)
(In reply to Olivier Fourdan from comment #133) > (In reply to Jonas Ådahl from comment #130) > > +XWAYLAND_GRAB_DEFAULT_WHITELIST="gnome-boxes,remote-viewer,virt-viewer,virt- > > manager,vinagre,vncviewer,Xephyr" > > > > Isn't gnome-boxes using the Wayland backend? > > Yes, it is, just like virt-viewer, virt-manager, etc. but... > > > I guess the question is, should we limit these to applications that doesn't > > have Wayland backends? > > ... when used over ssh tunnelling, gtk+ will fall back to plain good old X11 > when Wayland isn;t available, and it seems plenty of sysadmins just do that > (don ask me for the exact figures, I have none), ssh to some other VM host > and run the VM viewer of their choice (whatever it is) via ssh tunnelling, > so for those it makes sense (imho) to whitelist the well known ones. Fair enough. Then comes the next question, should we try to get them to add the flag or no? Shouldn't that make it possible to limit the grabbing to the correct window? (and try to use the whitelist for the cases where we have yet to fix upstream)
(In reply to Jonas Ådahl from comment #135) > Fair enough. Then comes the next question, should we try to get them to add > the flag or no? Shouldn't that make it possible to limit the grabbing to the > correct window? (and try to use the whitelist for the cases where we have > yet to fix upstream) Well, if you ask me ;) I'd say it's not worth it. We would have to add a new API to gdk/gtk+ (as sending an ClientMessage is pretty much X11 centric, and we do not want to do this for all grabs as we would end up with all gtk+ menus succeeding in issuing grabs...) for a pretty specific use case (basically, a corner case, using a Wayland capable client with X11 instead of Wayland and issuing grabs that are really needed). All this Xwayland grab mechanisms is really to be able to have those specific use case working without too much hassle, I wouldn't invest too much in this imho (I mean, from a gtk+ application point of view).
(In reply to Jonas Ådahl from comment #134) > From comment 92: > > > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > > META, WINDOW_XWAYLAND, GObject) > > I think this should have been > > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > META, WINDOW_XWAYLAND, MetaWindowX11) Yes, I thought of that as well at the time but then that doesn't build: glib-2.0/glib/gmacros.h:436:43: warning: implicit declaration of function ‘glib_autoptr_cleanup_MetaWindowX11’; did you mean ‘glib_autoptr_cleanup_GtkWindow’? [-Wimplicit-function-declaration] #define _GLIB_AUTOPTR_FUNC_NAME(TypeName) glib_autoptr_cleanup_##TypeName ... include/glib-2.0/gobject/gtype.h:1399:38: error: field ‘parent_class’ has incomplete type typedef struct { ParentName##Class parent_class; } ModuleObjName##Class; \ ^ /home/ofourdan/src/gnome/mutter/src/wayland/meta-window-xwayland.h:42:1: note: in expansion of macro ‘G_DECLARE_FINAL_TYPE’ G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, ^ Maybe because MetaWindowX11 doesn't support g_autoptr() which is required by those macros: https://developer.gnome.org/gobject/stable/gobject-Type-Information.html#G-DECLARE-FINAL-TYPE:CAPS > "You can only use this function if your parent type also supports g_autoptr()."
(In reply to Olivier Fourdan from comment #137) > (In reply to Jonas Ådahl from comment #134) > > From comment 92: > > > > > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > > > META, WINDOW_XWAYLAND, GObject) > > > > I think this should have been > > > > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > > META, WINDOW_XWAYLAND, MetaWindowX11) > > Yes, I thought of that as well at the time but then that doesn't build: > > glib-2.0/glib/gmacros.h:436:43: warning: implicit declaration of function > ‘glib_autoptr_cleanup_MetaWindowX11’; did you mean > ‘glib_autoptr_cleanup_GtkWindow’? [-Wimplicit-function-declaration] > #define _GLIB_AUTOPTR_FUNC_NAME(TypeName) glib_autoptr_cleanup_##TypeName > ... > include/glib-2.0/gobject/gtype.h:1399:38: error: field ‘parent_class’ has > incomplete type > typedef struct { ParentName##Class parent_class; } ModuleObjName##Class; > \ > ^ > /home/ofourdan/src/gnome/mutter/src/wayland/meta-window-xwayland.h:42:1: > note: in expansion of macro ‘G_DECLARE_FINAL_TYPE’ > G_DECLARE_FINAL_TYPE (MetaWindowXwayland, meta_window_xwayland, > ^ > > Maybe because MetaWindowX11 doesn't support g_autoptr() which is required by > those macros: > > > https://developer.gnome.org/gobject/stable/gobject-Type-Information.html#G- > DECLARE-FINAL-TYPE:CAPS > > > "You can only use this function if your parent type also supports g_autoptr()." Indeed. The following in window-private.h should fix that: G_DEFINE_AUTOPTR_CLEANUP_FUNC(MetaWindowX11, g_object_unref)
Created attachment 362146 [details] [review] [PATCH 1/4] xwayland: Add MetaWindowXwayland MetaWindowXwayland derives from MetaWindowX11 to allow for some Xwayland specific vfunc that wouldn't apply to plain X11 windows, such as shortcut inhibit routines. Chnages: - Use G_DECLARE_FINAL_TYPE() instead of old style definitions.
Created attachment 362147 [details] [review] [PATCH 3/4] settings: Add xwayland grab settings Add new settings to control which X11 windows are allowed to issue Xwayland grabs. Changes: - Use "acces_rules" instead of "whitelist where applicable, including the configure option, gsettings key name, function names, signal name
Created attachment 362148 [details] [review] [PATCH 4/4] wayland: Add Xwayland grab keyboard support This protocol is limited to Xwayland only and is not visible/usable by any other client. Mutter uses the following mechanisms to determine if an X11 client should be granted a grab: - is "xwayland-allow-grabs" set? - if set, is the client blacklisted? - otherwise, has the client set the X11 window property _XWAYLAND_MAY_GRAB_KEYBOARD on the window using a client message? - if not, is it a client white-listed either via the default system list or the settings "xwayland-grab-access-rules"? Changes: - Disconnect window associate handler in meta_xwayland_keyboard_grab_end() - Test grab->interface->key to determine is a grab is in effect instead of using a boolean - Use new api meta_settings_are_xwayland_grabs_allowed() instead of meta_settings_get_xwayland_allow_grabs()
Argh sorry, wrong patches uploaded! /me tries again
Created attachment 362150 [details] [review] [PATCH 1/4] xwayland: Add MetaWindowXwayland
Created attachment 362151 [details] [review] [PATCH 3/4] settings: Add xwayland grab settings Changes: - Use "acces_rules" instead of "whitelist where applicable, including the configure option, gsettings key name, function names, signal name - Rename meta_settings_get_xwayland_allow_grabs() as meta_settings_are_xwayland_grabs_allowed()
Created attachment 362152 [details] [review] [PATCH 4/4] wayland: Add Xwayland grab keyboard support Changes: - Disconnect window associate handler in meta_xwayland_keyboard_grab_end() - Test grab->interface->key to determine is a grab is in effect instead of using a boolean - Use new api meta_settings_are_xwayland_grabs_allowed() instead of meta_settings_get_xwayland_allow_grabs()
(In reply to Jonas Ådahl from comment #131) [...] > All our grab APIs are terrible, but I think the most "correct" way to check > whether we actually still have the grab is to check whether the > keyboard->grab->key == meta_xwayland_keyboard_grab_key. (see > https://bugzilla.gnome.org/show_bug.cgi?id=785218 for a brighter future) Speaking of which, should we actually implement that keyboard grab? While this "emulates" best the old X11 behavior and fixes those old apps that rely on such a behavior (like vmware-gksu), it also (re)opens the door for all sort of abuse (the risk is mitigated by the blacklist and "xwayland-allow-grabs" being FALSE by default), so I wonder if it is wise to do the actual grab... Most apps do not use O-R and would work without the grab.
(In reply to Olivier Fourdan from comment #146) > (In reply to Jonas Ådahl from comment #131) > [...] > > All our grab APIs are terrible, but I think the most "correct" way to check > > whether we actually still have the grab is to check whether the > > keyboard->grab->key == meta_xwayland_keyboard_grab_key. (see > > https://bugzilla.gnome.org/show_bug.cgi?id=785218 for a brighter future) > > Speaking of which, should we actually implement that keyboard grab? That grab is not for clients to invoke, it is more about unifying the 3, 4 or so different grabbing mechanism we have internally in mutter/clutter/gnome-shell . The comment was more about how awkward our MetaWayland[Keyboard|Pointer|Touch]Grab API is.
Review of attachment 362150 [details] [review]: lgtm.
Review of attachment 362151 [details] [review]: lgtm. marking as reviewed because I'd like to get a second opinion on the gsettings schema (as it'll have long term consequences).
Created attachment 363040 [details] [review] [PATCH 4/4] wayland: Add Xwayland grab keyboard support What changed: Start the mutter grab only when dealing with override-redirect windows that would never receive keyboard focus otherwise, so we don't "lock" keyboard focus for regular windows.
(In reply to Jonas Ådahl from comment #149) > lgtm. marking as reviewed because I'd like to get a second opinion on the > gsettings schema (as it'll have long term consequences). Who should we ask for that second opinion?
Review of attachment 362151 [details] [review]: the gsettings schema seems fine now as it allows user configuration to always override the upstream defaults even if they change in the future ::: src/backends/meta-settings.c @@ +392,3 @@ + { + update_xwayland_grab_access_rules (settings); + g_signal_emit (settings, signals[XWAYLAND_GRAB_ACCESS_RULES_CHANGED], 0); this signal seems unused, do we need to keep it?
Created attachment 364327 [details] [review] [PATCH 3/4] settings: Add xwayland grab settings (In reply to Rui Matos from comment #152) > Review of attachment 362151 [details] [review] [review]: > > the gsettings schema seems fine now as it allows user configuration to > always override the upstream defaults even if they change in the future > > ::: src/backends/meta-settings.c > @@ +392,3 @@ > + { > + update_xwayland_grab_access_rules (settings); > + g_signal_emit (settings, signals[XWAYLAND_GRAB_ACCESS_RULES_CHANGED], > 0); > > this signal seems unused, do we need to keep it? Oh you're right, well spotted, thanks!
Humble ping, anyone to do a review the two remaining patches (attachment 363040 [details] [review] and attachment 364327 [details] [review]) please?
Review of attachment 364327 [details] [review]: lgtm
Review of attachment 363040 [details] [review]: ::: src/wayland/meta-xwayland-grab-keyboard.c @@ +275,3 @@ + active_grab->window_associate_handler = + g_signal_connect (surface->role, "window-associated", + G_CALLBACK (meta_xwayland_keyboard_grab_activate), This doesn't look correct. the "window-associated" signal will pass a MetaWaylandSurfaceRole as the first argument, but this function takes a MetaWaylandSurface.
Created attachment 365682 [details] [review] [PATCH 4/4] wayland: Add Xwayland grab keyboard support (In reply to Jonas Ådahl from comment #156) > Review of attachment 363040 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-grab-keyboard.c > @@ +275,3 @@ > + active_grab->window_associate_handler = > + g_signal_connect (surface->role, "window-associated", > + G_CALLBACK (meta_xwayland_keyboard_grab_activate), > > This doesn't look correct. the "window-associated" signal will pass a > MetaWaylandSurfaceRole as the first argument, but this function takes a > MetaWaylandSurface. Oh right! Actually, we don't even need the surface in meta_xwayland_keyboard_grab_activate() since it's already part of the MetaXwaylandKeyboardActiveGrab stucture... Updated patch attached.
Review of attachment 365682 [details] [review]: lgtm
Comment on attachment 362150 [details] [review] [PATCH 1/4] xwayland: Add MetaWindowXwayland attachment 362150 [details] [review] pushed to git master as commit 1546989 - xwayland: Add MetaWindowXwayland
Comment on attachment 362072 [details] [review] [PATCH 2/4] xwayland: add _XWAYLAND_MAY_GRAB_KEYBOARD property attachment 362072 [details] [review] pushed to git master as commit 5f132f3 - xwayland: add _XWAYLAND_MAY_GRAB_KEYBOARD property
Comment on attachment 364327 [details] [review] [PATCH 3/4] settings: Add xwayland grab settings attachment 364327 [details] [review] pushed to git master as commit 519a0fd - settings: Add xwayland grab settings
Comment on attachment 365682 [details] [review] [PATCH 4/4] wayland: Add Xwayland grab keyboard support attachment 365682 [details] [review] pushed to git master as commit 072afa5 - wayland: Add Xwayland grab keyboard support
Thanks!
Hi, I am a qemu/spice developer, and the inhibitshortcut dialog is annoying me. Can I disable it? whitelist some apps? Is there documentation for that feature somewhere, or do I need to read through all that bug and code to figure out? with love, many thanks !
(In reply to Marc-Andre Lureau from comment #164) > Hi, I am a qemu/spice developer, and the inhibitshortcut dialog is annoying > me. > > Can I disable it? whitelist some apps? > > Is there documentation for that feature somewhere, or do I need to read > through all that bug and code to figure out? > > with love, many thanks ! This bug is about the allow-xwayland-to-grab feature, which is disabled by default, not the inhibit shortcuts dialog. If you want to avoid having your Wayland client (presumingly using gtk) not try to inhibit shortcuts, you'll have to avoid explicitly grabbing only the keyboard on a toplevel (not popup/menu) window. Make sure you have the "gdk/wayland: Restrict shortcut inhibition to keyboard grabs on toplevels" gtk+ commit which should limit the exposure to said dialog.
I reckon qemu/spice needs the shortcut inhibitor (to be able to receive the shell shortcuts and pass the mto the virtual machine, that's why it grabs the keyboad on X11) There is no option to disable the shortcut inhibitor dialog in neither gnome-shell nor mutter.