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

New approach for handling keyboard layouts and keyboard events #4724

Merged
merged 4 commits into from
Apr 10, 2019

Conversation

spoenemann
Copy link
Contributor

Closes #4079, closes #1401, closes #3885, maybe also #2200 and #3518 (and possibly more).

I refactored a lot of the existing keybindings related code and introduced two new services:

  • KeyboardLayoutProvider (layout-provider.ts) tries to get the user's keyboard layout in the format defined by the native-keymap package.
  • KeyboardLayoutService (keyboard-layout-service.ts) maps keybindings according to the provided keyboard layout and determines the characters to display in the UI (menus, quick open etc.)

The KeyboardLayoutProvider behaves differently depending on the platform and browser:

  • On Electron I use the native-keymap package in the backend and transfer that information to the frontend via JSON-RPC.
  • On browsers I load static JSON files that have been prepared with the script generate-layout.js, which simply writes the output of native-keymap to a file. I have already created layout files for German, French and US layouts.
  • On Chrome there is partial support for the experimental Keyboard API. I find the user's layout by comparing all keys in the Keyboard API mapping with the statically loaded layouts.
  • On browsers with no support for the Keyboard API, I use navigator.language to guess the keyboard by the user's language setting (this is highly error-prone, of course).

The KeyboardLayoutService behaves differently depending on the platform:

  • On Mac OS and Linux I compute a reverse mapping from reachable characters to the respective KeyCodes, i.e. keys with possible shift and alt modifiers. This reverse mapping is then used to transform keybindings. For example, the keybinding cmd+/ requires the / character, which is reachable via shift+7 on German Mac keyboards. The reverse mapping transforms the keybinding to cmd+shift+7.
  • On Windows every key has a so-called virtual key which is provided by native-keymap. Here the reverse mapping is computed based on the virtual keys. For example, the virtual key identifier for / is VK_OEM_2. On German Mac keyboards, this virtual key is mapped to the key labeled #, so the keybinding ctrl+/ is transformed to ctrl+#.

The platform-specific behavior should be equivalent to what VS Code does. Please tell me if you find keybindings that are still different in VS Code.

There is still a lot of work to be done in order to improve keyboard detection for browsers that don't support the Keyboard API. For example, we could implement some heuristics that detect the keyboard after the user has pressed some keys, or implement a UI for selecting a keyboard. Furthermore, we should add more static layouts. Should we extract the script to a separate repository to make that easier also for external contributors? We don't need to build the whole of Theia just to run a simple NodeJS script.

The native-keymap package has a method to receive notifications when the keyboard layout has changed in the system. Unfortunately, this does not work yet (microsoft/node-native-keymap#10).

I have created a CQ for the new dependency to native-keymap:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19423
I also had to copy the typings from VS Code (native-keymap.d.ts) because they are not included in the package. Do we need another CQ for this?

@benoitf
Copy link
Contributor

benoitf commented Mar 26, 2019

hello, is it planned to be included in the upcoming release ?

@paul-marechal
Copy link
Member

Detail, but the build on Linux now requires additional libraries, see:
https://travis-ci.org/theia-ide/theia/jobs/511526642#L788

It is also not possible to build this branch on Gitpod because of this.

@akosyakov akosyakov requested a review from svenefftinge March 27, 2019 04:57
@akosyakov
Copy link
Member

hello, is it planned to be included in the upcoming release ?

Maybe we better to test it in next for a month before putting in latest? That's a cool change, but we have to verify that there are not surprises. cc @marcdumais-work @svenefftinge

@spoenemann spoenemann force-pushed the msp_keyboardLayout branch 2 times, most recently from ff11c6d to 28864ff Compare March 27, 2019 08:27
@spoenemann
Copy link
Contributor Author

the build on Linux now requires additional libraries, see:
https://travis-ci.org/theia-ide/theia/jobs/511526642#L788

Yes, we need the additional package libxkbfile-dev:

https://github.com/Microsoft/node-native-keymap#installing

I added it to our .travis.yml.

It is also not possible to build this branch on Gitpod because of this.

@svenefftinge should we add a Dockerfile to this repository, or would you rather add libxkbfile-dev to workspace-full?

@spoenemann spoenemann force-pushed the msp_keyboardLayout branch 2 times, most recently from dfcf2fe to 387d167 Compare March 27, 2019 08:48
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

I still need to try more things on Electron, great change!

@spoenemann spoenemann force-pushed the msp_keyboardLayout branch from 387d167 to 84c7660 Compare April 1, 2019 14:02
@spoenemann
Copy link
Contributor Author

spoenemann commented Apr 1, 2019

I restructured some of the code according to microsoft/node-native-keymap#10 (comment). Automatic update of all keybindings after a keyboard layout change works now in Electron.

@akosyakov
Copy link
Member

@spoenemann could you rebase please, i can start reviewing afterwards

@spoenemann spoenemann force-pushed the msp_keyboardLayout branch from 84c7660 to 08f2f7f Compare April 8, 2019 07:14
@spoenemann
Copy link
Contributor Author

Done.

@akosyakov akosyakov force-pushed the msp_keyboardLayout branch from 08f2f7f to 1840786 Compare April 9, 2019 07:15
@akosyakov akosyakov force-pushed the msp_keyboardLayout branch 8 times, most recently from 21f798e to a1f1deb Compare April 9, 2019 09:57
@akosyakov
Copy link
Member

That's a great improvement! Shortcuts work way better now any existing web IDE i've tried!

Testing Electron part...

@spoenemann would be fine if I do one pass and fix naming of files/events to follow ours naming conventions?

@akosyakov
Copy link
Member

I still need to try more things on Electron, great change!

@marechal-p Do you have timeframe on it? I've finished testing Electron now and it looks great, but i'm always glad to more extensive testing :)

I was looking at:

  • monaco chords support
  • refactor/comment line actions for different layouts
  • layouts switching in browser on mac and in Electron on linux
  • terminal still propagates process interruption
  • cleaning terminal with ctrlcmd + l

I will have a look at generation of ru layout for mac and playing with that around a bit.

@paul-marechal
Copy link
Member

@akosyakov to be fair, I don't see much more else to comment about.

When your review comments will be addressed I will be entirely good with these changes!

Even if we happen to see something later, the code looks better now so I expect issues to be trivial.

@akosyakov akosyakov force-pushed the msp_keyboardLayout branch from 562586c to bfc4235 Compare April 10, 2019 05:48
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I've resolved all outstanding issues and rebased.

@marechal-p If you are fine please approve and we can merge it.

@akosyakov akosyakov requested a review from paul-marechal April 10, 2019 12:26
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

@akosyakov just noticed something: on Firefox layout doesn't seem to be automatically recognized.

IMO, we can get away just with a command to pick from the list of layouts:

> (quick pick)
- Linux: English (en)
- Linux: German (de)
- Linux: Français (fr)
- OSX: English (en)
...

This can either be done here, or on a following PR.

@akosyakov
Copy link
Member

akosyakov commented Apr 10, 2019

@marechal-p as far as I know @spoenemann is going to open 2 more issues and work on them:

  • quick pick as you suggested if the layout cannot be auto-recognized
  • perf optimizations to move detection to the backend

But if the layout is not recognized then it works as on master for you. Not unexpected behaviour?

spoenemann and others added 3 commits April 10, 2019 14:55
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov force-pushed the msp_keyboardLayout branch from bfc4235 to db5a826 Compare April 10, 2019 15:02
@paul-marechal
Copy link
Member

@akosyakov I only noticed that using the German keyboard, it is not possible to do ctrl+/ in order to toggle line comments, using [LeftCtrl] + [LeftShift] + [Digit7] or [LeftCtrl] + [Slash].

But it feels like expected behavior, as that will be fixed by being able to select the current layout.

@akosyakov
Copy link
Member

I only noticed that using the German keyboard, it is not possible to do ctrl+/ in order to toggle line comments, using [LeftCtrl] + [LeftShift] + [Digit7] or [LeftCtrl] + [Slash].

@marechal-p the same on master, so it's fine

@paul-marechal
Copy link
Member

Everything is green on my side then :)

Amazing work by the way, I really like these changes!

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.

works nicely with international keyboard layout!

@akosyakov
Copy link
Member

kudos to @spoenemann :)

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@spoenemann
Copy link
Contributor Author

What about my question:

I have created a CQ for the new dependency to native-keymap:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19423
I also had to copy the typings from VS Code (native-keymap.d.ts) because they are not included in the package. Do we need another CQ for this?

@akosyakov
Copy link
Member

@spoenemann yes, we need a CQ for all copied code

@spoenemann
Copy link
Contributor Author

spoenemann commented Apr 12, 2019

https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19529

@marcdumais-work
Copy link
Contributor

Hi @spoenemann ,

The CQ looks good, 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
6 participants