GNOME Bugzilla – Bug 748526
wayland: cannot set Super in keyboard shortcuts
Last modified: 2016-04-20 04:17:11 UTC
In F22/rawhide setting Super+key as a keyboard shortcut is broken, see https://bugzilla.redhat.com/show_bug.cgi?id=1213113 It is not one, but two bugs. The first is that the Super key press is not even sent to the client (g-c-c). It is handled as a keybinding with positive result, so it is not passed to wayland and send to the client. The other is that under wayland the virtual modifiers got not resolved. Under X, when user pressed (in may case) Mod4 modifier key,it got translated to Super modifier and client got Super modifier. Under wayland it just gets Mod4. Mod4 is masked away in gtk by default, so g-c-c under wayland gets no modifier as input.
The gtk+ part of fix is here https://bugzilla.gnome.org/show_bug.cgi?id=748904
Created attachment 302878 [details] [review] send Super key only after we know what is the desired action
Review of attachment 302878 [details] [review]: ::: src/core/events.c @@ +178,3 @@ + /* send modifiers and handle the event as if it were just + * received */ + compositor->seat->keyboard.mods_changed = 1; Why is this needed? As far as I can tell, no matter what the keybinding code does, meta_wayland_compositor_update will always update the modifier state according with the Super key, so mods_changed should already he 1. Hmm. Also, the meta_wayland_compositor_handle_event below will eat up the modifiers_changed that was a side effect of the meta_wayland_compositor_update call in the beginning of meta_display_handle_event, meaning if we press Super then Shift, the shift modifier will be sending 1: key(Super), 2: mod(Super|Shift), 3: key(Shift) instead of 1: key(Super): 2: mod(Super): 3: key(Shift), 4: mod(Super|Shift). ::: src/core/keybindings.c @@ +1726,3 @@ + clutter_event_free (keys->overlay_pending_event); + + keys->overlay_pending_event = NULL; The content of this function can be replaced with: g_clear_pointer (&keys->overlay_pending_event, clutter_event_free);
(In reply to Jonas Ådahl from comment #3) > Review of attachment 302878 [details] [review] [review]: > > ::: src/core/events.c > @@ +178,3 @@ > + /* send modifiers and handle the event as if it were just > + * received */ > + compositor->seat->keyboard.mods_changed = 1; > > Why is this needed? As far as I can tell, no matter what the keybinding code > does, meta_wayland_compositor_update will always update the modifier state > according with the Super key, so mods_changed should already he 1. It was needed because the key that followed Super reseted the mods_changed (the mods_changed is not accumulated, it is set after each key stroke). In the new version of patch it is not needed, though. > > Hmm. Also, the meta_wayland_compositor_handle_event below will eat up the > modifiers_changed that was a side effect of the > meta_wayland_compositor_update call in the beginning of > meta_display_handle_event, meaning if we press Super then Shift, the shift > modifier will be sending 1: key(Super), 2: mod(Super|Shift), 3: key(Shift) > instead of 1: key(Super): 2: mod(Super): 3: key(Shift), 4: mod(Super|Shift).> true > ::: src/core/keybindings.c > @@ +1726,3 @@ > + clutter_event_free (keys->overlay_pending_event); > + > + keys->overlay_pending_event = NULL; > > The content of this function can be replaced with: > > g_clear_pointer (&keys->overlay_pending_event, clutter_event_free); nice, forgot about this func :)
Created attachment 303021 [details] [review] send Super key only after we know what is the desired action
Review of attachment 303021 [details] [review]: ::: src/core/events.c @@ +334,2 @@ { + compositor = meta_wayland_compositor_get_default (); Should put the declaration of compositor her as well, since its invalid in any other path. @@ +339,3 @@ + + /* update compositor state (i. e. modifiers) */ + meta_wayland_compositor_update (compositor, event); I don't think this is Okay, since AFAIK XKB (which is called indirectly by meta_wayland_compositor_update ()) needs to get sane input. Here we will potentially bypass this path (bypass_wayland == TRUE, which is set when keyboard shortcut, gastures, grabs, etc are handled), which means it wouldn't get sane input any more. I believe that was the reason why it was originally unconditionally called before.
Review of attachment 303021 [details] [review]: ::: src/core/events.c @@ +339,3 @@ + + /* update compositor state (i. e. modifiers) */ + meta_wayland_compositor_update (compositor, event); Yeah, we can't have update(); in the !bypass path. update needs to be called unconditionally.
> I don't think this is Okay, since AFAIK XKB (which is called indirectly by > meta_wayland_compositor_update ()) needs to get sane input. I think XKB would be ok in this case, since it is used only with wayland and when we bypass wayland, that it should not cause problem to bypass XKB too. But maybe there's something that makes this assumption not hold... Anyway, it could hurt pointer events and window managing, because in update is called repick, so the focus might get broken. So here comes the new version of the patch (patches in this case)
Created attachment 303758 [details] [review] Allow compositor to mask modifiers
Created attachment 303759 [details] [review] send overlay key only after we know what is the desired action
*** Bug 757468 has been marked as a duplicate of this bug. ***
Created attachment 314755 [details] [review] keybindings: Keep virtual modifier masks around Besides the resolved real modifier masks, having the virtual masks around will be useful too. -- The discussion on the gtk+ side (bug 748904) seems to have settled on virtual modifiers being sent from the compositor. These patches allow us to do that.
Created attachment 314756 [details] [review] wayland-keyboard: Include virtual modifiers along with real modifiers The wayland protocol has enough space to send both virtual and real modifiers on modifiers events which saves clients the work of resolving virtual modifiers themselves.
Created attachment 314757 [details] [review] wayland-keyboard: Notify clients of pending modifier state changes If we get a key event but still have pending modifier state changes we need to send a modifiers event right away so that the key event can be interpreted by clients correctly modified. This case could happen when mutter/gnome-shell itself consumes the modifier key press event such as with the overview key which by default is triggered on super press.
Looks OK to me. I don't really think #2 is necessary per se, but I guess it can't really hurt.
Seems working fine (Tried only the new patches, I guess the old two are made obsolete by these?)
Doesn't look fine for me (i.e. still not working) on Fedora 24 Alpha with Gtk+ 3.20.0 on Gnome+Wayland.
*** Bug 765068 has been marked as a duplicate of this bug. ***
*** Bug 764424 has been marked as a duplicate of this bug. ***
Review of attachment 314755 [details] [review]: Looks good to me.
Review of attachment 314756 [details] [review]: Looks good to me.
Review of attachment 314757 [details] [review]: I guess this means we won't send the key event that triggered the modifier change then? Is that intentional? Because as far as I can tell even with this patch we'll send the Super release event, but without the Super pressed event. I guess it might be out of scope for this patch, which only aims to make sure modifier state is correct, but how do you imagine we'd fix the key pressed/released asymmetry? Anyhow, with a nit, this patch looks good as in it properly sends the correct modifier state. ::: src/wayland/meta-wayland-keyboard.c @@ +489,3 @@ + if (keyboard->mods_changed) + notify_modifiers (keyboard); Wouldn't hurt with a comment here why we need to send updated modifier state *before* updating the mods_changed state.
(In reply to Jonas Ådahl from comment #22) > I guess this means we won't send the key event that triggered the modifier > change then? Is that intentional? Because as far as I can tell even with > this patch we'll send the Super release event, but without the Super pressed > event. > > I guess it might be out of scope for this patch, which only aims to make > sure modifier state is correct, but how do you imagine we'd fix the key > pressed/released asymmetry? Why is the asymmetry bad? On X that's exactly what clients see today, i.e. only the release event, and everything seems to cope well enough, i.e. ignore it. xkbcommon seems to handle it well enough too.
Added the following comment: /* If we get a key event but still have pending modifier state * changes from a previous event that didn't get cleared, we need to * send that state right away so that the new key event can be * interpreted by clients correctly modified. */ Attachment 314755 [details] pushed as 82a247c - keybindings: Keep virtual modifier masks around Attachment 314756 [details] pushed as 0fa9751 - wayland-keyboard: Include virtual modifiers along with real modifiers Attachment 314757 [details] pushed as e284370 - wayland-keyboard: Notify clients of pending modifier state changes
(In reply to Rui Matos from comment #23) > (In reply to Jonas Ådahl from comment #22) > > I guess this means we won't send the key event that triggered the modifier > > change then? Is that intentional? Because as far as I can tell even with > > this patch we'll send the Super release event, but without the Super pressed > > event. > > > > I guess it might be out of scope for this patch, which only aims to make > > sure modifier state is correct, but how do you imagine we'd fix the key > > pressed/released asymmetry? > > Why is the asymmetry bad? On X that's exactly what clients see today, i.e. > only the release event, and everything seems to cope well enough, i.e. > ignore it. xkbcommon seems to handle it well enough too. AFAIK it can mess up the client side xkbcommon state some how. I suppose its not visible now, since we'd send the modifier state anyway, and the button we don't send all the events for is just used as a modifier. Doesn't seem like the "correct" thing anyway.