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 706963 - wayland: implement global and window keybindings
wayland: implement global and window keybindings
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
wayland 3.10
Depends on:
Blocks:
 
 
Reported: 2013-08-28 09:19 UTC by Giovanni Campagna
Modified: 2013-11-15 02:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: implement global and window keybindings (11.24 KB, patch)
2013-08-28 09:19 UTC, Giovanni Campagna
reviewed Details | Review
wayland: reimplement keyboard state handling properly (15.62 KB, patch)
2013-09-04 13:18 UTC, Giovanni Campagna
none Details | Review
wayland: reimplement keyboard state handling properly (13.73 KB, patch)
2013-09-09 11:42 UTC, Giovanni Campagna
needs-work Details | Review
wayland: implement global and window keybindings (9.71 KB, patch)
2013-09-09 11:43 UTC, Giovanni Campagna
needs-work Details | Review
wayland: reimplement keyboard state handling properly (15.68 KB, patch)
2013-09-09 14:43 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-08-28 09:19:48 UTC
Synthetize XInput events from ClutterEvents in MetaWaylandKeyboard,
and pass them to the keybindings infrastructure for early handling,
so that we can activate them even if the currently focused window
is not an X11 one (or if there is no focused window, or we're
modal)
Comment 1 Giovanni Campagna 2013-08-28 09:19:51 UTC
Created attachment 253361 [details] [review]
wayland: implement global and window keybindings
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-08-28 14:00:51 UTC
Review of attachment 253361 [details] [review]:

"Synthesize"

And why not just port the keybindings infrastructure to just use Clutter events? We need to do that at *some* point.

::: src/core/keybindings.c
@@ +2070,2 @@
+  /* We only ever have one screen */
+  screen = display->screens->data;

We should get rid of this list at some point.

@@ +2208,3 @@
+ out:
+  if (was_current_time)
+    display->current_time = CurrentTime;

What's the point of this current time handling?
Comment 3 Giovanni Campagna 2013-08-30 08:36:17 UTC
(In reply to comment #2)
> Review of attachment 253361 [details] [review]:
> 
> @@ +2208,3 @@
> + out:
> +  if (was_current_time)
> +    display->current_time = CurrentTime;
> 
> What's the point of this current time handling?

It is because I bypassed meta_display_handle_xevent and went straight to keybinding handling, to be able to pass a MetaWindow.
But that also is problematic, in that I lose the grab_op handling...
Comment 4 Giovanni Campagna 2013-09-04 13:18:09 UTC
Created attachment 254068 [details] [review]
wayland: reimplement keyboard state handling properly

We can't rely on clutter's xkb_state, because that's updated
when events are pulled from the kernel, not when we see them.
Instead, use the new clutter API to get the full modifier state
from the event (which, as a side effect, also works when clutter
is using the X11 backend for running nested).

Ok, with this (that relies on all patches from clutter bug 706494)
I can finally use Alt-Tab properly, as the device state reported
by get_pointer() is correct, the event state is correct, and
clients don't get confused by clutter batching events.
Comment 5 Giovanni Campagna 2013-09-09 11:42:50 UTC
Created attachment 254472 [details] [review]
wayland: reimplement keyboard state handling properly

We can't rely on clutter's xkb_state, because that's updated
when events are pulled from the kernel, not when we see them.
Instead, use the new clutter API to get the full modifier state
from the event (which, as a side effect, also works when clutter
is using the X11 backend for running nested).
Comment 6 Giovanni Campagna 2013-09-09 11:43:06 UTC
Created attachment 254473 [details] [review]
wayland: implement global and window keybindings

Synthetize XInput events from ClutterEvents in MetaWaylandKeyboard,
and pass them to the keybindings infrastructure for early handling,
so that we can activate them even if the currently focused window
is not an X11 one (or if there is no focused window, or we're
modal)
Comment 7 Giovanni Campagna 2013-09-09 11:43:59 UTC
I swapped the two patches, and I'd like to have the first one reviewed ASAP, because ebassi wants me to remove the get_keyboard_state() API before the stable release.
Comment 8 Jasper St. Pierre (not reading bugmail) 2013-09-09 14:32:21 UTC
Review of attachment 254472 [details] [review]:

You seem to have a bad rebase here. You change the API to add a "handled" return value, but never update the any of your calls to use it.

Just focus on removing xkb_state.

::: src/wayland/meta-wayland-keyboard.c
@@ +453,3 @@
+      !clutter_input_device_keycode_to_evdev (event->device,
+					      xkb_keycode, &evdev_code))
+    evdev_code = xkb_keycode - 8; /* What everyone is doing in practice... */

You can rely on this.
Comment 9 Giovanni Campagna 2013-09-09 14:43:53 UTC
Created attachment 254491 [details] [review]
wayland: reimplement keyboard state handling properly

We can't rely on clutter's xkb_state, because that's updated
when events are pulled from the kernel, not when we see them.
Instead, use the new clutter API to get the full modifier state
from the event (which, as a side effect, also works when clutter
is using the X11 backend for running nested).

No, proper keyboard state handling also implies blocking the events
after we sent them to wayland clients.
Comment 10 Jasper St. Pierre (not reading bugmail) 2013-09-09 15:41:03 UTC
Review of attachment 254491 [details] [review]:

OK.

::: src/wayland/meta-wayland-keyboard.c
@@ +604,3 @@
+	  keyboard->grab->interface->key (keyboard->grab,
+					  timestamp,
+					  *k, 0);

Any reason for the braces?

@@ +649,3 @@
+	  keyboard->grab->interface->key (keyboard->grab,
+					  timestamp,
+					  *k, 1);

Any reason for the braces?
Comment 11 Giovanni Campagna 2013-09-09 16:00:52 UTC
Comment on attachment 254491 [details] [review]
wayland: reimplement keyboard state handling properly

Attachment 254491 [details] pushed as 450afba - wayland: reimplement keyboard state handling properly
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-09-10 13:57:14 UTC
Review of attachment 254473 [details] [review]:

You should know how I feel about this one. You said bpeel was working on replacing it, right?
Comment 13 Giovanni Campagna 2013-09-10 14:31:46 UTC
Yes, it's in the wip/wayland-clutter-events branch.
Comment 14 Giovanni Campagna 2013-09-11 09:15:32 UTC
(In reply to comment #13)
> Yes, it's in the wip/wayland-clutter-events branch.

Also, well, the keybindings infrastructure is very much X based. It would require an API change and a lot of churn to convert to clutter events, so it's not feasible for 3.10 at this point (hard code freeze in four days), but we definitely need keybindings in wayland, at least for VT switching.
Comment 15 Jasper St. Pierre (not reading bugmail) 2013-11-15 02:14:13 UTC
this has been fixed