search

microsoft / node-native-keymap

Provide OS keyboard layout functionality as a nodejs module
MIT License
136 stars 37 forks source link

Avoid permanently switching to foreground window keyboard layout on Windows #36

Closed poiru closed 2 years ago

poiru commented 2 years ago

In e1ee48fa20, we started switching to the foreground window keyboard layout when using some of the APIs. This was done to ensure that we'd get the main process keyboard layout (which is what Electron windows would use) even when the APIs are called in the render process.

This works fine if the foreground app is the same as the caller of these APIs, but if the foreground app is something else, we are inadvertently switching to its keyboard layout. This can happen e.g. if some Electron app calls these APIs while it's in the background.

This goes against Windows principles — the user may choose to use different layouts in different apps.

Ideally we should remove this code and recommend that these APIs are only used in the main process, but for now, we can avoid breaking changes by simply switching back to the original layout after the relevant keyboard querying code has executed.

ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

poiru commented 2 years ago

@alexdima Can you approve CI for this PR and check that it passes before merging?

I don't currently have a Windows machine nor VM to test this on, I noticed this while reviewing the code for this module.

alexdima commented 2 years ago

@poiru I've approved the CI, there are compilation errors. Is there a missing public:?

D:\a\node-native-keymap\node-native-keymap\src\keyboard_win.cc(285,66): error C2248: 'vscode_keyboard::UseForegroundKeyboardLayoutScope::UseForegroundKeyboardLayoutScope': cannot access private member declared in class 'vscode_keyboard::UseForegroundKeyboardLayoutScope' [D:\a\node-native-keymap\node-native-keymap\build\keymapping.vcxproj]
D:\a\node-native-keymap\node-native-keymap\src\keyboard_win.cc(265): message : see declaration of 'vscode_keyboard::UseForegroundKeyboardLayoutScope::UseForegroundKeyboardLayoutScope' [D:\a\node-native-keymap\node-native-keymap\build\keymapping.vcxproj]
D:\a\node-native-keymap\node-native-keymap\src\keyboard_win.cc(264): message : see declaration of 'vscode_keyboard::UseForegroundKeyboardLayoutScope' [D:\a\node-native-keymap\node-native-keymap\build\keymapping.vcxproj]
D:\a\node-native-keymap\node-native-keymap\src\keyboard_win.cc(348,66): error C2248: 'vscode_keyboard::UseForegroundKeyboardLayoutScope::UseForegroundKeyboardLayoutScope': cannot access private member declared in class 'vscode_keyboard::UseForegroundKeyboardLayoutScope' [D:\a\node-native-keymap\node-native-keymap\build\keymapping.vcxproj]
D:\a\node-native-keymap\node-native-keymap\src\keyboard_win.cc(265): message : see declaration of 'vscode_keyboard::UseForegroundKeyboardLayoutScope::UseForegroundKeyboardLayoutScope' [D:\a\node-native-keymap\node-native-keymap\build\keymapping.vcxproj]
D:\a\node-native-keymap\node-native-keymap\src\keyboard_win.cc(264): message : see declaration of 'vscode_keyboard::UseForegroundKeyboardLayoutScope' [D:\a\node-native-keymap\node-native-keymap\build\keymapping.vcxproj]
poiru commented 2 years ago

@alexdima Yup, fixed! Can you approve again? Hope it will compile this time!

alexdima commented 2 years ago

Thank you!