Skip to content

Commit

Permalink
fix(osx/onivim#2924): Modifier keys not working with Romanji keyboard (
Browse files Browse the repository at this point in the history
…onivim#2938)

__Issue:__ On OSX, if the Romanji keyboard is selected, keys with modifiers (`Cmd+P`, etc) would not be processed correctly.

__Defect:__ This was a regression from onivim#2902 - in that change, we rely on querying our `keyboard-layout` library for keymap information. However, we weren't able to get keymap information for Japanese / virtual keyboard layouts.

__Fix:__ Turns out a similar issue was encountered in the `node-native-keymap` library here: microsoft/node-native-keymap@f735d67

Ported over a similar fix for our `keyboard-layout` strategy - first, try `TISCopyCurrentKeyboardInputSource`, and if that returns `NULL`, try `TISCopyCurrentKeyboardLayoutInputSource`.

Fixes onivim#2924
  • Loading branch information
bryphe authored Jan 6, 2021
1 parent 0deaed9 commit d4e21ff
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGES_CURRENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
- #2628 - Input: Right arrow key treated as PageUp
- #2929 - Input: Fix intermittent crash when scrolling with the mouse (fixes #2919)
- #2927 - Input: Windows - fix crash in entering Unicode character (fixes #2926)
- #2938 - Input: OSX - Modifier keys not working on Romanji keyboard (fixes #2924)

### Performance

Expand Down
7 changes: 7 additions & 0 deletions integration_test/lib/Oni_IntegrationTestLib.re
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ let runTest =

Vim.init();
Oni2_KeyboardLayout.init();
Log.infof(m =>
m(
"Keyboard Language: %s Layout: %s",
Oni2_KeyboardLayout.getCurrentLanguage(),
Oni2_KeyboardLayout.getCurrentLayout(),
)
);

let initialBuffer = {
let Vim.BufferMetadata.{id, version, filePath, modified, _} =
Expand Down
17 changes: 17 additions & 0 deletions manual_test/cases.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,20 @@ __Pass:__
- [ ] Win
- [ ] OSX
- [ ] Linux

## 7.2 Japanese / Romanji layout

Regression test for #2924

Prerequisite:
- Install Romanji keyboard layout

- Switch keyboard layout to Romanji
- Run Onivim 2
- Verify can open quickopen menu (Command+P/Control+P)
- Verify can enter insert mode and type text

__Pass:__
- [ ] Win
- [ ] OSX
- [ ] Linux
8 changes: 8 additions & 0 deletions src/bin_editor/Oni2_editor.re
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,14 @@ switch (eff) {
Oni2_KeyboardLayout.init();
Oni2_Sparkle.init();

Log.infof(m =>
m(
"Keyboard Language: %s Layout: %s",
Oni2_KeyboardLayout.getCurrentLanguage(),
Oni2_KeyboardLayout.getCurrentLayout(),
)
);

// Grab initial working directory prior to trying to set it -
// in some cases, a directory that does not have permissions may be persisted (ie #2742)
let initialWorkingDirectory = Sys.getcwd();
Expand Down
21 changes: 19 additions & 2 deletions src/oni2-keyboard-layout/stubs/keyboard-layout-linux.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ CAMLprim value oni2_KeyboardLayoutInit() {

void oni2_priv_GetCurrentKeyboardLayout(char *layout) {
XkbRF_VarDefsRec vdr;
char *tmp;
if (XkbRF_GetNamesProp(xDisplay, &tmp, &vdr) && vdr.layout) {
if (XkbRF_GetNamesProp(xDisplay, NULL, &vdr) && vdr.layout) {
XkbStateRec xkbState;
XkbGetState(xDisplay, XkbUseCoreKbd, &xkbState);

Expand All @@ -113,6 +112,24 @@ void oni2_priv_GetCurrentKeyboardLayout(char *layout) {
} else {
sprintf(layout, "%s[%d]", vdr.layout, xkbState.group);
}

// TODO: Is there an actual unitializer for XkbRF_VarDefsRec?
// Looks like we have to manually free this fields...
if (vdr.variant) {
free(vdr.variant);
}

if (vdr.layout) {
free(vdr.layout);
}

if (vdr.options) {
free(vdr.options);
}

if (vdr.model) {
free(vdr.model);
}
} else {
layout[0] = '\0';
}
Expand Down
11 changes: 9 additions & 2 deletions src/oni2-keyboard-layout/stubs/keyboard-layout-mac.c
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,16 @@ CAMLprim value oni2_KeyboardLayoutPopulateCurrentKeymap(value keymap, value Hash

TISInputSourceRef source = TISCopyCurrentKeyboardInputSource();
CFDataRef layoutData = (CFDataRef)TISGetInputSourceProperty(source, kTISPropertyUnicodeKeyLayoutData);

if (layoutData == NULL) {
CAMLreturn(Val_unit);
// TISGetInputSourceProperty returns null with Japanese keyboard layout.
// Using TISCopyCurrentKeyboardLayoutInputSource to fix NULL return.
// From: https://github.com/microsoft/node-native-keymap/blob/308e260b26d7f6ab8b20380c3d6dd26d86a570e5/src/keyboard_mac.mm#L88
source = TISCopyCurrentKeyboardLayoutInputSource();
layoutData = (CFDataRef)((TISGetInputSourceProperty(source, kTISPropertyUnicodeKeyLayoutData)));

if (layoutData == NULL) {
CAMLreturn(Val_unit);
}
}

const UCKeyboardLayout *keyboardLayout = (UCKeyboardLayout *)CFDataGetBytePtr(layoutData);
Expand Down

0 comments on commit d4e21ff

Please sign in to comment.