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

Implemented dynamic keyboard layout detection #5063

Merged
merged 1 commit into from
May 3, 2019
Merged

Conversation

spoenemann
Copy link
Contributor

Fixes #5042.

Layout changes are now logged as INFO. When I open Theia with Firefox on my machine it says:

INFO Detected keyboard layout: US (OSX)

because I have set English as OS language. As soon as I press the Ö key on my keyboard:

INFO Detected keyboard layout: German (OSX)

I switch the system keyboard layout to French and press the same key again:

INFO Detected keyboard layout: French (OSX)

The layout change is even intercepted before a keybinding is executed. For example, pressing ctrl+shift+´ after loading the page, the keyboard layout is immediately switched to German and the translated keybinding for New Terminal (ctrl+`) is executed.

@spoenemann
Copy link
Contributor Author

@AlexTugarev I had to change the fix for #4986. Please verify it still works with your keyboard.

@spoenemann
Copy link
Contributor Author

The next step should be to store the detected layout so the user doesn't start with the default every time she reloads the page. Where should this be stored? LocalStorageService?

@AlexTugarev
Copy link
Contributor

2019-05-02 16 17 20

this breaks the new terminal keybinding ctrl + ` with:
Screen Shot 2019-05-02 at 16 20 43

while letting it work fine for the builtin ISO keeb:
Screen Shot 2019-05-02 at 16 21 08

import { injectable, postConstruct } from 'inversify';
import { isOSX, isWindows } from '../../common/os';
import { injectable, postConstruct, inject } from 'inversify';
import { IKeyboardLayoutInfo } from 'native-keymap';
Copy link
Member

Choose a reason for hiding this comment

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

it's just type right? it's erased from generated js? native-keymap dependency is not installed for the browser target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's just an interface. I reference that from several files. Should we move the type definition to core/src/common to avoid the warning?

Copy link
Member

Choose a reason for hiding this comment

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

i'm fine with any way. Keeping in one place could be better that noone tries to import in other places. Maybe adding a comment would help.

@spoenemann
Copy link
Contributor Author

@AlexTugarev what is your setup? A built-in ISO keyboard (with ` left of Z) and an external US keyboard (with ` left of 1)?

@AlexTugarev
Copy link
Contributor

  • built-in ISO keyboard (with ` left of Z) ✔️
    Screen Shot 2019-05-02 at 16 48 43

  • external US keyboard (with ` left of 1)?
    external US keyboard ✔️
    Screen Shot 2019-05-02 at 16 48 16

    ➡️ in my case it's a custom 60% keeb and ` key is located left to the Z. but you are right, on the default ansi layout it would be left to the 1 key.

@spoenemann
Copy link
Contributor Author

Strange requirements you have :-D

I'll propose a different fix.

Signed-off-by: Miro Spönemann <miro.spoenemann@typefox.io>
@spoenemann
Copy link
Contributor Author

@AlexTugarev done. Please verify again.

I also changed the naming scheme of the static json files with keyboard layouts: instead of mac, linux and win now there is only mac and pc, where the latter is used both for Linux and Windows.

@AlexTugarev
Copy link
Contributor

@spoenemann, this works like a charm!

2019-05-03 11 39 49

@AlexTugarev
Copy link
Contributor

Strange requirements you have :-D

isn't it great to have it tested with a proper setup? :-D
joking aside, I think it's quite important to support this dynamics (and this PR works fine for me at least,) because mech-keys are getting more and more popular among developers.
thanks for solving this, @spoenemann!

@geropl
Copy link
Contributor

geropl commented May 3, 2019

@spoenemann Thank you so very very much! :-D
image

image

@akosyakov
Copy link
Member

@AlexTugarev can you finish the review and merge it if it is ready?

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

That works really smooth!
Tested with US, German, ISO, ansi settings.

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.

Improve automatic keyboard layout detection
4 participants