Fix some keyboard shortcuts for layouts with AltGr

lydell commented 6 years ago

In many keyboard layouts, such as German, French and Swedish, some characters have to be typed using the AltGr key. If such a key is the non-first key of a VMP keyboard shortcut sequence, the sequence fails to trigger. This is because Atom receive an 'altgraph' key press inbetween. For example, the shortcut g ~ is seen as g altgraph ~. 'altgraph' seems to be handled just like any other key, so just like g a ~ does not exist, neither does g altgraph ~.

This commit duplicates shortcuts containing problematic AltGr keys. For example, in addition to the g ~ shortcut I've added g altgraph ~.

The keys I did this for are: ~ $ | [ ] { }.

Fixes #661.

lydell commented 6 years ago

This solution works for me, and it was easy to implement.

However, if users add their own shortcuts, they might be confused why some of them don’t work. For example:

  'i space {': 'vim-mode-plus:inner-curly-bracket-allow-forwarding'
  # should be:
  'i space altgraph {': 'vim-mode-plus:inner-curly-bracket-allow-forwarding'

How come 'shift' does not have this problem? If I open the “Key Binding Resolver” in Atom (ctrl+.) and press a shift key, it does say that 'shift' has been pressed. This is true for all modifiers. Does VMP special-case shift somewhere – so we could special-case 'altgraph' too? Or does Atom special-case shift?

It would be awesome if we didn’t have to specify 'altgraph' in our keymaps, but I understand if there’s nothing VMP can do about this.

Ben3eeE commented 6 years ago

@lydell Do you have any keyboard layout packages installed like keyboard-localization? What do you see in the keybinding resolver when typing these keystrokes?

I am interested to fix this issue in Atom but I don't have any problems with the reported commands on a Swedish layout. Atom by default does not resolve any keystroke to altgraph.

lydell commented 6 years ago

I can reproduce it on Ubuntu 17.10 with the standard 'se' keyboard layout in a clean Atom profile with only VMP installed.

❯ atom --version
Atom    : 1.23.3
Electron: 1.6.15
Chrome  : 56.0.2924.87
Node    : 7.4.0

# How I started Atom with a clean profile:
❯ env ATOM_HOME=$HOME/.atom-test atom .

❯ setxkbmap -print
xkb_keymap {
    xkb_keycodes  { include "evdev+aliases(qwerty)" };
    xkb_types     { include "complete"  };
    xkb_compat    { include "complete"  };
    xkb_symbols   { include "pc+se+inet(evdev)+terminate(ctrl_alt_bksp)"    };
    xkb_geometry  { include "pc(pc105)" };
};

But as you can see I haven’t installed Atom 1.24 yet so I guess I should try that...

EDIT: Installed Atom 1.24.0. Made no difference.

Ben3eeE commented 6 years ago

@lydell That's really interesting. What do you see in the keybinding resolver when typing this keystroke?

Ben3eeE commented 6 years ago

I have been testing the commands reported in https://github.com/t9md/atom-vim-mode-plus/issues/661 on Windows and they all worked without this workaround so maybe this issue is platform dependent. I'll check on Linux later to see if I can understand why this is happening.

lydell commented 6 years ago

Here’s what it looks like after pressing va[ with this PR:

key-resolver

Not sure what those ^a and ^v mean.

(The v command doesn’t show since it is a separate command: First v enters visual mode, then a[ is another command inside visual mode, and only the latest command is shown by the key resolver.)

t9md commented 6 years ago

@lydell

^a and is keyup event when you keyup a after keypress(=a). I understand a motivation of this PR. Not sure this is thorough. E.g. % is not included this PR but necessary?

As my understanding root issue is https://github.com/atom/atom-keymap/issues/126. I wonder workarounding issue as client(vmp) side ambiguate original cause.

lydell commented 6 years ago

Not sure this is thorough. E.g. % is not included this PR but necessary?

I think it’s pretty thorough. I know the Swedish layout best, but as far as I know, no layout has % in the AltGr layer. I looked through https://en.wikipedia.org/wiki/AltGr_key a bit to find what symbols are usually in the AltGr layer.

I also searched through the keymap file several times to make sure that I didn’t miss anything.

And even if I missed something I can add it when somebody reports a problem :)

t9md commented 6 years ago

will merge and release this, but this is not ideal fix. The ideal fix is to fix to make atom-keymap treat altgraph as modifier key properly.

Ben3eeE commented 6 years ago

@t9md I will take a look to see if I can fix this issue in atom-keymap. A workaround is good but I agree that it should be fixed in atom-keymap.

t9md commented 6 years ago

released as v1.28.1

lydell commented 6 years ago

Wow, that was fast! Thanks! I wish you the best of luck in fixing this in atom-keymap.

t9md commented 6 years ago

@lydell Can you help me to understand issue correctly?

I want you to do following two things.

I want to get keymap info where you hit this issue. so

paste following code in init.js and restart atom.
execute user:clip-keymap. JSON keymap table is written to clipboard.
Paste JSON info as comment or gist whatever.

function clipKeymap() {
  const disposable = atom.keymaps.addKeystrokeResolver(event => {
    disposable.dispose()
    const keymap = JSON.stringify(event.keymap, null, '  ')
    atom.clipboard.write(keymap)
  })
}
atom.commands.add('atom-workspace', {
  'user:clip-keymap': () => clipKeymap(),
})

Let me know if following workaround works for you

downgrade vmp to v1.28.0 (git co v1.28.0 in your forked vmp)
confirm issue reproduceable when you type altgraph require keystroke like i {
paste following code in your init.json and restart atom
let me know if issue is resolved or not.

atom.keymaps.addKeystrokeResolver(({event}) => {
  const {keyCode, key, type} = event
  // console.log(keyCode, key, type); // just for debug

  // On Linux AltGraph is sent as keyCode 225.
  // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
  //
  // When we encounter 225, we want to just ignore to avoid terminate pending state.
  // To do that we treat it as bare `ctrl`. Why? `ctrl` is ignored keystroke by folllwing
  // isBareModifier(keystroke) chek at here
  // https://github.com/atom/atom-keymap/blob/802745d4a2a7740d95c5e3f08e141d15fd349457/src/keymap-manager.coffee#L516
  if (keyCode === 225) {
    // I just want to know this result for AltGraph(keyCode 225) typed in linux.
    console.log(event.getModifierState('AltGraph'));

    return 'ctrl'
  }
})

lydell commented 6 years ago

JSON keymap table

``` { "KeyA": { "unmodified": "a", "withShift": "A" }, "KeyB": { "unmodified": "b", "withShift": "B" }, "KeyC": { "unmodified": "c", "withShift": "C" }, "KeyD": { "unmodified": "d", "withShift": "D" }, "KeyE": { "unmodified": "e", "withShift": "E" }, "KeyF": { "unmodified": "f", "withShift": "F" }, "KeyG": { "unmodified": "g", "withShift": "G" }, "KeyH": { "unmodified": "h", "withShift": "H" }, "KeyI": { "unmodified": "i", "withShift": "I" }, "KeyJ": { "unmodified": "j", "withShift": "J" }, "KeyK": { "unmodified": "k", "withShift": "K" }, "KeyL": { "unmodified": "l", "withShift": "L" }, "KeyM": { "unmodified": "m", "withShift": "M" }, "KeyN": { "unmodified": "n", "withShift": "N" }, "KeyO": { "unmodified": "o", "withShift": "O" }, "KeyP": { "unmodified": "p", "withShift": "P" }, "KeyQ": { "unmodified": "q", "withShift": "Q" }, "KeyR": { "unmodified": "r", "withShift": "R" }, "KeyS": { "unmodified": "s", "withShift": "S" }, "KeyT": { "unmodified": "t", "withShift": "T" }, "KeyU": { "unmodified": "u", "withShift": "U" }, "KeyV": { "unmodified": "v", "withShift": "V" }, "KeyW": { "unmodified": "w", "withShift": "W" }, "KeyX": { "unmodified": "x", "withShift": "X" }, "KeyY": { "unmodified": "y", "withShift": "Y" }, "KeyZ": { "unmodified": "z", "withShift": "Z" }, "Digit1": { "unmodified": "1", "withShift": "!" }, "Digit2": { "unmodified": "2", "withShift": "\"" }, "Digit3": { "unmodified": "3", "withShift": "#" }, "Digit4": { "unmodified": "4", "withShift": "¤" }, "Digit5": { "unmodified": "5", "withShift": "%" }, "Digit6": { "unmodified": "6", "withShift": "&" }, "Digit7": { "unmodified": "7", "withShift": "/" }, "Digit8": { "unmodified": "8", "withShift": "(" }, "Digit9": { "unmodified": "9", "withShift": ")" }, "Digit0": { "unmodified": "0", "withShift": "=" }, "Space": { "unmodified": " ", "withShift": " " }, "Minus": { "unmodified": "+", "withShift": "?" }, "BracketLeft": { "unmodified": "å", "withShift": "Å" }, "Backslash": { "unmodified": "'", "withShift": "*" }, "Semicolon": { "unmodified": "ö", "withShift": "Ö" }, "Quote": { "unmodified": "ä", "withShift": "Ä" }, "Backquote": { "unmodified": "§", "withShift": "½" }, "Comma": { "unmodified": ",", "withShift": ";" }, "Period": { "unmodified": ".", "withShift": ":" }, "Slash": { "unmodified": "-", "withShift": "_" }, "NumpadDivide": { "unmodified": "/", "withShift": "/" }, "NumpadMultiply": { "unmodified": "*", "withShift": "*" }, "NumpadSubtract": { "unmodified": "-", "withShift": "-" }, "NumpadAdd": { "unmodified": "+", "withShift": "+" }, "Numpad1": { "unmodified": null, "withShift": "1" }, "Numpad2": { "unmodified": null, "withShift": "2" }, "Numpad3": { "unmodified": null, "withShift": "3" }, "Numpad4": { "unmodified": null, "withShift": "4" }, "Numpad5": { "unmodified": null, "withShift": "5" }, "Numpad6": { "unmodified": null, "withShift": "6" }, "Numpad7": { "unmodified": null, "withShift": "7" }, "Numpad8": { "unmodified": null, "withShift": "8" }, "Numpad9": { "unmodified": null, "withShift": "9" }, "Numpad0": { "unmodified": null, "withShift": "0" }, "NumpadDecimal": { "unmodified": null, "withShift": "," }, "IntlBackslash": { "unmodified": "<", "withShift": ">" }, "NumpadEqual": { "unmodified": "=", "withShift": "=" }, "NumpadComma": { "unmodified": ".", "withShift": "." }, "NumpadParenLeft": { "unmodified": "(", "withShift": "(" }, "NumpadParenRight": { "unmodified": ")", "withShift": ")" } } ```

The workaround works! :tada:

t9md commented 6 years ago

@lydell Thanks! I too want to know the output of console.log(event.getModifierState('AltGraph')); line in above workaround code.

t9md commented 6 years ago

After I found the workaround works from @lydell 's feedback.

What I can come up with to fix atom/atom-keymap#126 is to let atom-keymap just ignore keyCode 225(used for AltGraph in Linux platform) as like key 229 is ignored currently.

https://github.com/atom/atom-keymap/blob/802745d4a2a7740d95c5e3f08e141d15fd349457/src/keymap-manager.coffee#L505-L510

This ignore is done at very top of keyevent handling, atom-keymap would become never handle keyCode 225 in every platform. I think it's OK since

KeyCode 225 is used for AltGraph on Linux platform only.
I understand AltGraph is always used as modifier, as combination with other key to input alternate key, in other word AltGraph is never be used solely, so ignoring this keycode completely doesn't harm practical usage of AltGraph key.

@lydell, @Ben3eeE How do you see above explanation/idea? I have NOT practical experience using AltGraph key, so please let me correct if my understand above is not correct.

lydell commented 6 years ago

Sorry, I forgot about the console.log. When I push the AltGr key down, false is logged. When I release it, true is logged.

You are correct in your understanding about AltGr. It is only used as a modifier. But people don't think about it as a modifier: There are no keyboard shortcuts like altgr+a. AltGr is only used to access a "third layer" of the keyboard containing extra symbols, such as @ £ $ [ ] { } \ ~ | € (depending on your keyboard layout).

To me it sounds like the correct approach is to ignore KeyCode 225 like you suggest.

t9md commented 6 years ago

Thanks for clarification, it really getting clear. Let me clarify one more thing, AltGr is basically not solely pressed right? I mean

Not: AltGr then otherKey
But: AltGr with otherKey(this is the way how AltGr is used right?)

Ben3eeE commented 6 years ago

@t9md On Windows AltGr is the same as Ctrl+Alt so then it can be used as a single modifier because AltGr+o is the same as Ctrl+Alt+o.

I don't remember 100% how atom-keymap works with Linux in this case. Also keyCode is deprecated and I think we want to avoid using it. Keystroke resolution is done by the KeyboardEvent.key field (And in some few cases KeyboardEvent.code but I want to remove these). I think keyCode is only used in one place because we don't have any alternative.

@lydell Can you paste the result of the following script(Just paste it into the Developer tools console and press AltGraph in Atom) when you press AltGraph on Linux:

document.addEventListener('keydown', e => console.log(e), true)

I opened a PR yesterday that registers AltGraph as a modifier https://github.com/atom/atom-keymap/pull/231. This is because AltGr was a non modifier on Windows because of the Electron upgrade. This may be another way to solve this issue.

So AltGraph is currently causing this issue on Windows in Atom 1.25 and it seems on Linux in earlier versions of Atom. I would like to find a solution for both Windows and Linux :thought_balloon:

lydell commented 6 years ago

@t9md Yes, you never press AltGr alone. You hold it down and press another key while it is down, just like you use the shift key.

@Ben3eeE Thanks, I had forgotten that AltGr is the same as Ctrl+Alt in Windows. That is not the case on Linux, though. Good to keep in mind.

keydown event for AltGr

``` KeyboardEvent altKey : false bubbles : true cancelBubble : false cancelable : true charCode : 0 code : "AltRight" composed : true ctrlKey : false currentTarget : null defaultPrevented : false detail : 0 eventPhase : 0 isComposing : false isTrusted : true key : "AltGraph" keyCode : 225 location : 2 metaKey : false path : Array[10] repeat : false returnValue : true shiftKey : false sourceCapabilities : InputDeviceCapabilities srcElement : atom-pane.pane.active target : atom-pane.pane.active timeStamp : 25630.760000000002 type : "keydown" view : global which : 225 __proto__ : KeyboardEvent ```

t9md commented 6 years ago

Thanks!

Ben3eeE commented 6 years ago

key : "AltGraph"

So this is the same issue that Windows is having but the PR https://github.com/atom/atom-keymap/pull/231 will not work on Linux yet I think but that might be fixable.

Sorry, I forgot about the console.log. When I push the AltGr key down, false is logged. When I release it, true is logged.

This is interesting I would think it's the other way around based on reading the code in atom-keymap. Hmm :thought_balloon: I wonder if there are more issues with AltGraph on Linux or if I am misunderstanding something.

Ben3eeE commented 6 years ago

So the issue is that in keystrokeForKeyboardEvent we get an event that has AltGraph key. When Atom sees this is not a modifier in MODIFIERS it assumes it is a pressed key like the Home key. And because altgraph is not part of the partial match keybinding this will interrupt partial matching. Which is why this PR works as a workaround.

lydell commented 6 years ago

I was also surprised by the order of the false and true logs when I press AltGr, but I double-checked like five times and that is the order they are logged. :)

t9md commented 6 years ago

I was also surprised by the order of the false and true logs when I press AltGr, but I double-checked like five times and that is the order they are logged. :)

Yes, this part is confusing, strange.

t9md commented 6 years ago

@Ben3eeE

For me, after I know that my workaround code in https://github.com/t9md/atom-vim-mode-plus/pull/1031#issuecomment-366532266 works. Problem in linux seems to be very simple and just ignoring AlgGraph key is simple solution.

Like sole ctrl key is ignored because of isBareModifier. This always is happening. For example when you want to type ctrl-cmd-a, you can type cmd then ctrl then a. In 2nd ctrl atom-keymap early return since ctrl is isBareModifier.

Which ever by ignoreing keycode 225 or adding it to ENDS_IN_MODIFIER_REGEX is OK I think.

Ben3eeE commented 6 years ago

@t9md I agree. Ignoring keyCode 225 will also work on Linux.

@lydell @t9md Thank you so much for investigating this :bowing_man: It has been really helpful and it's definitely something that we want to fix in atom-keymap.

t9md / atom-vim-mode-plus

Fix some keyboard shortcuts for layouts with AltGr #1031

I want to get keymap info where you hit this issue. so

Let me know if following workaround works for you