osu-framework icon indicating copy to clipboard operation
osu-framework copied to clipboard

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

Open Susko3 opened this issue 2 years ago • 8 comments

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

  1. 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
  2. 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

Susko3 avatar May 14 '23 09:05 Susko3

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.

Susko3 avatar May 21 '23 19:05 Susko3

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

Susko3 avatar Jul 10 '23 19:07 Susko3

Hoping we can get this in sometime soon... probably should be prioritised for review when we get around to reviewing more framework / QoL stuff.

peppy avatar Dec 13 '23 04:12 peppy

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?

bdach avatar May 24 '24 09:05 bdach

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

Susko3 avatar May 27 '24 13:05 Susko3

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 avatar May 27 '24 14:05 bdach

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

bdach avatar Aug 07 '24 06:08 bdach