Message ID | 20180222070513.8740-6-kraxel@redhat.com |
---|---|
State | New |
Headers | show |
Series | keymap: support kbd layouts with multiple mappings for the same key. | expand |
On Thu, Feb 22, 2018 at 08:05:13AM +0100, Gerd Hoffmann wrote: > Pass the modifier state to the keymap lookup function. In case multiple > keysym -> keycode mappings exist look at the modifier state and prefer > the mapping where the modifier state matches. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > --- > ui/keymaps.h | 3 ++- > ui/curses.c | 3 ++- > ui/keymaps.c | 33 ++++++++++++++++++++++++++++++++- > ui/sdl.c | 6 +++++- > ui/vnc.c | 9 +++++++-- > 5 files changed, 48 insertions(+), 6 deletions(-) [snip] > diff --git a/ui/vnc.c b/ui/vnc.c > index a77b568b57..d19f86c7f4 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -1734,7 +1734,8 @@ static void reset_keys(VncState *vs) > > static void press_key(VncState *vs, int keysym) > { > - int keycode = keysym2scancode(vs->vd->kbd_layout, keysym) & SCANCODE_KEYMASK; > + int keycode = keysym2scancode(vs->vd->kbd_layout, keysym, > + false, false, false) & SCANCODE_KEYMASK; > qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, true); > qemu_input_event_send_key_delay(vs->vd->key_delay_ms); > qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, false); > @@ -1993,6 +1994,9 @@ static const char *code2name(int keycode) > > static void key_event(VncState *vs, int down, uint32_t sym) > { > + bool shift = vs->modifiers_state[0x2a] || vs->modifiers_state[0x36]; > + bool altgr = vs->modifiers_state[0xb8]; > + bool ctrl = vs->modifiers_state[0x1d] || vs->modifiers_state[0x9d]; > int keycode; > int lsym = sym; > > @@ -2000,7 +2004,8 @@ static void key_event(VncState *vs, int down, uint32_t sym) > lsym = lsym - 'A' + 'a'; > } > > - keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF) & SCANCODE_KEYMASK; > + keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF, > + shift, altgr, ctrl) & SCANCODE_KEYMASK; > trace_vnc_key_event_map(down, sym, keycode, code2name(keycode)); > do_key_event(vs, down, keycode, sym); > } It looks like this patch is causing some buggy input handling behaviour with VNC (and probably SDL though I didn't test) Consider the user wants to type a "<" character. There are two valid key sequences for this down 0xffe1 (shift) down 0x3c (dot) up 0x3c (dot) up 0xffe1 (shift) Or down 0xffe1 (shift) down 0x3c (dot) up 0xffe1 (shift) up 0x3c (dot) With the original code, the "dot" character would be intepreted the same way in both sequences. With this patch applied, the "dot", the second sequence is broken. The "dot" character is interpreted with no modifiers on down, and interpreted with the shift modifier on release. This is then causing QEMU to see a different QKeyCode on down vs up. The trace events show the problem: Before this patch, the two sequences show: 18544@1545157940.945319:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift] 18544@1545157941.186608:vnc_key_event_map down 1, sym 0x3c -> keycode 0x56 [less] 18544@1545157941.346898:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less] 18544@1545157941.810041:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift] 18544@1545157943.447085:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift] 18544@1545157943.752193:vnc_key_event_map down 1, sym 0x3c -> keycode 0x56 [less] 18544@1545157943.986145:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift] 18544@1545157944.258757:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less] IOW, in both sequences we have matched pairs of events After this patch the two sequences now show: 19237@1545158356.679095:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift] 19237@1545158356.896528:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma] 19237@1545158356.993672:vnc_key_event_map down 0, sym 0x3c -> keycode 0x33 [comma] 19237@1545158357.233437:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift] 19237@1545158358.726002:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift] 19237@1545158358.999542:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma] 19237@1545158359.201503:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift] 19237@1545158359.435021:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less] Notice how we sent a "down(comma)" and paired it with an "up(less)". This causes the guest to consider the key stuck down, so next time you type a "<" it will get lost. This is easily reproduced with gtk-vnc and the following QEMU command line: ./x86_64-softmmu/qemu-system-x86_64 -vnc :1 -d trace:vnc* I don't see any nice way to fix the problem without having to keep a memory of modifier state with every "down" event, so you can use the same modifier state with the later "up" event to ensure you get a matching key up. This is quite horrible though. I'm more inclined to revert this patch and find another way to fix the original problem which won't require the UI frontends to track modifier state. This problem is reported in https://bugs.launchpad.net/bugs/1738283 https://bugzilla.redhat.com/show_bug.cgi?id=1658676 Regards, Daniel
Hi, > After this patch the two sequences now show: > > 19237@1545158356.679095:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift] > 19237@1545158356.896528:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma] > 19237@1545158356.993672:vnc_key_event_map down 0, sym 0x3c -> keycode 0x33 [comma] > 19237@1545158357.233437:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift] > > 19237@1545158358.726002:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift] > 19237@1545158358.999542:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma] > 19237@1545158359.201503:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift] > 19237@1545158359.435021:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less] > > Notice how we sent a "down(comma)" and paired it with an "up(less)". This > causes the guest to consider the key stuck down, so next time you type > a "<" it will get lost. Ouch. > This is easily reproduced with gtk-vnc and the following QEMU command > line: > > ./x86_64-softmmu/qemu-system-x86_64 -vnc :1 -d trace:vnc* > > > I don't see any nice way to fix the problem without having to keep a > memory of modifier state with every "down" event, so you can use the > same modifier state with the later "up" event to ensure you get a > matching key up. Well, for key-up you can prefer keys in "down" state instead of looking at the modifiers. > This is quite horrible though. I'm more inclined > to revert this patch and find another way to fix the original problem > which won't require the UI frontends to track modifier state. The UIs track modifier state anyway. I fact I have some WIP patches to add a generic keyboard state tracker, so the UIs can use common code instead of having their own state tracking code each. Also to make UI hotkeys configurable, consistent across all UIs we have. Guess I should undust them, at least the state tracking part of it. With that in place we can easily pass the full keyboard state to the keymap code. cheers, Gerd
Hi, > > This is quite horrible though. I'm more inclined > > to revert this patch and find another way to fix the original problem > > which won't require the UI frontends to track modifier state. > > The UIs track modifier state anyway. > > I fact I have some WIP patches to add a generic keyboard state tracker, > so the UIs can use common code instead of having their own state > tracking code each. Also to make UI hotkeys configurable, consistent > across all UIs we have. Guess I should undust them, at least the state > tracking part of it. > > With that in place we can easily pass the full keyboard state to the > keymap code. Tried that: https://git.kraxel.org/cgit/qemu/log/?h=sirius/kbd-state Not polished yet. cheers, Gerd
On Wed, Dec 19, 2018 at 10:42:14AM +0100, Gerd Hoffmann wrote: > Hi, > > > > This is quite horrible though. I'm more inclined > > > to revert this patch and find another way to fix the original problem > > > which won't require the UI frontends to track modifier state. > > > > The UIs track modifier state anyway. > > > > I fact I have some WIP patches to add a generic keyboard state tracker, > > so the UIs can use common code instead of having their own state > > tracking code each. Also to make UI hotkeys configurable, consistent > > across all UIs we have. Guess I should undust them, at least the state > > tracking part of it. > > > > With that in place we can easily pass the full keyboard state to the > > keymap code. > > Tried that: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/kbd-state I've had a look & think that code makes sense & ought to be able to solve this problem. Regards, Daniel
On Wed, 2018-12-19 at 11:17 +0000, Daniel P. Berrangé wrote: > On Wed, Dec 19, 2018 at 10:42:14AM +0100, Gerd Hoffmann wrote: > > Hi, > > > > > > This is quite horrible though. I'm more inclined > > > > to revert this patch and find another way to fix the original problem > > > > which won't require the UI frontends to track modifier state. > > > > > > The UIs track modifier state anyway. > > > > > > I fact I have some WIP patches to add a generic keyboard state tracker, > > > so the UIs can use common code instead of having their own state > > > tracking code each. Also to make UI hotkeys configurable, consistent > > > across all UIs we have. Guess I should undust them, at least the state > > > tracking part of it. > > > > > > With that in place we can easily pass the full keyboard state to the > > > keymap code. > > > > Tried that: > > https://git.kraxel.org/cgit/qemu/log/?h=sirius/kbd-state > > I've had a look & think that code makes sense & ought to be able to > solve this problem. I'll try and find a minute to do a scratch build with this patch (and without my keymap patch) and throw it on openQA staging, since openQA seems to do a great job of testing qemu input code. :P
diff --git a/ui/keymaps.h b/ui/keymaps.h index 17ec03387a..0693588225 100644 --- a/ui/keymaps.h +++ b/ui/keymaps.h @@ -54,7 +54,8 @@ typedef struct kbd_layout_t kbd_layout_t; kbd_layout_t *init_keyboard_layout(const name2keysym_t *table, const char *language); -int keysym2scancode(kbd_layout_t *k, int keysym); +int keysym2scancode(kbd_layout_t *k, int keysym, + bool shift, bool altgr, bool ctrl); int keycode_is_keypad(kbd_layout_t *k, int keycode); int keysym_is_numlock(kbd_layout_t *k, int keysym); diff --git a/ui/curses.c b/ui/curses.c index 479b77bd03..597e47fd4a 100644 --- a/ui/curses.c +++ b/ui/curses.c @@ -271,7 +271,8 @@ static void curses_refresh(DisplayChangeListener *dcl) keysym = chr; } - keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK); + keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK, + false, false, false); if (keycode == 0) continue; diff --git a/ui/keymaps.c b/ui/keymaps.c index 1eba6d7057..43fe604724 100644 --- a/ui/keymaps.c +++ b/ui/keymaps.c @@ -176,8 +176,12 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table, } -int keysym2scancode(kbd_layout_t *k, int keysym) +int keysym2scancode(kbd_layout_t *k, int keysym, + bool shift, bool altgr, bool ctrl) { + static const uint32_t mask = + SCANCODE_SHIFT | SCANCODE_ALTGR | SCANCODE_CTRL; + uint32_t mods, i; struct keysym2code *keysym2code; #ifdef XK_ISO_Left_Tab @@ -193,6 +197,33 @@ int keysym2scancode(kbd_layout_t *k, int keysym) return 0; } + if (keysym2code->count == 1) { + return keysym2code->keycodes[0]; + } + + /* + * We have multiple keysym -> keycode mappings. + * + * Check whenever we find one mapping where the modifier state of + * the mapping matches the current user interface modifier state. + * If so, prefer that one. + */ + mods = 0; + if (shift) { + mods |= SCANCODE_SHIFT; + } + if (altgr) { + mods |= SCANCODE_ALTGR; + } + if (ctrl) { + mods |= SCANCODE_CTRL; + } + + for (i = 0; i < keysym2code->count; i++) { + if ((keysym2code->keycodes[i] & mask) == mods) { + return keysym2code->keycodes[i]; + } + } return keysym2code->keycodes[0]; } diff --git a/ui/sdl.c b/ui/sdl.c index 963cdf77a7..c4ae7ab05d 100644 --- a/ui/sdl.c +++ b/ui/sdl.c @@ -201,6 +201,9 @@ static kbd_layout_t *kbd_layout = NULL; static uint8_t sdl_keyevent_to_keycode_generic(const SDL_KeyboardEvent *ev) { + bool shift = modifiers_state[0x2a] || modifiers_state[0x36]; + bool altgr = modifiers_state[0xb8]; + bool ctrl = modifiers_state[0x1d] || modifiers_state[0x9d]; int keysym; /* workaround for X11+SDL bug with AltGR */ keysym = ev->keysym.sym; @@ -210,7 +213,8 @@ static uint8_t sdl_keyevent_to_keycode_generic(const SDL_KeyboardEvent *ev) if (keysym == 92 && ev->keysym.scancode == 133) { keysym = 0xa5; } - return keysym2scancode(kbd_layout, keysym) & SCANCODE_KEYMASK; + return keysym2scancode(kbd_layout, keysym, + shift, altgr, ctrl) & SCANCODE_KEYMASK; } diff --git a/ui/vnc.c b/ui/vnc.c index a77b568b57..d19f86c7f4 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -1734,7 +1734,8 @@ static void reset_keys(VncState *vs) static void press_key(VncState *vs, int keysym) { - int keycode = keysym2scancode(vs->vd->kbd_layout, keysym) & SCANCODE_KEYMASK; + int keycode = keysym2scancode(vs->vd->kbd_layout, keysym, + false, false, false) & SCANCODE_KEYMASK; qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, true); qemu_input_event_send_key_delay(vs->vd->key_delay_ms); qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, false); @@ -1993,6 +1994,9 @@ static const char *code2name(int keycode) static void key_event(VncState *vs, int down, uint32_t sym) { + bool shift = vs->modifiers_state[0x2a] || vs->modifiers_state[0x36]; + bool altgr = vs->modifiers_state[0xb8]; + bool ctrl = vs->modifiers_state[0x1d] || vs->modifiers_state[0x9d]; int keycode; int lsym = sym; @@ -2000,7 +2004,8 @@ static void key_event(VncState *vs, int down, uint32_t sym) lsym = lsym - 'A' + 'a'; } - keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF) & SCANCODE_KEYMASK; + keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF, + shift, altgr, ctrl) & SCANCODE_KEYMASK; trace_vnc_key_event_map(down, sym, keycode, code2name(keycode)); do_key_event(vs, down, keycode, sym); }