xrdp
xrdp copied to clipboard
Restructure keymap files
Based on #3022
Most of our users are using Linux, and hence 'evdev' key mappings. These key mappings are more comprehensive than the 'base' key mappings, in that several media keys are supported. This is why #3022 was raised.
During the course of #3022. a number of issues with the existing keymaps were identified:-
- The code to map the RDP scancodes received from the client to X11 keycodes is in three places, namely
xrdp/lang.c
,genkeymap/*.c
and xorgxrdp itself. This makes a wholesale change to evdev challenging and tricky to test. - The keymaps aren't all auto-generated, and some are out-of-date (see this comment)
- XKB changes over time, and the old way of supporting NumLock by pretending it's a shift isn't supported in newer releases (see this comment)
This PR attempts to address these deficiencies as follows:-
- There's now a single module to map RDP scancodes to evdev (or base) key codes in
common/scancode.c
. - The keymap file format has been updated to be indexed by RDP scancode rather than base key code. This should make it easier to triage user problems, as scancodes are readily viewable on (e.g.) https://kbdlayout.info/, and also removes the need to know the X11 keycode for the login screen or the VNC back-end.
- Keymap files now contain a short
[numlock]
section - The X11 keycode to be used by xorgxrdp is now sent directly to xorgxrdp as part of a KEY_UP / KEY_DOWN message in place of the Unicode field. This should allow for some simplification on the keyboard driver in
xorgxrdp
.
This is where I think we should be going after merging #3022, but I'd appreciate some constructive criticism around this, particularly from @sasha0552 and @jsorg71
I think it's ready for updating the xorgxrdp interface. The only possible change in this area is to pass the rules over from xrdp in the same way as we're passing the model and variant.
Next I'm going to look at getting manpages done for xrdp-genkeymap and the keymap files themselves, so I won't be messing much with the functionality.
I've updated a few things to make this more platform independent. The changes are all around the genkeymap utility.
The scancode module now understands both base/xfree86 and evdev keycode mappings - it's no longer a compile-time option. This is used by the genkeymap utility to select the correct table at runtime. It's similar to what it was doing before, but the tables are separate from each other within the scancode module. This should aid maintainability and make it possible to add other tables if we ever need to.
I've verified manually that the only differences between keymap files generated using 'evdev' rules and 'base' rules are the scancode mappings for extra keys. This is what we'd expect with the keymap tables now being based on RDP scancodes.
There's now a separate manpage documenting the key mapping file format, and the manpage for xrdp-genkeymap has been updated.
Comment still very welcome. If I get nothing by around the middle of next week I'll merge this, and we can get on with updating and (hopefully) simplifying xorgxrdp.
One last change; I'm quite keen on checking that we can reproduce keymap files, having gotten into a state where these are not maintainable. I've added code to the xrdp-genkeymap
command to query the X server for the setxkbmap
settings in effect when the keymap file was built, and I've added these to the top of the file as a comment.
For example, the top of instfiles/km-00000411.ini
now looks like this:-
# Created by xrdp-genkeymap V0.10.80
# Key code set: evdev+aliases(qwerty)
# setxkbmap -rules evdev -model pc105 -layout jp -variant OADG109A
# Description: ja-JP
# Operating system: Ubuntu 22.04.4 LTS
[General]
version=2
[noshift]
01="65307:U+001B" # Escape
. . .
This querying is now done within the xrdp-genkeymap
command itself, rather than being part of the dump-keymaps.sh
script. This should allow us to check that any keymaps submitted by users who maybe haven't used the dump-keymaps.sh
script are reproducible.
This change introduces a new dependency on the libxkbfile-dev
(or equivalent) package so we gain access to the same APIs as setxkbmap
uses.
@matt335672
I'm late to join this discussion, what about switching to TOML as I suggested #2659 before? I postponed merging #2659 because it breaks compatibility. This is also a breaking change so I think it is a good time to switch from our own ini parser to TOML library.
Actually, I'd used the TOML parser in this PR anyway as the format was changing!
I'd forgotten #2659 was in this area. I'll have a look and figure out the best way to get both PRs merged.
Ah, I only have a quick look at this so I didn't notice TOML is used since keymap files have .ini
suffix. That sounds good then.
I prefer .toml
suffix to indicate that keymap file is parsed by TOML v1.0.0 compliant parser.
Since I'm breaking the format anyway, it's probably a good idea to rename these as .toml
. That way any customisations made by users to old files won't be wiped out. I'll do that.
Having reviewed #2659, I think the following should be added to this PR from it:-
- Rename keyboard files to
.toml
from.ini
. - Add a test case for loading a keyboard file. The original purpose of the test in #2659 was to check the ini and TOML parsers returned the same result. The new test case can't fall back on an ini file to test, but at the very least it gives us a way to check memory is being allocated and de-allocated properly which IIRC is part of the test added for #3065.
@metalefty - I think I've addressed your latest comments. The files are all *.toml
now but I need to check manpages and a couple of other things before I'm happy with it, and can start merging.
The unit test I pulled in from #2659 needed a bit of re-writing to be useful but then it found an uninitialised memory error in the keyboard map loading logic. So I'm grateful to you for pointing it out to me.
I'm planning on merging #3022 as-is so @sasha0552 gets credit, and then I'll rebase this PR on latest devel. I can fix the CI problem then.
LGTM
Thanks.
I've updated NEWS with suitable notices as this is a backwards-incompatible change. I'll pop something on gitter too.