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

Fix shortcuts not working in non-ASCII keyboard layouts #2587

Merged
merged 3 commits into from
Nov 3, 2021

Conversation

nullst
Copy link
Contributor

@nullst nullst commented Oct 25, 2021

Description:

People who use Russian language (or many other languages with primarily non-ASCII symbols) typically switch between several keyboard layouts throughout the day, for example between English layout and a Russian layout. The same physical key will correspond to both English symbol (e.g., "V") and Russian symbol ("М" for the most common Cyrillic layout; that's not the same symbol as English M, it just looks the same). Normally, people expect the physical key combination corresponding to "Ctrl+V" in English to also work in Russian (otherwise muscle memory goes haywire if you switch between the keyboard layouts often). This is how it usually works in Windows, Mac, and well-developed graphical toolkits on Linux. Note that this is not what people with AZERTY do, since they usually stick to a single keyboard layout and type both French and English with it.

This used not to work in Fyne, and this pull request fixes it: when a user presses an unknown-to-fyne key (like Russian "Ф") with a Control/Alt modifier, the triggersShortcut function will try to replace the unknown key with the equivalent ASCII symbol. I'm not sure if the method I use for translation actually works on all platforms, but at least it seems to work on my Mac OS Sierra.

Fixes #1220 .

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

When user pressed the physical keys corresponding to Ctrl+V while using,
for example, Russian keyboard layout, they usually expect a Paste shortcut
to trigger. This is how Windows, Mac, and good graphics toolkits on Linux
work. In fyne this used to be represented as Ctrl+Unknown key, but now the
non-ascii keys are translated when pressed with Ctrl or Alt modifier.
@andydotxyz
Copy link
Member

I just wanted to check you've read through a previous attempt at fixing this issue #1227.
I think your approach is a good one, but I want to make sure this works for foreign keyboards without breaking on keyboard mappings.
Is it possible to add tests in this place? (I am not sure how to do that to test GLFW keyboards but it's worth checking)

@nullst
Copy link
Contributor Author

nullst commented Oct 25, 2021

I've seen #1227, I think it tries to modify too much. With the solution proposed here nothing is done if fyne can recognize the key that user pressed. So nothing would change for people who use mostly ASCII-based symbols (with QWERTY/AZERTY/whatever key layout). This is a narrow modification that only attempts to double guess if user pressed something that looks like a shortcut, but the key is not recognized by fyne.

Is it possible to add tests in this place? (I am not sure how to do that to test GLFW keyboards but it's worth checking)

I wondered about this, but I have no idea how to it. The glfw.Key and the scancode are the same when I press a physical-V key whether I'm currently using Russian or English layout. It's the function glfw.GetKeyName that performs a magical conversion that depends on the global OS state, and it just calls the corresponding C function. So I can't make a test without modifying whatever global state the function glfwGetKeyName coming from C is reading.

@nullst
Copy link
Contributor Author

nullst commented Oct 25, 2021

I've looked at the C source of the glfwGetKeyName function and it does not seem possible to hook into the mechanism that translates the same (key code, scan code) pair into different symbols depending on the currently chosen keyboard layout. At least, the X11 implementation just asks xkb directly.

@andydotxyz
Copy link
Member

OK thanks for the feedback. Let's see if we can get some other international keyboard users to review this and confirm the fix. Would @toaster or @MagicalTux be able to comment perhaps?

@Jacalz
Copy link
Member

Jacalz commented Oct 26, 2021

I don’t think German or Swedish keyboards were affected by this issue before, so I’m unfortunately not sure if me or @toaster can help much with testing.

@andydotxyz
Copy link
Member

I don’t think German or Swedish keyboards were affected by this issue before, so I’m unfortunately not sure if me or @toaster can help much with testing.

OK, just thought I would check various angles as I'm not sure where the keyboard complications creep in ;)

@nullst
Copy link
Contributor Author

nullst commented Oct 26, 2021

I did some research and found a complication: unlike standard Russian layout, many keyboard layouts, like US International layout, French layout, or German layout, produce special characters when some keys are pressed in combination with an AltGr key (which I think depending on OS/hardware is just reported by glfw as Alt+Ctrl or maybe sometimes as just Alt).

So, for example, in US International layout on a Mac "Alt+a" produces a symbol "å". Windows and Linux have their own versions of US international layout, and I got the impression that they may have different sets of key combinations. But let's assume we are on a Mac for now. Now what happens if an application has a shortcut for the key combination "Alt+A"? There are basically three design options:

  1. Every time a user with US international layout will try to type "å" symbol by pressing Alt+A, the shortcut will be triggered (in addition to the symbol å being reported to TypedRune).
  2. Users with US international layout are unable to use this shortcut since "Alt+A" is already used by the input method.
  3. Since the options above are not ideal, the application should not be permitted to ever register Alt+A as a shortcut as this clashes with internationalization.

Option 2 seems much better than the others. However, my patch attempts to translate a shortcut key if any modifiers are present, so it will follow option 1. Thus I probably should restrict the translation to the case when only Ctrl/Cmd are pressed.

(Fun fact: if user presses Ctrl+Alt+A, glfw would report it as "Ctrl+Alt+A" regardless of the keyboard layout, not as "Ctrl+å". You may think this is a good idea, but some keyboard layouts put key symbols like '[' behind an Alt+something combination. Then if you register an "Ctrl+[" shortcut, a user with such a layout will never be able to use it since the only way to type '[' for them is with an Alt-combination. See relevant upstream issue or a discussion in Qt docs that directly says that developers should try to stick to a list of "standard" keyboard shortcuts maintained by Qt.)

@andydotxyz
Copy link
Member

1 is not workable because we can't (or shouldn't) emit a key and a shortcut event for the same combination, if that is what the PR does then it needs to be fixed I think.
It may help that (for the most part) plain alt or super key combinations are usually reserved for the OS/window manager - in apps Alt is either for menu navigation or as an additional modifier alongside ctrl.

@nullst
Copy link
Contributor Author

nullst commented Oct 26, 2021

Restricting my patch to apply to key combinations where only Control/Command modifiers are applied would avoid the option 1. Or at least it avoids it assuming that there do not exist keyboard layouts which define combinations like "Ctrl+U" to produce a printable character (I originally thought this to be the case for Alt key as well, which was very foolish of me). So far I haven't seen any indications that such layouts may exist.

In some keyboard layouts pressing Alt+A results in a printable
but non-ASCII character. This should not be interpreted as an
attempt to invoke an Alt+A shortcut since in terms of glfw this
is a "char event", not "key event".
@andydotxyz
Copy link
Member

Yes we may have to filter out alt-only. Even on my en-GB mac keyboard all the keys display other characters with Alt held down.

@toaster
Copy link
Member

toaster commented Oct 27, 2021

Personally, I’d go for a variant of option 3:

3. Since the options above are not ideal, the application should not be permitted to ever register Alt+A as a shortcut as this clashes with internationalization.

Instead of preventing Alt+<Key> we should instead enforcing a keyboard shortcut to have at least one of Ctrl or Super modifier.
Technically, that would mean that we don’t trigger shortcuts unless at least Ctrl or Super are present.

@nullst
Copy link
Contributor Author

nullst commented Oct 27, 2021

I've changed the detection logic as discussed above: it will apply if either Control or Super modifier is pressed.

The question of "should a shortcut Alt+A be allowed to be registered" is out of scope for this pull request. Personally I think this avoiding shortcuts like that should be strongly recommended in the documentation, but not forbidden in code.

There may exist a case that's handled slightly incorrectly, but I think it's acceptable. Suppose a user has a keyboard layout which (1) features all symbols from latin alphabet; (2) maps the key that would be Z in the US layout to a symbol that's not known to glfw (as in, glfw.GetKeyName returns KeyUnknown). I don't know if such layouts exist. Then the translation logic will trigger if the user presses "Ctrl+(key-that-was-Z-in-the-US-layout)" and consider this as an attempt to perform Ctrl+Z shortcut. That's not too bad -- just an alternative way to trigger the same shortcut, pressing Ctrl+(what-is-Z-in-the-current-layout) would also work. We don't have a choice anyway: it would be best to only attempt the key translation if we know for sure that user's current layout has no way to produce ASCII latin alphabet symbols, but there is no way to do this with glfw.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks.
Are you happy too @toaster ?

Copy link
Member

@toaster toaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks good!

I’ve also gained some knowledge (by starting my Windows machine again ;)): Alt+<Key> is a valid shortcut on Windows and I think it’s not that uncommon there. Therefore, it definitely would be a bad decision to forbid it. However, a dev using it has to be aware that it breaks things in one way ore another on Mac at least.

@andydotxyz andydotxyz merged commit 36a8002 into fyne-io:develop Nov 3, 2021
@andydotxyz
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants