GNOME Bugzilla – Bug 722847
wayland: Update keyboard state unconditionally
Last modified: 2014-03-18 19:26:00 UTC
See patch
Created attachment 267065 [details] [review] wayland: Update keyboard state unconditionally In particular we need to keep track of xkb state like latched and locked modifiers even if the actual key event is then consumed by a global shortcut or grab and never reaches any wayland client.
Review of attachment 267065 [details] [review]: OK.
Created attachment 271205 [details] [review] wayland-keyboard: Remove unused modifier indexes This was copied from weston where they're used for compositor keybindings. Mutter has its own keybindings code which doesn't need this.
Created attachment 271206 [details] [review] wayland-keyboard: Make sure we send an updated modifiers event Any given clutter event carries the modifier state as it was before it occured but, for the wayland modifiers event, we want the state including the current event. To fix this, we'll keep our xkb_state instance around instead of the serialized mods.
Created attachment 271207 [details] [review] wayland-keyboard: Split out a function to determine the evdev keycode We will need to use this is in another place on the next commit.
Created attachment 271208 [details] [review] wayland: Update keyboard state unconditionally In particular we need to know about all key events to keep the xkb state reliable even if the event is then consumed by a global shortcut or grab and never reaches any wayland client. We also need to keep track of all pressed keys at all times so that we can send an updated set or pressed keys to the focused client when a grab ends. To avoid sending auto repeated key events to clients we can check if the clutter event has the CLUTTER_EVENT_FLAG_SYNTHETIC set. -- After further investigation, it turns out that the patch here wasn't enough. With these patches we're now sending correct modifiers events. By "correct" I mean closer to weston's semantics since this isn't really specified in the protocol.
Review of attachment 271205 [details] [review]: OK.
Review of attachment 271207 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +435,3 @@ if (event->device == NULL || !clutter_input_device_keycode_to_evdev (event->device, xkb_keycode, &evdev_code)) We should probably just drop clutter_input_device_keycode_to_evdev, since we're already relying on the fact that we're getting XKB keycodes back out, no matter the platform.
Review of attachment 271206 [details] [review]: This is sort of weird. Looking at the Clutter code, it should be retrieving the modifiers after the key press. It seems to me like a better way to do this would be to fix Clutter, but what do I know. If we want to go this way, should we revert the Clutter API that Giovanni added? Are we sure that Clutter's and wl_keyboard's xkb_state won't get out of sync? ::: src/wayland/meta-wayland-keyboard.c @@ +375,3 @@ + xkb_state_serialize_mods (state, XKB_STATE_MODS_LATCHED), + xkb_state_serialize_mods (state, XKB_STATE_MODS_LOCKED), + xkb_state_serialize_layout (state, XKB_STATE_LAYOUT_EFFECTIVE)); Would it perhaps make sense to pass an xkb_state through the modifiers vfunc, and have a wrapper method, so we only do this serialization once?
Review of attachment 271208 [details] [review]: Yeah, this is exactly what I mean by our xkb_state and the client's "getting out of sync". Clutter's doing all this cool stuff with the xkb_state, but we're not seeing any of it. It really seems like Clutter events don't really suit our needs, and we're effectively using them at the same level of libinput events. Given that we really also want more control over the pointer, and the ability to stuff more data (like the window) in event structures, maybe inventing our own event structure here is the way to go, where we manually translate from XI2 and libinput ourselves. ::: src/wayland/meta-wayland-seat.c @@ +362,3 @@ +void +meta_wayland_seat_update (MetaWaylandSeat *seat, + const ClutterEvent *event) We should probably have a doc comment explaining the difference between handle_event and update.
Comment on attachment 271205 [details] [review] wayland-keyboard: Remove unused modifier indexes Attachment 271205 [details] pushed as dfc7f72 - wayland-keyboard: Remove unused modifier indexes
Created attachment 272145 [details] [review] wayland-keyboard: Make sure we send an updated modifiers event -- Just rebased on master. (In reply to comment #9) > This is sort of weird. Looking at the Clutter code, it should be > retrieving the modifiers after the key press. It seems to me like a > better way to do this would be to fix Clutter, but what do I know. We already spoke about this on IRC but for the record: clutter events use the X semantics regarding modifiers, i.e. event N carries with it the effective modifiers as of event N-1. "Fixing" clutter means changing behavior for all clutter consumers so I'd rather not do that. > If we want to go this way, should we revert the Clutter API that > Giovanni added? I guess you mean https://git.gnome.org/browse/clutter/commit/?h=clutter-1.18&id=59f1e531f9ac0a3e43a7b1aa80019373cf2ac01c ? It doesn't work for our use case here unfortunately. But it's public clutter API now and it might be useful for someone. > Are we sure that Clutter's and wl_keyboard's xkb_state won't get out > of sync? With this patch only it does get out of sync. But it gets fixed on the other one as you've noticed. > ::: src/wayland/meta-wayland-keyboard.c > @@ +375,3 @@ > + xkb_state_serialize_mods (state, > XKB_STATE_MODS_LATCHED), > + xkb_state_serialize_mods (state, > XKB_STATE_MODS_LOCKED), > + xkb_state_serialize_layout (state, > XKB_STATE_LAYOUT_EFFECTIVE)); > > Would it perhaps make sense to pass an xkb_state through the modifiers vfunc, > and have a wrapper method, so we only do this serialization once? It's only done in two places and it's straightforward so not really worth it IMO.
Created attachment 272147 [details] [review] wayland-keyboard: Split out a function to determine the evdev keycode -- (In reply to comment #8) > We should probably just drop clutter_input_device_keycode_to_evdev, > since we're already relying on the fact that we're getting XKB > keycodes back out, no matter the platform. Ok, so just like this?
Created attachment 272152 [details] [review] wayland: Update keyboard state unconditionally -- (In reply to comment #10) > Yeah, this is exactly what I mean by our xkb_state and the client's > "getting out of sync". Clutter's doing all this cool stuff with the > xkb_state, but we're not seeing any of it. Right. This is because of how clutter event processing works: 1. main loop wakes on hardware (libinput) events; 2. events are read and queued on the stage; 3. back to mainloop; 4. master clock wakes up and processes events queued on 2; 5. our clutter event handler is finally called for each event; This means that when we process a given event we don't know if more hardware events have already been processed and queued. > It really seems like Clutter events don't really suit our needs, *nod* > and we're effectively using them at the same level of libinput > events. Given that we really also want more control over the > pointer, and the ability to stuff more data (like the window) in > event structures, maybe inventing our own event structure here is > the way to go, where we manually translate from XI2 and libinput > ourselves. I agree. Again as already discussed on IRC: to do this means that we need to have clutter in-tree and modify it enough to fake clutter devices so that the clutter event processing inside mutter and gnome-shell still works. > ::: src/wayland/meta-wayland-seat.c > @@ +362,3 @@ > +void > +meta_wayland_seat_update (MetaWaylandSeat *seat, > + const ClutterEvent *event) > We should probably have a doc comment explaining the difference > between handle_event and update. Added.
Review of attachment 272145 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +147,2 @@ keymap_str = xkb_map_get_as_string (xkb_info->keymap); + Don't see a big reason for the newline here. @@ +341,3 @@ + event->hardware_keycode, + event->type == CLUTTER_KEY_PRESS ? + XKB_KEY_DOWN : XKB_KEY_UP); Can you put this all on one line? My eyes tend to read these two lines as two separate parameters.
Review of attachment 272145 [details] [review]: (whoops, meant to mark as ACN. Looks good)
Review of attachment 272147 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +407,3 @@ + guint xkb_keycode; + xkb_keycode = event->hardware_keycode; + return xkb_keycode - 8; /* What everyone is doing in practice... */ Hm. The comment is sort of wrong, but clutter-xkb-utils.c has this code. /* We use a fixed offset of 8 because evdev starts KEY_* numbering from * 0, whereas X11's minimum keycode, for really stupid reasons, is 8. * So the evdev XKB rules are based on the keycodes all being shifted * upwards by 8. */ key += 8; So I'd just say "clutter-xkb-utils.c adds a fixed offset of 8 to go into XKB's range, so we do the reverse here". Obviously, as we talked about on IRC, the protocol semantics are quite broken here. Ideally, we'd add +8 during immediate event translation, and always work in the "keycode space" of the keymap. ( And would you mind just making this just: return event->hardware_keycode - 8; )
Review of attachment 272152 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +404,3 @@ + enum xkb_state_component changed_state; + + update_pressed_keys (keyboard, evdev_code (event), is_press); I don't think we use update_pressed_keys for anything other than autorepeat detection. If we can test for synthetic events that easily, I think we can just flat out remove update_pressed_keys. (And if you do that, can you split it into another patch?) ::: src/wayland/meta-wayland-seat.c @@ +371,3 @@ } +void Hm, I was imagining that we'd document meta_wayland_seat_update and meta_wayland_seat_handle_event.
Created attachment 272301 [details] [review] wayland-keyboard: Split out a function to determine the evdev keycode -- (In reply to comment #17) > So I'd just say "clutter-xkb-utils.c adds a fixed offset of 8 to go > into XKB's range, so we do the reverse here". Added this as a comment instead. > ( And would you mind just making this just: return > event->hardware_keycode - 8; ) Sure.
Created attachment 272302 [details] [review] wayland: Update keyboard state unconditionally -- (In reply to comment #18) > I don't think we use update_pressed_keys for anything other than > autorepeat detection. If we can test for synthetic events that > easily, I think we can just flat out remove update_pressed_keys. As the commit message says, it's used to send the set of pressed keys on wl_keyboard.enter events. > (And if you do that, can you split it into another patch?) I did remove the return value now since that's not used anymore. > Hm, I was imagining that we'd document meta_wayland_seat_update and > meta_wayland_seat_handle_event. Done.
(In reply to comment #15) > Don't see a big reason for the newline here. > Can you put this all on one line? My eyes tend to read these two lines as two > separate parameters. Both fixed locally.
Review of attachment 272301 [details] [review]: OK.
Review of attachment 272302 [details] [review]: Hm, I thought you were going to split the autorepeat cleanup out?
Created attachment 272312 [details] [review] wayland-keyboard: Drop now unused update_pressed_keys() return value -- (In reply to comment #23) > Hm, I thought you were going to split the autorepeat cleanup out? Ok. I guess it's not worth attaching the other patch again as it just doesn't contain this part now.
Attachment 272145 [details] pushed as 5cc6bec - wayland-keyboard: Make sure we send an updated modifiers event Attachment 272301 [details] pushed as 3502cfb - wayland-keyboard: Split out a function to determine the evdev keycode Attachment 272302 [details] pushed as b3364ca - wayland: Update keyboard state unconditionally Also pushed a better version of attachment 272312 [details] [review] from Jasper.