xrdp icon indicating copy to clipboard operation
xrdp copied to clipboard

Restructure keymap files

Open matt335672 opened this issue 10 months ago • 3 comments

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:-

  1. 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.
  2. The keymaps aren't all auto-generated, and some are out-of-date (see this comment)
  3. 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:-

  1. There's now a single module to map RDP scancodes to evdev (or base) key codes in common/scancode.c.
  2. 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.
  3. Keymap files now contain a short [numlock] section
  4. 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

matt335672 avatar Apr 22 '24 14:04 matt335672

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.

matt335672 avatar Apr 23 '24 08:04 matt335672

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.

matt335672 avatar Apr 27 '24 11:04 matt335672

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 avatar May 01 '24 16:05 matt335672

@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.

metalefty avatar May 21 '24 15:05 metalefty

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.

matt335672 avatar May 22 '24 08:05 matt335672

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.

metalefty avatar May 22 '24 09:05 metalefty

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.

matt335672 avatar May 22 '24 11:05 matt335672

Having reviewed #2659, I think the following should be added to this PR from it:-

  1. Rename keyboard files to .toml from .ini.
  2. 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.

matt335672 avatar May 23 '24 13:05 matt335672

@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.

matt335672 avatar May 23 '24 19:05 matt335672

LGTM

metalefty avatar May 24 '24 15:05 metalefty

Thanks.

I've updated NEWS with suitable notices as this is a backwards-incompatible change. I'll pop something on gitter too.

matt335672 avatar May 24 '24 16:05 matt335672