After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 783342 - Wayland: RFC - add xwayland keyboard grab / shortcut inhibitor support
Wayland: RFC - add xwayland keyboard grab / shortcut inhibitor support
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal enhancement
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2017-06-02 09:22 UTC by Olivier Fourdan
Modified: 2018-01-25 15:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH 1/4] configure.ac: update wayland-server requirement (943 bytes, patch)
2017-06-02 09:23 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/4] wayland: add inhibit shortcut mechanism (8.01 KB, patch)
2017-06-02 09:23 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4] wayland: add Xwayland grab keyboard support (14.22 KB, patch)
2017-06-02 09:24 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4] wayland: add keyboard shortcuts inhibitor protocol (10.54 KB, patch)
2017-06-02 09:24 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4 v2] wayland: add keyboard shortcuts inhibitor protocol (11.27 KB, patch)
2017-06-02 10:04 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4 v2] wayland: add Xwayland grab keyboard support (14.29 KB, patch)
2017-06-02 13:04 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/4 v2] wayland: add inhibit shortcut mechanism (16.87 KB, patch)
2017-06-08 15:18 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4 v3] wayland: add keyboard shortcuts inhibitor protocol (12.78 KB, patch)
2017-06-08 15:20 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/4 v3] wayland: add inhibit shortcut mechanism (16.92 KB, patch)
2017-06-08 16:21 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/9] configure.ac: update wayland-server requirement (994 bytes, patch)
2017-06-16 11:34 UTC, Olivier Fourdan
committed Details | Review
[PATCH 2/8] wayland: add inhibit shortcut mechanism (16.97 KB, patch)
2017-06-16 11:34 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/8] wayland: add Xwayland grab keyboard support (14.34 KB, patch)
2017-06-16 11:35 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/8] wayland: add keyboard shortcuts inhibitor protocol (12.83 KB, patch)
2017-06-16 11:35 UTC, Olivier Fourdan
none Details | Review
[PATCH 5/8] core: add MetaInhibitShortcutsDialog (6.91 KB, patch)
2017-06-16 11:35 UTC, Olivier Fourdan
none Details | Review
[PATCH 6/8] core: implement MetaInhibitShortcutsDialogDefault (11.84 KB, patch)
2017-06-16 11:36 UTC, Olivier Fourdan
none Details | Review
[PATCH 7/8] compositor: add vmethod to override inhibit shortcut dialog (4.84 KB, patch)
2017-06-16 11:36 UTC, Olivier Fourdan
none Details | Review
[PATCH 8/8] wayland: use the inhibit shortcuts dialog (9.51 KB, patch)
2017-06-16 11:37 UTC, Olivier Fourdan
none Details | Review
ui: Add InhibitShortcutsDialog (11.19 KB, patch)
2017-07-14 14:17 UTC, Florian Müllner
none Details | Review
ui: Add InhibitShortcutsDialog (10.29 KB, patch)
2017-07-16 20:03 UTC, Florian Müllner
none Details | Review
ui: Add InhibitShortcutsDialog (10.65 KB, patch)
2017-07-18 14:48 UTC, Florian Müllner
committed Details | Review
[PATCH 2/9] wayland: add inhibit shortcut mechanism (20.99 KB, patch)
2017-07-18 15:04 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/9] configure.ac: update wayland-protocols requirement (1.13 KB, patch)
2017-07-18 15:06 UTC, Olivier Fourdan
accepted-commit_now Details | Review
[PATCH 4/9] wayland: add Xwayland grab keyboard support (13.85 KB, patch)
2017-07-18 15:10 UTC, Olivier Fourdan
none Details | Review
[PATCH 5/9] wayland: add keyboard shortcuts inhibitor protocol (12.64 KB, patch)
2017-07-18 15:10 UTC, Olivier Fourdan
committed Details | Review
[PATCH 6/9] core: add MetaInhibitShortcutsDialog (7.00 KB, patch)
2017-07-18 15:11 UTC, Olivier Fourdan
committed Details | Review
[PATCH 7/9] core: implement MetaInhibitShortcutsDialogDefault (7.15 KB, patch)
2017-07-18 15:12 UTC, Olivier Fourdan
committed Details | Review
[PATCH 8/9] compositor: add vmethod to override inhibit shortcut (4.84 KB, patch)
2017-07-18 15:12 UTC, Olivier Fourdan
committed Details | Review
[PATCH 9/9] wayland: use the inhibit shortcuts dialog (10.27 KB, patch)
2017-07-18 15:13 UTC, Olivier Fourdan
none Details | Review
[PATCH 10/9] wayland: remember last choice for inhibit shortcut (4.70 KB, patch)
2017-07-19 10:13 UTC, Olivier Fourdan
none Details | Review
[PATCH 9/9] wayland: use the inhibit shortcuts dialog (11.43 KB, patch)
2017-07-21 09:11 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/9] wayland: add Xwayland grab keyboard support (14.45 KB, patch)
2017-07-30 11:29 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/9] wayland: add inhibit shortcut mechanism (20.47 KB, patch)
2017-07-31 13:50 UTC, Olivier Fourdan
committed Details | Review
[PATCH 9/9] wayland: use the inhibit shortcuts dialog (11.57 KB, patch)
2017-07-31 14:09 UTC, Olivier Fourdan
none Details | Review
[PATCH 9/9] wayland: use the inhibit shortcuts dialog (11.51 KB, patch)
2017-07-31 15:38 UTC, Olivier Fourdan
committed Details | Review
[PATCH 1/3] xwayland: Add MetaWindowXWayland (6.47 KB, patch)
2017-09-01 12:00 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/3] settings: Add "xwayland-grab-granted" setting (6.82 KB, patch)
2017-09-01 12:01 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/3] wayland: Add Xwayland grab keyboard support (14.24 KB, patch)
2017-09-01 12:02 UTC, Olivier Fourdan
none Details | Review
[PATCH v2 1/3] xwayland: Add MetaWindowXwayland (7.32 KB, patch)
2017-09-21 09:04 UTC, Olivier Fourdan
none Details | Review
[PATCH v2 2/3] settings: Add "xwayland-grab-*-whitelist" settings (9.24 KB, patch)
2017-09-21 09:05 UTC, Olivier Fourdan
none Details | Review
[PATCH v2 3/3] wayland: Add Xwayland grab keyboard support (14.66 KB, patch)
2017-09-21 09:05 UTC, Olivier Fourdan
none Details | Review
[PATCH v3 2/3] settings: Add "xwayland-grab-whitelist" setting (10.88 KB, patch)
2017-09-22 13:31 UTC, Olivier Fourdan
none Details | Review
[PATCH v3 3/3] wayland: Add Xwayland grab keyboard support (15.02 KB, patch)
2017-09-22 13:32 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/4] xwayland: Add MetaWindowXwayland (7.32 KB, patch)
2017-10-23 08:18 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/4] xwayland: add _XWAYLAND_MAY_GRAB_KEYBOARD property (5.43 KB, patch)
2017-10-23 08:19 UTC, Olivier Fourdan
committed Details | Review
[PATCH 3/4] settings: Add xwayland grab settings (11.28 KB, patch)
2017-10-23 08:19 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4] wayland: Add Xwayland grab keyboard support (18.07 KB, patch)
2017-10-23 08:20 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/4] xwayland: Add MetaWindowXwayland (7.32 KB, patch)
2017-10-24 07:40 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4] settings: Add xwayland grab settings (11.28 KB, patch)
2017-10-24 07:43 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4] wayland: Add Xwayland grab keyboard support (18.07 KB, patch)
2017-10-24 07:47 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/4] xwayland: Add MetaWindowXwayland (7.32 KB, patch)
2017-10-24 07:50 UTC, Olivier Fourdan
committed Details | Review
[PATCH 3/4] settings: Add xwayland grab settings (11.63 KB, patch)
2017-10-24 07:53 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4] wayland: Add Xwayland grab keyboard support (18.57 KB, patch)
2017-10-24 07:53 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4] wayland: Add Xwayland grab keyboard support (18.70 KB, patch)
2017-11-06 09:44 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4] settings: Add xwayland grab settings (10.94 KB, patch)
2017-11-24 13:24 UTC, Olivier Fourdan
committed Details | Review
[PATCH 4/4] wayland: Add Xwayland grab keyboard support (18.89 KB, patch)
2017-12-18 10:49 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2017-06-02 09:22:15 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.
Comment 1 Olivier Fourdan 2017-06-02 09:23:11 UTC
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.
Comment 2 Olivier Fourdan 2017-06-02 09:23:46 UTC
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.
Comment 3 Olivier Fourdan 2017-06-02 09:24:18 UTC
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.
Comment 4 Olivier Fourdan 2017-06-02 09:24:41 UTC
Created attachment 353062 [details] [review]
[PATCH 4/4] wayland: add keyboard shortcuts inhibitor protocol
Comment 5 Olivier Fourdan 2017-06-02 10:04:53 UTC
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)
Comment 6 Olivier Fourdan 2017-06-02 13:04:04 UTC
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
Comment 7 Olivier Fourdan 2017-06-08 15:18:42 UTC
Created attachment 353391 [details] [review]
[PATCH 2/4 v2] wayland: add inhibit shortcut mechanism

adds new keybinding flag ("non_maskable") and signals for surfaces
Comment 8 Olivier Fourdan 2017-06-08 15:20:11 UTC
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
Comment 9 Olivier Fourdan 2017-06-08 16:21:37 UTC
Created attachment 353405 [details] [review]
[PATCH 2/4 v3] wayland: add inhibit shortcut mechanism
Comment 10 Olivier Fourdan 2017-06-16 11:34:05 UTC
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.
Comment 11 Olivier Fourdan 2017-06-16 11:34:35 UTC
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.
Comment 12 Olivier Fourdan 2017-06-16 11:35:00 UTC
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.
Comment 13 Olivier Fourdan 2017-06-16 11:35:27 UTC
Created attachment 353883 [details] [review]
[PATCH 4/8] wayland: add keyboard shortcuts inhibitor protocol
Comment 14 Olivier Fourdan 2017-06-16 11:35:55 UTC
Created attachment 353884 [details] [review]
[PATCH 5/8] core: add MetaInhibitShortcutsDialog

Add a new interface for allowing or denying shortcuts inhibit requests.
Comment 15 Olivier Fourdan 2017-06-16 11:36:20 UTC
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.
Comment 16 Olivier Fourdan 2017-06-16 11:36:46 UTC
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.
Comment 17 Olivier Fourdan 2017-06-16 11:37:41 UTC
Created attachment 353887 [details] [review]
[PATCH 8/8] wayland: use the inhibit shortcuts dialog

Plug the new MetaInhbitShortcutsDialog to the relevant Wayland protocol
implementation.
Comment 18 Piotr Drąg 2017-06-16 16:48:59 UTC
(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"));
                                                            ^^
Comment 19 Jonas Ådahl 2017-07-11 10:05:57 UTC
Review of attachment 353880 [details] [review]:

lgtm.
Comment 20 Jonas Ådahl 2017-07-11 10:06:26 UTC
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.
Comment 21 Jonas Ådahl 2017-07-11 10:06:43 UTC
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.
Comment 22 Jonas Ådahl 2017-07-11 10:06:57 UTC
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.
Comment 23 Jonas Ådahl 2017-07-13 08:40:45 UTC
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.
Comment 24 Jonas Ådahl 2017-07-13 08:55:43 UTC
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.
Comment 25 Jonas Ådahl 2017-07-13 08:59:28 UTC
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.
Comment 26 Jonas Ådahl 2017-07-13 09:00:31 UTC
Review of attachment 353886 [details] [review]:

lgtm.
Comment 27 Jonas Ådahl 2017-07-13 09:05:58 UTC
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.
Comment 28 Olivier Fourdan 2017-07-13 11:53:02 UTC
(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)
Comment 29 Jonas Ådahl 2017-07-13 13:21:26 UTC
(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.
Comment 30 Florian Müllner 2017-07-13 13:42:06 UTC
(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.
Comment 31 Olivier Fourdan 2017-07-13 15:04:15 UTC
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).
Comment 32 Jonas Ådahl 2017-07-13 15:14:09 UTC
(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.
Comment 33 Olivier Fourdan 2017-07-13 15:24:52 UTC
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...
Comment 34 Jonas Ådahl 2017-07-13 15:34:47 UTC
(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).
Comment 35 Florian Müllner 2017-07-13 22:13:11 UTC
(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?
Comment 36 Jonas Ådahl 2017-07-14 00:58:22 UTC
(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.
Comment 37 Florian Müllner 2017-07-14 14:17:20 UTC
Created attachment 355599 [details] [review]
ui: Add InhibitShortcutsDialog

Quick shell implementation for testing + design feedback.
Comment 38 Florian Müllner 2017-07-16 20:03:30 UTC
Created attachment 355723 [details] [review]
ui: Add InhibitShortcutsDialog

Rebased to master.
Comment 39 Olivier Fourdan 2017-07-17 06:45:28 UTC
(In reply to Florian Müllner from comment #37)
> Quick shell implementation for testing + design feedback.

Oh that's awesome, thanks so much Florian!
Comment 40 Olivier Fourdan 2017-07-18 14:43:48 UTC
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
Comment 41 Florian Müllner 2017-07-18 14:48:24 UTC
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 ...)
Comment 42 Olivier Fourdan 2017-07-18 14:57:01 UTC
nice! :) It works!
Comment 43 Olivier Fourdan 2017-07-18 15:04:30 UTC
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...
Comment 44 Olivier Fourdan 2017-07-18 15:06:08 UTC
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.
Comment 45 Olivier Fourdan 2017-07-18 15:10:00 UTC
Created attachment 355845 [details] [review]
[PATCH 4/9] wayland: add Xwayland grab keyboard support
Comment 46 Olivier Fourdan 2017-07-18 15:10:40 UTC
Created attachment 355846 [details] [review]
[PATCH 5/9] wayland: add keyboard shortcuts inhibitor protocol
Comment 47 Olivier Fourdan 2017-07-18 15:11:32 UTC
Created attachment 355847 [details] [review]
[PATCH 6/9] core: add MetaInhibitShortcutsDialog
Comment 48 Olivier Fourdan 2017-07-18 15:12:12 UTC
Created attachment 355848 [details] [review]
[PATCH 7/9] core: implement MetaInhibitShortcutsDialogDefault
Comment 49 Olivier Fourdan 2017-07-18 15:12:47 UTC
Created attachment 355849 [details] [review]
[PATCH 8/9] compositor: add vmethod to override inhibit shortcut
Comment 50 Olivier Fourdan 2017-07-18 15:13:26 UTC
Created attachment 355850 [details] [review]
[PATCH 9/9] wayland: use the inhibit shortcuts dialog
Comment 51 Olivier Fourdan 2017-07-18 15:43:32 UTC
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.
Comment 52 Florian Müllner 2017-07-18 19:16:00 UTC
(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?
Comment 53 Olivier Fourdan 2017-07-18 19:41:18 UTC
(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.
Comment 54 Olivier Fourdan 2017-07-19 08:23:42 UTC
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)
Comment 55 Olivier Fourdan 2017-07-19 08:25:56 UTC
...or we could even avoid asking, like automatically remember the user choice as long as the app is running. Would that be acceptable?
Comment 56 Olivier Fourdan 2017-07-19 10:13:51 UTC
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).
Comment 57 Olivier Fourdan 2017-07-21 09:11:57 UTC
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.
Comment 58 Olivier Fourdan 2017-07-30 11:29:30 UTC
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.
Comment 59 Jonas Ådahl 2017-07-30 18:04:39 UTC
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.
Comment 60 Jonas Ådahl 2017-07-30 18:05:34 UTC
Review of attachment 355844 [details] [review]:

lgtm.
Comment 61 Jonas Ådahl 2017-07-30 18:10:50 UTC
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().
Comment 62 Jonas Ådahl 2017-07-30 18:15:43 UTC
Review of attachment 355847 [details] [review]:

lgtm.
Comment 63 Jonas Ådahl 2017-07-30 18:17:05 UTC
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.
Comment 64 Jonas Ådahl 2017-07-31 09:26:05 UTC
Review of attachment 355849 [details] [review]:

lgtm.
Comment 65 Jonas Ådahl 2017-07-31 09:50:08 UTC
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.
Comment 66 Olivier Fourdan 2017-07-31 13:50:52 UTC
Created attachment 356645 [details] [review]
[PATCH 2/9] wayland: add inhibit shortcut mechanism

New version following review in comment 59
Comment 67 Olivier Fourdan 2017-07-31 14:09:59 UTC
Created attachment 356647 [details] [review]
[PATCH 9/9] wayland: use the inhibit shortcuts dialog

Update after review in comment 65
Comment 68 Olivier Fourdan 2017-07-31 15:38:59 UTC
Created attachment 356654 [details] [review]
[PATCH 9/9] wayland: use the inhibit shortcuts dialog

Oops, forgot the "typedef struct" from comment 65.
Comment 69 Jonas Ådahl 2017-07-31 15:46:23 UTC
Review of attachment 356645 [details] [review]:

lgtm
Comment 70 Jonas Ådahl 2017-07-31 15:48:06 UTC
Review of attachment 356654 [details] [review]:

lgtm
Comment 71 Olivier Fourdan 2017-08-01 10:02:52 UTC
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.
Comment 72 Olivier Fourdan 2017-08-01 22:15:32 UTC
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?
Comment 73 Florian Müllner 2017-08-02 09:29:54 UTC
(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 p‌atch 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 74 Olivier Fourdan 2017-08-02 10:09:27 UTC
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 75 Olivier Fourdan 2017-08-02 10:11:52 UTC
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 76 Olivier Fourdan 2017-08-02 10:13:22 UTC
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 77 Olivier Fourdan 2017-08-02 10:14:42 UTC
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 78 Olivier Fourdan 2017-08-02 10:16:20 UTC
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 79 Olivier Fourdan 2017-08-02 10:17:35 UTC
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 80 Olivier Fourdan 2017-08-02 10:18:34 UTC
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 81 Olivier Fourdan 2017-08-02 10:19:47 UTC
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 82 Olivier Fourdan 2017-08-02 10:20:42 UTC
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 83 Florian Müllner 2017-08-02 10:36:13 UTC
Comment on attachment 355840 [details] [review]
ui: Add InhibitShortcutsDialog

Attachment 355840 [details] pushed as dff3e4e - ui: Add InhibitShortcutsDialog
Comment 84 Olivier Fourdan 2017-09-01 12:00:46 UTC
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.
Comment 85 Olivier Fourdan 2017-09-01 12:01:31 UTC
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.
Comment 86 Olivier Fourdan 2017-09-01 12:02:28 UTC
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.
Comment 87 Jonas Ådahl 2017-09-19 08:21:47 UTC
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)
Comment 88 Jonas Ådahl 2017-09-19 08:39:11 UTC
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?
Comment 89 Jonas Ådahl 2017-09-19 09:16:42 UTC
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
Comment 90 Olivier Fourdan 2017-09-20 15:34:27 UTC
(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.
Comment 91 Jonas Ådahl 2017-09-20 15:40:31 UTC
(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.
Comment 92 Olivier Fourdan 2017-09-21 06:34:20 UTC
(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...
Comment 93 Olivier Fourdan 2017-09-21 06:50:16 UTC
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
Comment 94 Olivier Fourdan 2017-09-21 07:39:18 UTC
(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?..
Comment 95 Olivier Fourdan 2017-09-21 09:04:09 UTC
Created attachment 360171 [details] [review]
[PATCH v2 1/3] xwayland: Add MetaWindowXwayland
Comment 96 Olivier Fourdan 2017-09-21 09:05:01 UTC
Created attachment 360172 [details] [review]
[PATCH v2 2/3] settings: Add "xwayland-grab-*-whitelist" settings
Comment 97 Olivier Fourdan 2017-09-21 09:05:42 UTC
Created attachment 360173 [details] [review]
[PATCH v2 3/3] wayland: Add Xwayland grab keyboard support
Comment 98 Rui Matos 2017-09-21 13:42:07 UTC
(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 '!' .
Comment 99 Olivier Fourdan 2017-09-21 13:56:58 UTC
(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.
Comment 100 Olivier Fourdan 2017-09-21 14:00:03 UTC
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)
Comment 101 Rui Matos 2017-09-21 14:22:13 UTC
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.
Comment 102 Olivier Fourdan 2017-09-21 14:51:38 UTC
OK, I see how we could do it...
Comment 103 Olivier Fourdan 2017-09-22 13:31:48 UTC
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.
Comment 104 Olivier Fourdan 2017-09-22 13:32:45 UTC
Created attachment 360261 [details] [review]
[PATCH v3 3/3] wayland: Add Xwayland grab keyboard support

Use whitelist/blacklist
Comment 105 Olivier Fourdan 2017-10-03 09:33:17 UTC
Anyone to redo a review of these new iteration? I hope it does everything that was requested previously.
Comment 106 Pierre Ossman 2017-10-10 14:53:05 UTC
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.
Comment 107 Olivier Fourdan 2017-10-10 15:17:07 UTC
apps developpers can add the the wm_class to the gsettings
Comment 108 Pierre Ossman 2017-10-10 15:31:03 UTC
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. :)
Comment 109 Olivier Fourdan 2017-10-10 15:35:51 UTC
Hehe, err, well, my comment 107 wasn't a recommendation per se, merely stating what would possible with the proposed solution :)
Comment 110 Jonas Ådahl 2017-10-11 09:50:30 UTC
(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!".
Comment 111 Olivier Fourdan 2017-10-11 09:56:16 UTC
(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?
Comment 112 Jonas Ådahl 2017-10-11 10:04:15 UTC
(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).
Comment 113 Pierre Ossman 2017-10-11 10:08:50 UTC
> (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)
Comment 114 Olivier Fourdan 2017-10-11 10:10:56 UTC
(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.
Comment 115 Olivier Fourdan 2017-10-11 10:17:47 UTC
(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.
Comment 116 Jonas Ådahl 2017-10-11 10:18:54 UTC
(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.
Comment 117 Pierre Ossman 2017-10-11 10:26:33 UTC
(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. :/
Comment 118 Olivier Fourdan 2017-10-11 11:10:17 UTC
(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
Comment 119 Olivier Fourdan 2017-10-11 11:13:55 UTC
(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)?
Comment 120 Pierre Ossman 2017-10-11 11:23:22 UTC
(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?
Comment 121 Olivier Fourdan 2017-10-11 11:31:33 UTC
(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.
Comment 122 Pierre Ossman 2017-10-11 11:48:23 UTC
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.
Comment 123 Olivier Fourdan 2017-10-23 08:18:20 UTC
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.
Comment 124 Olivier Fourdan 2017-10-23 08:19:11 UTC
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.
Comment 125 Olivier Fourdan 2017-10-23 08:19:54 UTC
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.
Comment 126 Olivier Fourdan 2017-10-23 08:20:42 UTC
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"?
Comment 127 Olivier Fourdan 2017-10-23 08:28:08 UTC
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).
Comment 128 Jonas Ådahl 2017-10-23 09:05:56 UTC
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?
Comment 129 Jonas Ådahl 2017-10-23 09:07:40 UTC
Review of attachment 362072 [details] [review]:

lgtm.
Comment 130 Jonas Ådahl 2017-10-23 09:08:23 UTC
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.
Comment 131 Jonas Ådahl 2017-10-23 09:10:38 UTC
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)
Comment 132 Olivier Fourdan 2017-10-23 09:38:26 UTC
(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.
Comment 133 Olivier Fourdan 2017-10-23 09:43:38 UTC
(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.
Comment 134 Jonas Ådahl 2017-10-23 09:45:47 UTC
(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)
Comment 135 Jonas Ådahl 2017-10-23 09:48:19 UTC
(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)
Comment 136 Olivier Fourdan 2017-10-23 09:56:10 UTC
(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).
Comment 137 Olivier Fourdan 2017-10-23 15:21:11 UTC
(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()."
Comment 138 Jonas Ådahl 2017-10-23 15:26:36 UTC
(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)
Comment 139 Olivier Fourdan 2017-10-24 07:40:45 UTC
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.
Comment 140 Olivier Fourdan 2017-10-24 07:43:27 UTC
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
Comment 141 Olivier Fourdan 2017-10-24 07:47:29 UTC
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()
Comment 142 Olivier Fourdan 2017-10-24 07:49:06 UTC
Argh sorry, wrong patches uploaded!

/me tries again
Comment 143 Olivier Fourdan 2017-10-24 07:50:49 UTC
Created attachment 362150 [details] [review]
[PATCH 1/4] xwayland: Add MetaWindowXwayland
Comment 144 Olivier Fourdan 2017-10-24 07:53:01 UTC
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()
Comment 145 Olivier Fourdan 2017-10-24 07:53:48 UTC
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()
Comment 146 Olivier Fourdan 2017-10-26 07:04:36 UTC
(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.
Comment 147 Jonas Ådahl 2017-10-26 10:06:32 UTC
(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.
Comment 148 Jonas Ådahl 2017-10-30 10:09:32 UTC
Review of attachment 362150 [details] [review]:

lgtm.
Comment 149 Jonas Ådahl 2017-10-30 10:11:19 UTC
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).
Comment 150 Olivier Fourdan 2017-11-06 09:44:35 UTC
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.
Comment 151 Olivier Fourdan 2017-11-16 09:52:33 UTC
(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?
Comment 152 Rui Matos 2017-11-24 11:14:29 UTC
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?
Comment 153 Olivier Fourdan 2017-11-24 13:24:11 UTC
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!
Comment 154 Olivier Fourdan 2017-12-14 09:58:16 UTC
Humble ping, anyone to do a review the two remaining patches (attachment 363040 [details] [review] and attachment 364327 [details] [review]) please?
Comment 155 Jonas Ådahl 2017-12-18 09:44:35 UTC
Review of attachment 364327 [details] [review]:

lgtm
Comment 156 Jonas Ådahl 2017-12-18 09:51:58 UTC
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.
Comment 157 Olivier Fourdan 2017-12-18 10:49:58 UTC
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.
Comment 158 Jonas Ådahl 2017-12-18 11:31:33 UTC
Review of attachment 365682 [details] [review]:

lgtm
Comment 159 Olivier Fourdan 2017-12-18 12:25:54 UTC
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 160 Olivier Fourdan 2017-12-18 12:26:30 UTC
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 161 Olivier Fourdan 2017-12-18 12:27:19 UTC
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 162 Olivier Fourdan 2017-12-18 12:27:51 UTC
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
Comment 163 Olivier Fourdan 2017-12-18 12:28:08 UTC
Thanks!
Comment 164 Marc-Andre Lureau 2018-01-25 15:20:17 UTC
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 !
Comment 165 Jonas Ådahl 2018-01-25 15:26:00 UTC
(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.
Comment 166 Olivier Fourdan 2018-01-25 15:37:24 UTC
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.