osu-framework
osu-framework copied to clipboard
Fix keyboard shortcuts not working as expected on non-QWERTY keyboard
- Closes https://github.com/ppy/osu/issues/11143
- Closes #5767
- Alternative to / closes #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/StateVelocity)- added with the new
struct KeyboardKey, andchar Characterproperties 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
- added with the new
- Add support for using characters from [1.] in
KeyCombination/InputKey- this is done by adding new
InputKeys: fromKeycodeAtroughKeycodeZ
- this is done by adding new
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
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.
I was working on something else when I noticed ManualInputManager.Keys(PlatformAction) needs consideration in this pull, as it's currently just pressing invalid Keys when InputKey.Scancode* is used. Haven't yet tested.
https://github.com/ppy/osu-framework/blob/75ed421f60228920abd819cebc5982cb7799bbbb/osu.Framework/Testing/Input/ManualInputManager.cs#L107-L120
Hoping we can get this in sometime soon... probably should be prioritised for review when we get around to reviewing more framework / QoL stuff.
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?
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 ofKey.{A-Z}to mean keycode / letter produced
This was the previous way of doing things, but it lead to problems, see the fix: https://github.com/ppy/osu-framework/pull/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 bothosuTK.Input.KeyandInputKey.
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.
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.
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