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

Fix some keyboard shortcuts for layouts with AltGr #1031

Merged
merged 1 commit into from
Feb 18, 2018
Merged

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Feb 17, 2018

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.

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 t9md#661.
@lydell
Copy link
Contributor Author

lydell commented Feb 17, 2018

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
Copy link

Ben3eeE commented Feb 17, 2018

@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
Copy link
Contributor Author

lydell commented Feb 17, 2018

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
Copy link

Ben3eeE commented Feb 17, 2018

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

@Ben3eeE
Copy link

Ben3eeE commented Feb 17, 2018

I have been testing the commands reported in #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
Copy link
Contributor Author

lydell commented Feb 17, 2018

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
Copy link
Owner

t9md commented Feb 18, 2018

@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 atom/atom-keymap#126.
I wonder workarounding issue as client(vmp) side ambiguate original cause.

@lydell
Copy link
Contributor Author

lydell commented Feb 18, 2018

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 t9md merged commit 1d15697 into t9md:master Feb 18, 2018
@t9md
Copy link
Owner

t9md commented Feb 18, 2018

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
Copy link

Ben3eeE commented Feb 18, 2018

@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
Copy link
Owner

t9md commented Feb 18, 2018

released as v1.28.1

@lydell lydell deleted the altgraph branch February 18, 2018 11:02
@lydell
Copy link
Contributor Author

lydell commented Feb 18, 2018

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

@t9md
Copy link
Owner

t9md commented Feb 18, 2018

@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

  1. paste following code in init.js and restart atom.
  2. execute user:clip-keymap. JSON keymap table is written to clipboard.
  3. 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

  1. downgrade vmp to v1.28.0 (git co v1.28.0 in your forked vmp)
  2. confirm issue reproduceable when you type altgraph require keystroke like i {
  3. paste following code in your init.json and restart atom
  4. 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
Copy link
Contributor Author

lydell commented Feb 18, 2018

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! 🎉

@t9md
Copy link
Owner

t9md commented Feb 19, 2018

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

@t9md
Copy link
Owner

t9md commented Feb 19, 2018

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
Copy link
Contributor Author

lydell commented Feb 19, 2018

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
Copy link
Owner

t9md commented Feb 19, 2018

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
Copy link

Ben3eeE commented Feb 19, 2018

@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 atom/atom-keymap#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 💭

@lydell
Copy link
Contributor Author

lydell commented Feb 19, 2018

@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
Copy link
Owner

t9md commented Feb 19, 2018

Thanks!

@Ben3eeE
Copy link

Ben3eeE commented Feb 19, 2018

key : "AltGraph"

So this is the same issue that Windows is having but the PR atom/atom-keymap#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 💭 I wonder if there are more issues with AltGraph on Linux or if I am misunderstanding something.

@Ben3eeE
Copy link

Ben3eeE commented Feb 19, 2018

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
Copy link
Contributor Author

lydell commented Feb 19, 2018

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
Copy link
Owner

t9md commented Feb 19, 2018

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
Copy link
Owner

t9md commented Feb 19, 2018

@Ben3eeE

For me, after I know that my workaround code in #1031 (comment) 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
Copy link

Ben3eeE commented Feb 19, 2018

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants