Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790

Susko3 · 2023-05-14T09:30:47Z

Closes Default key bindings don't work properly with non-QWERTY layouts osu#11143
Closes InputKey does not use the correct keys when the keyboard layout is not QWERTY #5767
Alternative to / closes Use SDL_Keycodes instead of SDL_Scancodes to map the keys #5774

This PR does two things combined into one PR (to help see the full picture, as it's hopefully not too big):

Add support for propagating SDL_Keycodes trough the input hierarchy (implementation idea taken from MidiEvent/State Velocity)
- added with the new struct KeyboardKey, and char Character properties on events / where needed
- also supported on Android (but Android is werid and already kinda broken on master)
- this is done in a semi-extensive way, to allow adding in shifted/altgr characters if needed
Add support for using characters from [1.] in KeyCombination/InputKey
- this is done by adding new InputKeys: from KeycodeA trough KeycodeZ

While fixing the above issues, this PR also adds new things, mainly in [1.]:

The new KeyboardEvent.Character property would be useful for access keys (old docs, ref for underlining) that are seen in newer osu! figma designs (... underline on Mods, Random, Options, etc).
For access keys, I'd like to have some level of localisation support (and ways to check that the localisations don't have conflicts).

Main considerations when making this PR

Don't break existing behaviour

Consumers using KeyboardEvent will not notice any changes, as the new Character property is added in a way that only augments the existing information and no behaviour is changed.

Existing KeyBindings are not affected, even in cases where the new keycode feature doesn't work.

Simple to use for consumers

KeyboardEvents have minimal API where it makes sense for the newly added char Character property.

Changing existing KeyBindings to keycode ones is as simple as:

-new KeyBinding(new KeyCombination(InputKey.Control, InputKey.X), PlatformAction.Cut)
+new KeyBinding(new KeyCombination(InputKey.Control, InputKey.KeycodeX), PlatformAction.Cut)

Only cover the most important use cases and don't have API footguns

Support has been added where it makes sense and where it would be used.
That's why the new keycodes don't work with KeyCombinationMatchingMode.Exact. While it would be possible to support this, there would need to be a way to correlate an InputKey.Keycode{} with an InputKey.{} (like the Character and the Key in a KeyboardKey). This is deemed unnecessary to implement as it's complex and KeyCombinationMatchingMode.Exact isn't used. (This now works properly as a side effect of an unrelated fix.)

Even though KeyboardKey.Character has support for all Unicode BMP characters, InputKey only has support for Keycode[A-Z]. This is because only those keys can reliably be used on all (or most, I cannot check them all) keyboard layouts. Other characters, like punctuation, may not be available. Number keycode are not available as they would be functionally identical to InputKey.Number# + InputKey.Keypad#.

It would be nice to have support for plus and minus keys, but SDL doesn't work like that. Windows does have virtual keys for those (VK_OEM_PLUS, VK_OEM_MINUS), but SDL does not map them in this way (VK_OEM_PLUS would be SDL_SCANCODE_EQUALS, SDLK_EQUALS on en_us keyboard).

Stuff to test and check

Check that the keys on macOS / iOS map to the expected actions when using different keyboard layouts - that it matches other native programs

MatthieuHernandez

I tested your branch and found a bug.
When I press A or any other key it is counted as 2 times. (only happens with letters)
Example below:

Susko3 · 2023-05-21T19:57:04Z

When I press A or any other key it is counted as 2 times. (only happens with letters)

That's to be expected, as both InputKey.A and InputKey.KeycodeA are pressed (they have the same display name as they are the same to the user).

I've updated the tests to show them separately now, hopefully this clears the confusion.

osu.Framework/Input/KeyboardKey.cs

bdach · 2024-03-26T17:20:37Z

osu.Framework/Input/Bindings/InputKey.cs

+        /// <summary>
+        /// Key that produces the 'a' character when pressed. Depends on the current keyboard layout.
+        /// </summary>
+        /// <remarks>Different from <see cref="A"/>.</remarks>
+        KeycodeA = 10752 + 1,


I feel like this is missing two details in this xmldoc:

first, that there might be no physical key that produces this InputKey on the current keyboard layout,

what InputKey.A really means now (because this says that they're "different" now, but doesn't say how) - the best approximation of which would be "the key on the user's keyboard that is closest to the position of the A key on a QWERTY US keyboard", I guess?

first, that there might be no physical key that produces this InputKey on the current keyboard layout

I thought that all keyboard layouts would have ascii alpha keys, and any non-Latin languages (eg. Japanese) would have a standard US keyboard and have an IME do all the work. Turns out some keyboard layouts don't have all the ascii, but windows lies to SDL about what the characters are, so it works out fine. (Typing in a textbox is not affected by this lie.)

Take Azerbaijani (Standard), which has some A-Z characters. Windows lies to SDL (in hopes of compatibility I guess) and overlays the A-Z of the US layout over the actual keyboard. This works in our favour and ensures all InputKey.Keycode* keys are accessible.

(Notice how the Z keycode is pressed twice, since the US A-Z doesn't overlap the original Z.)

what InputKey.A really means now (because this says that they're "different" now, but doesn't say how) - the best approximation of which would be "the key on the user's keyboard that is closest to the position of the A key on a QWERTY US keyboard", I guess?

Maybe better to have "Not to be confused with A"?

I'll expand the xmldoc of InputKey.[A-Z] to mention something like

/// <summary> /// The physical key corresponding to the A key on a US QWERTY keyboard. /// </summary> /// <remarks>Not to be confused with <see cref="KeycodeA"/></remarks> A = 83,

I'll expand the xmldoc of InputKey.[A-Z] to mention something like

Doesn't look to have been applied yet.

Also I'm not sure what to think about the naming here. Keycode is ambiguous to me. I'd probably ideally see KeyA for the "scancode" key and CharacterA for the "keycode" key, but that's very breaking.....

osu.Framework/Input/Events/KeyboardEvent.cs

osu.Framework/Input/States/KeyboardState.cs

bdach · 2024-03-26T17:27:56Z

osu.Framework/Input/States/KeyboardState.cs

+        /// Mapping of <see cref="Key"/>s to <see cref="KeyboardKey.Character"/>s printed on them.
+        /// </summary>
+        /// <seealso cref="KeyboardKey"/>
+        internal readonly Dictionary<Key, char> Characters = new Dictionary<Key, char>();


What is responsible for cleaning up potentially-stale key-to-character mappings after a key is released? Does anything even do that?

Or, to reword the question: what's the guarantee that this mapping is fully correct at any given moment? Because that's what I would expect from KeyboardState.

This mapping may contain stale values and those are not cleaned up, but it's guaranteed to have valid values for currently pressed keys. This mapping is deliberately made internal as it's an implementation detail.

The public methods/properties of KeyboardState guarantee correctness as they access the mapping via PressedKeys.

Usage of the mapping outside KeyboardState requires more scrutiny. I'll make some tests for that, especially for PassThroughInputManager.

bdach · 2024-03-26T17:30:33Z

osu.Framework/Platform/SDL2/SDL2Extensions.cs

+                case SDL.SDL_Keycode.SDLK_RETURN: // SDLK_RETURN is '\r'
+                    return '\n';


I'm not even sure how to react to this. So is it \r or \n? Does anything even produce \n if not SDLK_RETURN? Does this mean there's a possibility that two \ns can be emitted? If yes then in what circumstances?

SDLK_RETURN is defined in SDL as \r. It makes more sense to have it map to \n as that is the ubiquitous newline character (eg. \n makes line breaks in TextFlowContainer).

It makes more sense to have it map to \n

I'd say whether it "makes more sense" depends on the answers to my other questions, namely

Does anything even produce \n if not SDLK_RETURN? Does this mean there's a possibility that two \ns can be emitted? If yes then in what circumstances?

which I don't see the answer to here.

It's possible for two physical keys to produce the same keycode. Eg. regular enter and numpad enter.

I've added a .Distinct() to cover that in KeyboardState, but it's not a silver bullet.

osu-framework/osu.Framework/Input/States/KeyboardState.cs

Line 32 in 54b49e1

public IEnumerable<char> PressedCharacters => PressedKeys.Select(key => Characters[key]).Where(c => c != null).Select(c => c!.Value).Distinct();

Two keys having the same keycode means that KeyBindingContainer.pressedInputKeys can get desynchronised from KeyboardState.PressedCharacters, but that whole logic is pending changes. I'll add a failing test for that.

bdach · 2024-03-26T17:33:17Z

osu.Framework/Platform/SDL2/SDL2Extensions.cs

+
+                case SDL.SDL_Keycode.SDLK_KP_PERIOD:
+                    // potentially return ',' as decimal separator based on keyboard layout.
+                    return numLockOn ? '.' : '\x7f'; // char for delete


Why would this method produce a control character at all anywhere? By that logic, where's U+0008 (backspace)? U+001B (escape)?

Why would this method produce a control character at all anywhere?

SDL already produces control characters, so I made this to match. I don't see the problem with providing control characters.

By that logic, where's U+0008 (backspace)? U+001B (escape)?

This is already handled by SDL. You can check by pressing those keys in TestSceneInputManager.

Can we just not handle control characters at all?

bdach · 2024-03-26T17:37:19Z

osu.Framework/Input/Bindings/KeyCombination.cs

+                        // in exact matching mode, every pressed key except the "virtual" keycode keys needs to be in the candidate.
+                        if (!IsKeycode(key) && !ContainsKeyPermissive(candidateKey, key))


An explanation of the implications of this PR on the exact matching mode is required somewhere because there is none in the OP and I'm not sure what they are myself at this point.

Thinking more about the relationship between physical keys and "virtual keys" (char?/Keycode) in our case. It seems that the relationship between a physical key and its keycode is similar to eg. InputKey.{L,R}Control and InputKey.Control. Same as joystick buttons - there's Joystick1, Joystick2 and also the virtual ABXY face buttons (joystick buttons are out of scope).

If the key combination is InputKey.Control, InputKey.Number9 and the user presses InputKey.LControl, InputKey.RControl, InputKey.Number9 - that is an exact match.

Maybe it's wrong to have the virtual keys present in the "keys pressed by user" array, and it's the job of ContainsKey and ContainsKeyPermissive to unmap any InputKey.Keycode* found in the candidate combination.

I'll have to think more about this.

What's the status on this?

bdach · 2024-03-26T17:39:53Z

osu.Framework/Extensions/ExtensionMethods.cs

+        /// </summary>
+        /// <param name="character"></param>
+        /// <returns><c>string</c> with form of <c>'\0'</c>, <c>'a'</c>, <c>'\x7f'</c>, etc.</returns>
+        public static string StringRepresentation(this char character)


What even is this method? Why does it exist?

Mostly for debugging, to have control characters be readable in text (eg. input event logging, TestSceneInputManager). The o!f font rendered would just displays them as ? otherwise.

Mentioned in ppy#5790 (comment)

Possible failure cases: - `KeyCombination` contains a physical key and its mapping, eg. LControl-Control-Z - keyboard handler reports a dubious combination, like (LControl, KeyCodeA) - only relevant after ppy#5790 makes it in This is bad because a single key should only match to one `InputKey` in the `KeyCombination`.

Doesn't compile

This broke the Ctrl, Shift and Alt visualiser, if releasing one physical key when the opposite hand-side was pressed.

bdach · 2024-05-24T09:09:22Z

osu.Framework/Extensions/InputKeyExtensions.cs

+        /// Gets the <see cref="Key"/> equavalent to this <see cref="InputKey"/>.
+        /// </summary>
+        /// <remarks>Inverse of <see cref="KeyCombination.FromKey"/>.</remarks>
+        public static bool IsKeyboardKey(this InputKey inputKey, out Key key)


This method is all sorts of confusing now.

called IsKeyboardKey (water wet anyone?)

has a weird out parameter that nobody really knows what it's doing

Does this method even do anything at this point? Its only caller is KeyCombination.GetVirtualKey(), which - as far as I can tell -

ensures that the incoming key is physical, so the first conditional is dead code

handles modifiers before the switch in this method, so the entire switch is dead code

depending on the range check this either returns the key but pushed through cast, or Key.Unknown

Can this just be inlined at its only usage?

bdach · 2024-05-24T09:11:33Z

osu.Framework/Input/Bindings/KeyCombination.cs

@@ -135,9 +141,11 @@ public bool IsPressed(KeyCombination pressedKeys, InputState inputState, KeyComb
        /// <param name="candidateKeyBinding">The candidate key binding to match against.</param>
        /// <param name="pressedPhysicalKeys">The keys which have been pressed by a user.</param>
        /// <param name="matchingMode">The matching mode to be used when checking.</param>
+        /// <param name="keyboardCharacters"><see cref="States.KeyboardState.Characters"/> of the current <see cref="InputState"/>.</param>


Param needs to be named better. Something like keyToChararacterMapping or whatever.

bdach · 2024-05-24T09:13:25Z

osu.Framework/Input/States/KeyboardState.cs

@@ -9,6 +11,26 @@ public class KeyboardState
    {
        public readonly ButtonStates<Key> Keys = new ButtonStates<Key>();

+        /// <summary>
+        /// Mapping of <see cref="Key"/>s to <see cref="KeyboardKey.Character"/>s printed on them.


Suggested change

/// Mapping of <see cref="Key"/>s to <see cref="KeyboardKey.Character"/>s printed on them.

/// Mapping of <see cref="Key"/>s to <see cref="KeyboardKey.Character"/>s which are produced when the key is pressed.

bdach · 2024-05-24T09:13:44Z

osu.Framework/Input/States/KeyboardState.cs

+        public bool IsPressed(Key key) => Keys.IsPressed(key);
+
+        /// <summary>
+        /// Whether a specific <see cref="KeyboardKey.Character"/> is pressed.


Suggested change

/// Whether a specific <see cref="KeyboardKey.Character"/> is pressed.

/// Whether a key that produces the specific <paramref name="character"/> is pressed.

bdach · 2024-05-24T09:14:05Z

osu.Framework/Input/States/KeyboardState.cs

+        internal readonly Dictionary<Key, char?> Characters = new Dictionary<Key, char?>();
+
+        /// <summary>
+        /// Whether a specific <see cref="Key"/> is pressed.


Suggested change

/// Whether a specific <see cref="Key"/> is pressed.

/// Whether a specific <paramref name="key"/> is pressed.

bdach · 2024-05-24T09:16:45Z

osu.Framework/Platform/SDL2/SDL2Extensions.cs

+
+                case SDL.SDL_Keycode.SDLK_KP_PERIOD:
+                    // potentially return ',' as decimal separator based on keyboard layout.
+                    return numLockOn ? '.' : '\x7f'; // char for delete


Can we just not handle control characters at all?

bdach · 2024-05-24T09:21:39Z

Re-reading this diff yet again I am yet again struck by the amount of complexity this introduces to everything.

I am wondering whether we can change direction here and sacrifice backwards compatibility for a simpler diff. I'd begin by not bothering to add all of that Keycode{A-Z} stuff and just changing semantics of Key.{A-Z} to mean keycode / letter produced rather than attempt to handle both keycode-like and scancode-like APIs and arguably failing at both.

@Susko3 did you explore the possibility of doing something like the above? @smoogipoo would you be fine with such a hard break?

Susko3 · 2024-05-27T13:33:19Z

I am wondering whether we can change direction here and sacrifice backwards compatibility for a simpler diff. I'd begin by not bothering to add all of that Keycode{A-Z} stuff and just changing semantics of Key.{A-Z} to mean keycode / letter produced

This was the previous way of doing things, but it lead to problems, see the fix: #3950.

We could explore that option, but there are multiple issues with it.

It would break default osu! bindings that care about the position of keys, and not the character (eg. osu! Z, X on a qwertz keyboard doesn't make sense, mania keybinds)
If we only change Key.{A-Z} to keycodes but keep the rest as scancodes it'll be confusing and will likely lead to conflicts
- if the rest of the keys are updated, then things like Ctrl-+ might not work for all keyboard layouts (even on EN US QWERTY as + is Shift-=)
We would need to make way for all 0x10FFFF (or 0xFFFF if we scope to char) unicode characters in both osuTK.Input.Key and InputKey.

attempt to handle both keycode-like and scancode-like APIs and arguably failing at both

The implementation is complex, but I really tried to make the API understandable and simple to use. Please tell what failures do you see in those.

bdach · 2024-05-27T14:04:17Z

The implementation is complex, but I really tried to make the API understandable and simple to use

My primary issue at this stage wrt API is the InputKey enum. If you ask me it needs a sharp naming break. InputKey.[A-Z] should become InputKey.Scancode[A-Z] to really convey the differentiation. Or this. But I don't know if everyone else is willing to stomach that.

There's also the part where attempting to handle both keycode and scancode is an inherent increase in complexity. You might say it's unavoidable but it makes it a tough sell in general.

I dunno, I really need more of @ppy/team-client to sound off here. I just want to avoid another pooling situation where we go in a direction that seems logical one step at a time and then wake up a year later having no earthly idea how anything in pooling works anymore.

bdach · 2024-08-07T06:41:11Z

the French guys were on my ass about this issue the entire COE so i'd like to see movement on this. yes there are merge conflicts but i wanna rack people's brains about the general direction first. bump @ppy/team-client

Susko3 added 19 commits September 6, 2022 20:45

Add KeyboardKey and propagate Character trough the input hierarchy

6fb7adc

Update InputManagers with KeyboardKeyInput changes

af66d0f

Add native implementations for KeyboardKey.Character

a28777e

Update tests

0782390

Update benchmarks

7324d3d

Add test for keyboard layout changing during key press

9d50793

Fix numpad keys giving wrong characters when numlock was off

2087808

Merge branch 'master' into KeyboardEvent-Character

d0f002f

Merge branch 'master' into KeyboardEvent-Character

7f8f26d

Add way to use keycodes as InputKeys

f39fe32

Change PlatformKeyBindings to use new Keycode keys

f07a37a

Add xmldoc

b0bff4c

Remove unused method

fc318ae

Inline trivial function

be2d763

Fix wrong xmldoc

03be244

Fix handling when the character changes mid-press

f13d6f4

Misc code style

d9fa4ca

Add tests and xmldoc about non-working feature

fa287ae

Refine xmldoc to add missing information

1fbb979

pull-request-size bot added the size/XL label May 14, 2023

Susko3 added the area:input label May 14, 2023

MatthieuHernandez suggested changes May 21, 2023

View reviewed changes

Susko3 added 2 commits May 21, 2023 21:46

Add HeadlessTest attribute

4fbc7d9

Show keycodes separately from regular InputKeys

71e2231

Merge branch 'master' into InputKey-Character

2616982

bdach mentioned this pull request Jun 12, 2023

Ctrl+o not working with ctrl and caps swapped ppy/osu#23895

Closed

Add tests that show what works and what doesn't

6e4d9be

pull-request-size bot added size/XXL and removed size/XL labels Jun 19, 2023

Susko3 added 2 commits February 1, 2024 01:48

Merge branch 'master' into InputKey-Character

7402c7b

Add comment about why using default is fine

08f82c4

smoogipoo assigned Susko3 Mar 21, 2024

Susko3 added 2 commits March 22, 2024 08:18

Merge branch 'master' into InputKey-Character

51ac0a0

Update tests so they compile

c39c37b

bdach reviewed Mar 26, 2024

View reviewed changes

Susko3 added 4 commits March 27, 2024 08:30

Remove stray #nullable disable

1c88800

Use char? and denote no keycode as null instead of \0

54b49e1

Fix test

9c370bc

Add failing test

b74b208

Mentioned in ppy#5790 (comment)

Susko3 added the blocked label Mar 27, 2024

Susko3 mentioned this pull request Mar 27, 2024

Refactor KeyCombination.ContainsKey() and .ContainsKeyPermissive() for better extensibility #6229

Merged

Susko3 mentioned this pull request May 20, 2024

SDL3: IME regressions ppy/osu#28261

Closed

Susko3 added 10 commits May 23, 2024 19:46

Merge branch 'master' into InputKey-Character

76601d3

Doesn't compile

Copy SDL2 to SDL3 implementation

85229d3

Consider Keycode[A-Z] as virtual InputKeys

e54a409

Update getVirtualKey() to return Keycode[A-Z]

47b0b77

Revert KeyCombination.romKey() to work with regular Keys

d912e12

Show pressed virtual keys in the test

a4a3e7a

Fix pressed keys visualisation not accounting for virtual keys

f8be52c

This broke the Ctrl, Shift and Alt visualiser, if releasing one physical key when the opposite hand-side was pressed.

Update SDLKeyboardTest to check virtual separately

f82b9b7

Update SDLKeyboardTest to SDL3

d32c46e

Fix 'fixme' test

1d35383

bdach reviewed May 24, 2024

View reviewed changes

smoogipoo mentioned this pull request Aug 7, 2024

Update bindings 2 (no merging needed) ppy/SDL3-CS#116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790

Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790

Susko3 commented May 14, 2023 •

edited

Loading

MatthieuHernandez left a comment

Susko3 commented May 21, 2023

bdach Mar 26, 2024

Susko3 Mar 27, 2024

bdach May 24, 2024 •

edited

Loading

bdach Mar 26, 2024 •

edited

Loading

Susko3 Mar 27, 2024

bdach Mar 26, 2024

Susko3 Mar 27, 2024

bdach Mar 27, 2024

Susko3 Mar 27, 2024

bdach Mar 26, 2024

Susko3 Mar 27, 2024

bdach May 24, 2024

bdach Mar 26, 2024

Susko3 Mar 27, 2024

bdach May 24, 2024

bdach Mar 26, 2024

Susko3 Mar 27, 2024

bdach May 24, 2024

bdach May 24, 2024

bdach May 24, 2024

bdach May 24, 2024

bdach May 24, 2024

bdach May 24, 2024

bdach commented May 24, 2024 •

edited

Loading

Susko3 commented May 27, 2024

bdach commented May 27, 2024

bdach commented Aug 7, 2024

		case SDL.SDL_Keycode.SDLK_RETURN: // SDLK_RETURN is '\r'
		return '\n';

		// in exact matching mode, every pressed key except the "virtual" keycode keys needs to be in the candidate.
		if (!IsKeycode(key) && !ContainsKeyPermissive(candidateKey, key))

	/// Mapping of <see cref="Key"/>s to <see cref="KeyboardKey.Character"/>s printed on them.
	/// Mapping of <see cref="Key"/>s to <see cref="KeyboardKey.Character"/>s which are produced when the key is pressed.

	/// Whether a specific <see cref="KeyboardKey.Character"/> is pressed.
	/// Whether a key that produces the specific <paramref name="character"/> is pressed.

	/// Whether a specific <see cref="Key"/> is pressed.
	/// Whether a specific <paramref name="key"/> is pressed.

Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790

Are you sure you want to change the base?

Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790

Conversation

Susko3 commented May 14, 2023 • edited Loading

Main considerations when making this PR

Don't break existing behaviour

Simple to use for consumers

Only cover the most important use cases and don't have API footguns

Stuff to test and check

MatthieuHernandez left a comment

Choose a reason for hiding this comment

Susko3 commented May 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach May 24, 2024 • edited Loading

Choose a reason for hiding this comment

bdach Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented May 24, 2024 • edited Loading

Susko3 commented May 27, 2024

bdach commented May 27, 2024

bdach commented Aug 7, 2024

Susko3 commented May 14, 2023 •

edited

Loading

bdach May 24, 2024 •

edited

Loading

bdach Mar 26, 2024 •

edited

Loading

bdach commented May 24, 2024 •

edited

Loading