-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add preference to fall back to using keyCode
for keybindings
#8609
Add preference to fall back to using keyCode
for keybindings
#8609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foltik thank you for the contribution, do you mind squashing your commits (into a single one) so we may proceed with the review, logically they should be grouped together.
0f67097
to
5e9518b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foltik please be sure to perform the following updates:
- include a
sign-off
in your commit (reason for the failed eca CI check). - address the build errors by adding
CorePreferences
binding to thekeybinding.spec.ts
file: https://travis-ci.com/github/eclipse-theia/theia/jobs/397423822#L8732-L8734
5e9518b
to
a1463b2
Compare
Signed-off-by: Jack Foltz <jack@foltz.io>
a1463b2
to
db714ab
Compare
@vince-fugnitto I signed off the commit, added the binding to the test, and added an additional test for the new functionality in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foltik sorry for the late review, I must have missed the ping and it was in my backlog of reviews. Code-wise the changes look very good, thank you for taking care of it! 👍
My colleague will now verify the functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it does
By default, Theia interprets all
KeyboardEvent
s by thecode
, property instead ofkey
orkeyCode
. On Mac (#7919) and Linux (#7654), due to not handling specific user keymap customizations, this interpretation can sometimes be incorrect.I added the preference
keyboard.dispatch
toCorePreferences
, which can be eithercode
to use the default handling, orkeyCode
to fall back to using thekeyCode
property ofKeyboardEvent
s. This is the same workaround that is used in VSCode, due to an underlying issue withnative-keymap
as discussed in microsoft/vscode/issues/23991.The main logic where the keyboard event is interpreted resides in
KeyCode.createKeyCode
andKeyCode.toKey(event)
, which are static methods. Because of this I wasn't able to inject theCorePreferences
, so I added a parameter with a default value to the methods instead. I then injectedCorePreferences
intoKeybindingRegistry
, and used thekeyboard.dispatch
preference to pass into the callsite ofKeyCode.createKeyCode
when responding toKeyboardEvent
s.There is additional code in other packages in this repo that would need to be updated to use this new parameter, however I figured I'd just do the basics to start before I get feedback on the approach I took. Maybe it would also make sense to have the key decoding handled by a service in some non static context, such as how VSCode does it? I would also be willing to give that a shot if it seems like the right direction to go.
I am new to both this codebase and TypeScript/Inversify.js, so please let me know if I'm not doing something right or if I should make any changes!
How to test
With
keyboard.dispatch
set tokeyCode
, #7654 and #7919 should no longer be reproducible. I was only able to test #7654 on my machine.Review checklist
Reminder for reviewers