Created attachment 240260 [details] [review]
keybindings: Add API to freeze/unfreeze the keyboard

We'll use this in gnome-shell to freeze the keyboard right before
switching input source and unfreeze it after that's finished so that
we don't lose any key events to the wrong input source.

Review of attachment 240260 [details] [review]:

What's the utility of meta_display_grab_keyboard / meta_display_ungrab_keyboard?

(In reply to comment #2)
> Review of attachment 240260 [details] [review]:
> 
> What's the utility of meta_display_grab_keyboard /
> meta_display_ungrab_keyboard?

See bug 697007. If we have an active key grab (e.g. pushModal()) we have to freeze the key events otherwise we have to establish a keyboard grab in sync mode.

Created attachment 241352 [details] [review]
keybindings: Add API to freeze/unfreeze the keyboard

--

Found some problems with this:

As opposed to what I thought, XIAllowEvents(..., XISyncDevice, ...)
doesn't actually freeze events unless the initial grab was setup in
XIGrabModeSync which is not the case when we are in a compositor grab,
i.e. meta_begin_modal_for_plugin().

That means that when we want to freeze keyboard events we have to call
XIGrabDevice() with XIGrabModeSync regardless of already having a
grab. It also means that we can't grab on the root window as I was
doing before but instead have to grab on the stage's window otherwise
the stage would lose the focus and everything would stop working so I
also had to add a Window parameter so that the shell can pass in the
stage's window.

This uncovered a bug in the server's implementation of XIGrabDevice()
and to work around it we have to freeze both the keyboard and the
pointer and thus then have to XIAllowEvents() on both devices as well.

Created attachment 241353 [details]
X server patch

(In reply to comment #4)
> This uncovered a bug in the server's implementation of XIGrabDevice()
> and to work around it we have to freeze both the keyboard and the
> pointer and thus then have to XIAllowEvents() on both devices as well.

For the record, this is the patch for that server's bug which I'm going to submit upstream.

Review of attachment 241352 [details] [review]:

Looks good, but the XI workarounds are not obvious enough to leave them uncommented ...

::: src/core/keybindings.c
@@ +1287,3 @@
                               timestamp,
                               None,
+                              grab_mode, grab_mode,

Please add a comment why the mode is applied to the paired device as well (it's too non-obvious to only have it in bugzilla)

@@ +1440,3 @@
+                 XIAsyncDevice, timestamp);
+  XIAllowEvents (display->xdisplay, META_VIRTUAL_CORE_POINTER_ID,
+                 XIAsyncDevice, timestamp);

Same as above.

Created attachment 244111 [details] [review]
keybindings: Add API to freeze/unfreeze the keyboard

--
(In reply to comment #6)
> Looks good, but the XI workarounds are not obvious enough to leave them
> uncommented ...
>
> ::: src/core/keybindings.c
> @@ +1287,3 @@
>                                timestamp,
>                                None,
> +                              grab_mode, grab_mode,
>
> Please add a comment why the mode is applied to the paired device as well (it's
> too non-obvious to only have it in bugzilla)

Of course. What about this?

Review of attachment 244111 [details] [review]:

Works for me

::: src/core/keybindings.c
@@ +1448,3 @@
+  XIAllowEvents (display->xdisplay, META_VIRTUAL_CORE_KEYBOARD_ID,
+                 XIAsyncDevice, timestamp);
+  /* We shouldn't need to unfreeze the pointer device here however we

Should there be a comma between "here" and "however"?

Attachment 244111 [details] pushed as 056cc16 - keybindings: Add API to freeze/unfreeze the keyboard