Fix wrong keyboard mappings
Needs ReviewPublic

Authored by mrubli on Nov 3 2018, 6:14 AM.

Details

Summary

Certain key combinations got mapped to the wrong keycode, perhaps most
notably pressing Shift+, to type "<" got mapped to ">". This was caused
by XKeySymToKeycode() not being aware of the current modifier state.

The solution is to use XKB instead and keep track of the pressed keys.

The same bug appeared in TigerVNC's x0vncserver, so this is essentially
a port of that fix:
https://github.com/TigerVNC/tigervnc/issues/491
https://github.com/TigerVNC/tigervnc/commit/ac73038232dd7d6e41006357de2120f580b9f05f

BUG: 391079

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
mrubli requested review of this revision.Nov 3 2018, 6:14 AM
mrubli created this revision.
cfeck added a subscriber: KDE Applications.
cfeck added a subscriber: cfeck.Nov 23 2018, 1:05 AM
cfeck added inline comments.
krfb/events.cpp
44

I suggest to make it a member function of EventData.

114

Space after if.

Also, please use {} braces even for single-line statements after if. Needs to be fixed in multiple places.

cfeck requested changes to this revision.Nov 23 2018, 1:05 AM
This revision now requires changes to proceed.Nov 23 2018, 1:05 AM

Do we need any libs linked for XKB to work?

mrubli updated this revision to Diff 46678.Dec 2 2018, 10:52 AM

Take into account review comments:

  • Make XkbKeysymToKeycode() a static method of EventData
  • Fix whitespace and add braces for one-line ifs

Do we need any libs linked for XKB to work?

Looking at FindX11.cmake the existing X11 dependency in CMakeLists.txt seems to take care of that. It certainly builds and links fine for me when I build the Debian package but if there's a build scenario I've missed that needs an explicit dependency please let me know.

cfeck added inline comments.Dec 18 2018, 9:50 PM
krfb/events.cpp
58

Why static? Doesn't it always need to access the xkb and dpy from the instance?

mrubli updated this revision to Diff 48073.Dec 23 2018, 2:17 PM

Yup, sorry, it made no sense at all for that method to be static. Must have been late that day. Here's an updated patch:

Address review comment:

  • Make XkbKeysymToKeycode() non-static
pino added a subscriber: pino.Dec 23 2018, 2:38 PM

One general note regarding the pressedKeys map: what is the cost of calling XkbKeysymToKeycode for every key pressed? If it is not trivial, maybe it would be worth to not remove a mapping when handleKeyboard() is called with down=false, and thus use pressedKeys as simple ever-growing cache.

krfb/events.cpp
53

most probably this can be a QHash, as there is no requirement to keep the elements sorted by the key

58

this method can be 'const', as it does not change EventData

96–98

this can do return keycode; directly

101–105

the code out of the loop can be a simple return NoSymbol

113

const_iterator + constFind

114

constEnd

119

use insert() directly, which is slightly faster than creating an element and changing it immediately after

123–124

use take(), do avoid doing a lookup twice