Upgrade atom-keymap to fix issues with multiple layouts on Linux #13543

Merged
merged 3 commits into from Jan 9, 2017

Projects

None yet

4 participants

@nathansobo
Contributor

@Ben3eeE, @ungb: @as-cii and I are hoping that this version of atom-keymap fixes #13131 by improving our detection of the native keyboard layout on Gnome systems. Could you two give this a spin on Gnome and see if we're right about that? You should be able to download the builds associated with this branch.

I've decided to hold off on removing the fallback code via atom/atom-keymap#201 in the name of introducing less change. I think if we can reliably detect the layout, that code should be okay to run.

PS: If we're still failing to match any other bundled keystrokes, it might be worth creating an issue summarizing the platform, layout, and keystroke combinations that are still problematic.

@nathansobo nathansobo ⬆️ atom-keymap to fix issues w/ multiple layouts on Linux
aa93643
@Ben3eeE
Member
Ben3eeE commented Jan 4, 2017

Hey, I can test this tomorrow.

Any chance we can get atom/keyboard-layout#24 merged into this branch so we can test those changes by just downloading the builds as well?

@nathansobo
Contributor

@Ben3eeE I forgot about atom/keyboard-layout#24 and didn't realize it hadn't been merged. I'll get that on this branch too. Thanks for the heads-up.

@nathansobo nathansobo ⬆️ atom-keymap
805536e
@nathansobo
Contributor

@Ben3eeE atom/keyboard-layout#24 is now on this branch.

@nathansobo
Contributor
nathansobo commented Jan 5, 2017 edited

Actually, the build is failing on Travis. @as-cii it looks like you did something to address the build failure I'm seeing in the Travis config for keyboard-layout. However, in this case we're actually running tests in Atom itself so it's a bit unclear to me why it is failing. Do we need a foreground window for things to work? If so, I'm wondering if we want to degrade more gracefully in the keyboard-layout module since people may run headless tests in lots of contexts.

@ungb
ungb commented Jan 6, 2017

@nathansobo this does not fix #13131, the altgraph key is still getting resolved to alt when I use it with ctrl, For example if you wanted to type [ in Azerty layout, you have to type altGraph+'

In Azerty layout, When I try ctrl+altGraph+' I get ctrl+alt+'

@as-cii as-cii ⬆️ atom-keymap
63ad40a
@as-cii
Member
as-cii commented Jan 6, 2017 edited

If so, I'm wondering if we want to degrade more gracefully in the keyboard-layout module since people may run headless tests in lots of contexts.

Great point, I changed keyboard-layout to degrade to XLookupString in atom/keyboard-layout#26. I have also bumped atom-keymap's version and used it in this pull request. 👍

@nathansobo
Contributor
nathansobo commented Jan 6, 2017 edited

@ungb I left a comment on that issue. In short I don't have a good idea for a solution in the case of bindings involving characters that can only be reached via alt-graph. I think we could still release these improvements (if they work) and treat that issue separately.

@ungb
ungb commented Jan 6, 2017

@nathansobo Definitely, this does seem to fix the issue here: #13296.

@Ben3eeE
Member
Ben3eeE commented Jan 6, 2017

I have tested this with Swedish layout, Ubuntu 16.04 gnome and found no regressions. I can also confirm that this fixes #13296 and #13131.

It doesn't completely fix #13131 as @ungb pointed out but this change makes it so that users wanting access to these keybindings can rebind them in their own keymap without having it breaking because the order of layouts changed since rebinding so it is a big improvement for that issue.

I still have a few tests that I thought about today that I want to do but don't wait for me if you want to merge this. 🚢

@Ben3eeE
Member
Ben3eeE commented Jan 8, 2017

Additional tests done. No regressions found, just bugs that are also present in Atom 1.13.0-beta10, see below for some testing notes 🚢


I have verified that the same set of default keybindings are not reachable on Swedish layout on both Ubuntu 16.04 and Windows 10.

These following bindings are not reachable on a Swedish layout, on both Windows and Linux, because the keybinding includes a character is accessed by AltGraph to type.

'ctrl-]': 'editor:indent-selected-rows'
'ctrl-[': 'editor:outdent-selected-rows'
'ctrl-alt-[': 'editor:fold-current-row'
'ctrl-alt-]': 'editor:unfold-current-row'
'ctrl-alt-{': 'editor:fold-all'
'ctrl-alt-}': 'editor:unfold-all'
'ctrl-alt-shift-[': 'editor:fold-selection'

I tested rebinding these commands in my own personal keymap to verify that this works:

'atom-workspace atom-text-editor:not([mini])':
  'ctrl-alt-ö': 'editor:fold-current-row'
  'ctrl-alt-ä': 'editor:unfold-current-row'
  'ctrl-å': 'editor:fold-all' # Atom Specific
  'ctrl-alt-å': 'editor:unfold-all' # Atom Specific
  'ctrl-alt-shift-ö': 'editor:fold-selection'

I did not bind editor:indent-selected-rows and editor:outdent-selected-rows because they are available on the tab key. So the above rebind is what is needed for all commands that have default keybindings to be accessible on a Swedish layout.


Something strange is going on with the window:increase-font-size command where I can access it using many different keyboard combinations. This is also the case in Atom 1.13.0-beta10 so unrelated to this PR, just documenting it here for now...

Looking at window:increase-font-size I can access it from the following keyboard shortcuts(On a Swedish layout): ctrl-=, ctrl-?, ctrl-backtick and ctrl-+ and only two of these are bound. I think this can be another case of #10104.

@as-cii
Member
as-cii commented Jan 9, 2017

Thanks for thoroughly testing this, @Ben3eeE and @ungb! 🙇 I'll go ahead and merge this so that we can treat the other issues separately.

@as-cii as-cii merged commit 6296564 into master Jan 9, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@as-cii as-cii deleted the ns-upgrade-keymap branch Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment