Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(x11): Map Shift+keys to the same virtual code #2630

Closed

Conversation

zaidhaan
Copy link

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • [NA] Created or updated an example program if it would help users understand this functionality
  • [NA] Updated feature matrix, if new features were added or implemented

In x11, symbols (particularly number row symbols) are poorly mapped and Shift+[Number] would either not trigger a KeyboardInput event at all, or trigger one with a symbol (e.g. At) as opposed to the non-symbol (Key2).
The approach taken here would map, for instance, XK_2 and XK_at to VirtualKeyCode::Key2

Why was this done?
This approach was taken as opposed to mapping XK_at to VirtualKeyCode::At as it makes more sense to have a virtual_keycode of Key2 with Shift in modifiers active, rather than to have a virtual_keycode of At with a Shift in modifiers which would be nonsensical. This approach was also taken for the macOS implementation in src/platform_impl/macos/event.rs.

Furthermore, Alacritty (a notable user of this library) defines key bindings by specifying a key as Key[Number] as opposed to the symbol name, coupled with any modifiers, as shown here. In other words, as of this moment, none of the Shift+[Number] keys work, even on Alacritty (on x11).

To illustrate the change that this PR makes, here's what this example code would produce before this change.
Run the code and hit 2 followed by Shift+2 (the @ symbol)
Before:

---------------------------
event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 3, state: Pressed, virtual_keycode: Some(Key2), modifiers: (empty) }, is_synthetic: false }
---------------------------
event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: SHIFT }, is_synthetic: false }
---------------------------
event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 3, state: Pressed, virtual_keycode: Some(At), modifiers: SHIFT }, is_synthetic: false }

After:

---------------------------
event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 3, state: Pressed, virtual_keycode: Some(Key2), modifiers: (empty) }, is_synthetic: false }
---------------------------
event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: SHIFT }, is_synthetic: false }
---------------------------
event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 3, state: Pressed, virtual_keycode: Some(Key2), modifiers: SHIFT }, is_synthetic: false }
  • Note the Some(At) vs Some(Key2).
  • Also note that prior to this, pressing Shift + any number other than 2 or 8 (due to only At and Asterisk being uncommented previously) would not even log anything with that code.

This appears to be what was done in the macOS implementation so I believe it's the right approach to take.

Previously, symbols (particularly number row symbols) were poorly mapped
and Shift+[Number] would either not trigger a KeyboardInput event at
all, or trigger one with a symbol (e.g. At) as opposed to the non-symbol
(Key2).

The approach taken here would map, for instance, XK_2 and XK_at to
VirtualKeyCode::Key2, this approach was taken as opposed to mapping
XK_at to VirtualKeyCode::At as it makes more sense to have a
`virtual_keycode` of Key2 with Shift in `modifiers` active, rather than
to have a `virtual_keycode` of At with a Shift in `modifiers` which
would be nonsensical. This approach was also taken for the macOS
implementation in src/platform_impl/macos/event.rs.

Furthermore, Alacritty (a notable dependant of this library) defines
key bindings by specifying a key as `Key[Number]` as opposed to the
symbol name, coupled with any modifiers.
@dhardy
Copy link
Contributor

dhardy commented Jan 13, 2023

scancode: 3

Remains unchanged. But, unless anyone puts in the effort of making a cross-platform API over this based on physical key position (possibly necessary for reliable game key bindings; IIRC the great keyboard events overhaul #1806 / #753 does propose this), it's only real use is to match key release.

virtual_keycode: Some(At), [...] prior to this, pressing Shift + any number other than 2 or 8 (...) would not even log anything

Yes, probably preferable to assign bindings to e.g. Ctrl+Shift+2 than to Ctrl+@. But Qt does not (confirmed by testing a second key binding which produces the @ symbol in my keyboard layout). I'm sure this has also been discussed somewhere in the long keyboard event overhaul discussion.

@zaidhaan
Copy link
Author

Ah, I should have know this couldn't be such an easy fix :) Thanks for the info.

scancode: 3

Remains unchanged.

I'm not too knowledgeable on keyboard input, but is it an issue that the scancode remains the same? What I understand is that the scancode is hardware-dependent, whereas the keycode not and is specified by X11/keysymdef.h.

It looks to me like the scancode shouldn't really be of much of a concern, looking at the code it simply appears to be the raw keycode subtracted by an offset.

The virtual_keycode is assigned based on the keysym which is based on the translating the raw keycode (hardware) to something keysymdef.h is happy with. Does that not take care of the hardware independence? And after we know that any given hardware key code can be translated into something defined independent of hardware (keysymdef.h), wouldn't all that's left to be done be mapping the keysyms to a desired virtual key code? I don't understand how hardware is an issue.

But, unless anyone puts in the effort of making a cross-platform API over this based on physical key position

Just to clarify, do you mean cross-platform as in any hardware on an X11 system or any OS (which sounds like a big feat)? And are you referring to making it such that scancodes are the same on all platforms regardless of hardware?

Yes, probably preferable to assign bindings to e.g. Ctrl+Shift+2 than to Ctrl+@. But Qt does not

But isn't Qt written in C++? Does it somehow make use of winit as a dependency?

I'll try to take a look at those two threads when I'm free. But from your last statement, does it happen to be the case that there are two "binding styles" that need to be supported (with the non-preferred style being used now)?

In any case, I suppose this will take much longer than expected. It's just odd that winit only supports the At and Asterisk symbols on the number row.

@dhardy
Copy link
Contributor

dhardy commented Jan 13, 2023

the scancode is hardware-dependent, whereas the keycode not

You're probably right; it's a while since I looked at this. So the scancode can be ignored.

One proposal is keyboard_types::KeyboardEvent where key represents most input as UTF-8 while code is just an enumeration of they keys on a standard (US Qwerty) board. IIRC the winning Winit proposal was slightly different, but quite similar, also omitting scancode.

Cross-platform usually means across any OS. But that's essentially already done (code field in the above). The actual implementations can be found in some of these PRs: https://github.com/rust-windowing/winit/milestone/3

I don't know exactly what's going on here; I've just vaguely been following. It's possible this PR is useful (since the overhaul PRs haven't been landed yet).

No, Qt doesn't have much to do with Winit. Looking at prior examples can be useful (arguably looking at Windows or OSX would be more useful; Qt was just easier for me to test).

@maroider
Copy link
Member

I think #2662 should address your use-case. If it doesn't, fell free to tell us what could be improved.

@kchibisov
Copy link
Member

Closing given that #2662 merged.

@kchibisov kchibisov closed this May 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants