GNOME Bugzilla – Bug 781223
Cannot handle keysyms that resolve to more than one keycode
Last modified: 2017-09-16 12:52:53 UTC
While trying to add support for a new hotkey key (evdev keycode KEY_FAVORITES) on a Lenovo laptop I added the mapping of that keycode to XF86Favorites in "keycodes/evdev" of the xkeyboard-config. Investigation of why the new keybinding didn't work - although the new key properly registered in gnome-control-center - revealed that KEY_BOOKMARKS also already maps to XF86Favorites (and has a lower keycode then KEY_FAVORITES). A look into mutter's source (thanks Benjamin) showed that mutter registers only the first keycode when it does the reverse lookup from keysym to keycode. A short talk to Daniel Stone confirmed that the N (keycodes) : 1 (Keysym) is legal and should be supported. I am happy to provide the patch, if it is agreed upon that this is indeed a bug and should be handled. :)
It indeed sounds like a bug, I'll gladly review a patch :).
Is this just under Wayland, or in X11 as well?
It is a general (Wayland, X11) issue when one keysm maps to more then one keycode; in that case we currently only use (i.e. listen to) the first keycode for events. An example is XF86AudioPlay that maps to <I208> and <I215>. Hope that clarifies it a bit more.
Created attachment 352368 [details] [review] keybindings: handle multiple keycodes for keysym A single keysym can resolve to multiple keycodes. Instead of only using the first one and ignoring the others, we store all codes in MetaResolvedKeyCombo and then handle all of them in keybinding resolution.
Before looking at the code, I have a question: doesn't this increase the likelihood of shortcut conflicts because shortcuts that previously resolved to different first keycodes now might resolve to the same keycode in the "extra" keycodes? These conflicts IIRC, are silently ignored today but I think this potentially increases the problem so they should at least be logged.
Created attachment 352372 [details] [review] keybindings: warn on overwriting a binding If we already have bound a keycode for a keybinding with a specific keysym then this can get overwritten by a new keybinding with a different keysym that resolves to the same keycode. Now that we resolve and bind all keycodes for a keysym this might happen more often; in that case warn but still overwrite, because this should resemble more the old behavior where we only took the first keycode for any keysym.
(In reply to Rui Matos from comment #5) > These conflicts IIRC, are silently ignored today but I think this > potentially increases the problem so they should at least be logged. Fair enough. I added a warning but left the behavior as is; although I am not sure if not bailing out in the case of conflict is the right thing. My thinking was that if one keysym has, lets say two keycodes, we insert them both in the hashtable now, where before we only took the first. Lets call the second keycode K. If now a second keysym also wants to register the this keycode K then it will just overwrite the binding for it (with itself). This is exactly the same situation as if we had never registered K in the first place. Right?
Review of attachment 352368 [details] [review]: Overall makes sense to me, I just found nits with it. Also feel free to squash both patches together. ::: src/core/keybindings.c @@ +53,3 @@ #endif +#include <string.h> We don't need this header after all, do we? @@ +214,3 @@ static guint32 +key_combo_key (MetaResolvedKeyCombo *resolved_combo, + int i) 'i' needs indenting :). @@ +462,1 @@ + keys->iso_next_group_combo[0].mask = ShiftMask; Not a big fan of mixing array nomenclature and pointer arithmetic in this function... @@ +1866,3 @@ { + if (resolved_combo.keycodes[0] == keys->iso_next_group_combo[i].keycodes[j] && + resolved_combo.mask == keys->iso_next_group_combo[i].mask) Given you just use resolved_combo for this, you could have two separate keycode/mask variables as well. It's about the same amount of lines, but easier to read probably.
(In reply to Carlos Garnacho from comment #8) > Review of attachment 352368 [details] [review] [review]: Thanks for the review! > Overall makes sense to me, I just found nits with it. Also feel free to > squash both patches together. Will do! > ::: src/core/keybindings.c > @@ +53,3 @@ > #endif > > +#include <string.h> > > We don't need this header after all, do we? memcpy is in string.h, no? > @@ +214,3 @@ > static guint32 > +key_combo_key (MetaResolvedKeyCombo *resolved_combo, > + int i) > > 'i' needs indenting :). Had it indented, looked funny and other functions don't do it, but will fix it ;) > > @@ +462,1 @@ > + keys->iso_next_group_combo[0].mask = ShiftMask; > > Not a big fan of mixing array nomenclature and pointer arithmetic in this > function... You prefer &array[i], right? Will do that then. > @@ +1866,3 @@ > { > + if (resolved_combo.keycodes[0] == > keys->iso_next_group_combo[i].keycodes[j] && > + resolved_combo.mask == keys->iso_next_group_combo[i].mask) > > Given you just use resolved_combo for this, you could have two separate > keycode/mask variables as well. It's about the same amount of lines, but > easier to read probably. Oh yeah, good idea, like that way better, will do that.
(In reply to Christian Kellner from comment #9) > (In reply to Carlos Garnacho from comment #8) > > Review of attachment 352368 [details] [review] [review] [review]: > Thanks for the review! > > > Overall makes sense to me, I just found nits with it. Also feel free to > > squash both patches together. > Will do! > > > > ::: src/core/keybindings.c > > @@ +53,3 @@ > > #endif > > > > +#include <string.h> > > > > We don't need this header after all, do we? > memcpy is in string.h, no? No memcpy in sight :). You use g_memdup in the patch, but that doesn't require you to manually add the header. > > > @@ +214,3 @@ > > static guint32 > > +key_combo_key (MetaResolvedKeyCombo *resolved_combo, > > + int i) > > > > 'i' needs indenting :). > Had it indented, looked funny and other functions don't do it, but will fix > it ;) As with every old project, there's plenty of places with funky indenting indeed... still better to aim for a consistent one :), thanks. > > > > > @@ +462,1 @@ > > + keys->iso_next_group_combo[0].mask = ShiftMask; > > > > Not a big fan of mixing array nomenclature and pointer arithmetic in this > > function... > You prefer &array[i], right? Will do that then. Yup :).
Created attachment 352576 [details] [review] keybindings: handle multiple keycodes for keysym A single keysym can resolve to multiple keycodes. Instead of only using the first one and ignoring the others, we store all codes in MetaResolvedKeyCombo and then handle all of them in keybinding resolution. If we already have bound a keycode for a keybinding with a specific keysym then this can get overwritten by a new keybinding with a different keysym that resolves to the same keycode. Now that we resolve and bind all keycodes for a keysym this might happen more often; in that case warn but still overwrite, because this should resemble more the old behavior where we only took the first keycode for any keysym.
(In reply to Carlos Garnacho from comment #10) > > > We don't need this header after all, do we? > > memcpy is in string.h, no? > > No memcpy in sight :). You use g_memdup in the patch, but that doesn't > require you to manually add the header. Doh! I must have looked at an outdated patch. All comments addressed, hopefully. I also added another missing resolved_key_combo_free (in meta_display_ungrab_accelerator, L1490) and a newline in the new meta_warn() call.
Comment on attachment 352576 [details] [review] keybindings: handle multiple keycodes for keysym I looks correct to me now. Just a minuscule nit: reading again through the patch, the function resolved_key_combo_free() sounds a bit odd as it doesn't free the struct pointer. Perhaps resolved_key_combo_initialize/reset() is a better name? Feel free to change that in place before pushing. Rui, I marked the patch as a-c-n, but would still appreciate your sanity check :).
Review of attachment 352576 [details] [review]: ::: src/core/keybindings.c @@ +328,3 @@ + + /* duplicate keycode detection */ + for (i = 0; i < keycodes->len; i++) a bit weary of making this code traverse even more elements for each iteration. what's the practical problem of having duplicate keycodes here? @@ +541,3 @@ + } + + g_hash_table_replace (keys->key_bindings_index, to keep the current behavior as much as possible we should only replace if this is the first keycode, i.e. only for i == 0
(In reply to Rui Matos from comment #14) > Review of attachment 352576 [details] [review] [review]: > > ::: src/core/keybindings.c > @@ +328,3 @@ > + > + /* duplicate keycode detection */ > + for (i = 0; i < keycodes->len; i++) > > a bit weary of making this code traverse even more elements for each > iteration. Linear search is incredibly fast (with the prefetcher doing its thing, etc). And for the low number of keycodes we expect I don't think that will be a problem. But I also couldn't resist measuring it, so I added a GTimer and ran it once with (dedups.csv) and once without (dups.csv) de-duplication. kb.py dups.csv keycodes: sum: 1474, avg: 4.59190, max: 12 time: sum: 0.03820, avg: 0.00012 ± 5.805549e-05 kb.py dedup.csv keycodes: sum: 329, avg: 1.02492, max: 2 time: sum: 0.03576, avg: 0.00011 ± 5.312107e-05 Not doing the de-duplication is actually slower; I would guess due to GArray having to reallocate data when expanding the array. > @@ +541,3 @@ > + } > + > + g_hash_table_replace (keys->key_bindings_index, > > to keep the current behavior as much as possible we should only replace if > this is the first keycode, i.e. only for i == 0 Ack. I will update the patch to not replace keycodes for i > 0.
Created attachment 352864 [details] [review] keybindings: handle multiple keycodes for keysym A single keysym can resolve to multiple keycodes. Instead of only using the first one and ignoring the others, we store all codes in MetaResolvedKeyCombo and then handle all of them in keybinding resolution. If we already have bound a keycode for a keybinding with a specific keysym then this can get overwritten by a new keybinding with a different keysym that resolves to the same keycode. Now that we resolve and bind all keycodes for a keysym this might happen more often; in that case warn but still overwrite, but only for the first keycode for each keysym. If a secondary (i.e. all non-first keycodes) is already indexed we just ignore that; this should resemble the old behavior where we only took the first keycode for any keysym as close as possible.
Review of attachment 352864 [details] [review]: ok, great, thanks for measuring! this looks good
Attachment 352864 [details] pushed as 68dacb5 - keybindings: handle multiple keycodes for keysym Thanks everyone for the reviews!
*** Bug 703724 has been marked as a duplicate of this bug. ***
*** Bug 787755 has been marked as a duplicate of this bug. ***