GNOME Bugzilla – Bug 706963
wayland: implement global and window keybindings
Last modified: 2013-11-15 02:14:13 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)
Created attachment 253361 [details] [review] wayland: implement global and window keybindings
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?
(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...
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.
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).
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)
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.
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.
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.
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 on attachment 254491 [details] [review] wayland: reimplement keyboard state handling properly Attachment 254491 [details] pushed as 450afba - wayland: reimplement keyboard state handling properly
Review of attachment 254473 [details] [review]: You should know how I feel about this one. You said bpeel was working on replacing it, right?
Yes, it's in the wip/wayland-clutter-events branch.
(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.
this has been fixed