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

Keyboard shortcut prefix fix for Windows, Linux (Ctrl+Shift). Fixes #1069. #1191

Closed
wants to merge 4 commits into from
Closed

Conversation

nbolten
Copy link

@nbolten nbolten commented Dec 14, 2016

This pull request fixes #1069 (and related issues like #1100).

Overview

Interactive command line applications commonly bind to Ctrl+<key> keyboard shortcuts. For example, nano uses Ctrl+(G, O, W, K, J, C, X, R, \, U, T, _, V) for essential functionality like reading and writing files and exiting the program. Other applications are similar, like tmux and screen, relying on keyboard shortcuts to manage the program's basic functionality.

This isn't an issue when using hyper on Macs, as the modifier prefix for interacting with the hyper windows is Cmd. On non-Macs (like Windows and Linux machines), the default prefix is Ctrl, which causes many conflicts.

This pull request proposes to address these conflicts by setting the default prefix on non-Macs to Ctrl+Shift, which is common for terminal emulators in linux. The Super key has been suggested as an alternative, but this would conflict with several shortcuts built into the Windows OS as well as many linux-focused desktop environment like Unity and Gnome. To avoid conflicts with defaults that already used Ctrl+Shift, some shortcuts were changed to (hopefully) reasonable alternatives. This pull request does not change any shortcuts on Macs.

Summary of changes

  1. Default shortcut prefix on non-Macs changed from Ctrl to Ctrl+Shift.

  2. Some shortcuts that already used Ctrl+Shift were changed to be more similar to their Mac counterparts:

    • split vertically was changed from Ctrl+Shift+E to Ctrl+Shift+D
  3. Some shortcuts already used Ctrl+Shift and are unchanged

    • copy is Ctrl+Shift+C
    • devtools is Ctrl+Shift+I
    • update plugins is Ctrl+Shift+U
    • moveLeft2 and moveRight2 are Ctrl+Shift+{ and Ctrl+Shift+}
  4. Some shortcuts were changed to (hopefully) reasonable alternatives

    • split horizontally was changed from Ctrl+Shift+O to Ctrl+Shift+E
    • redo was changed from Ctrl+Shift+Z (now undo) to Ctrl+Shift+Y
    • closing the entire window was changed from Ctrl+Shift+W (now close tab) to Ctrl+Shift+Q
    • full reload was changed from Ctrl+Shift+R (now reload) to Ctrl+Shift+F5
  5. I didn't know what to do with some of the shortcuts, so in those cases I replaced Shift with Z (please review/change!)

    • previous pane was changed from Ctrl+Shift+Alt+Tab to Ctrl+Z+Alt+Tab.
    • moveLeft1 was changed from Ctrl+Shift+Left to Ctrl+Shift+Z+Left
    • moveRight1 was changed from Ctrl+Shift+Right to Ctrl+Shift+Z+Right
    • moveLeft4 was changed from Ctrl+Shift+Tab to Ctrl+Shift+Z+Tab

One of the shortcuts in that last case already involved five simultaneous keypresses, even on a Mac, so I'll humbly suggest it be switched to something else entirely!

I was unable to test whether the undo functionality was broken by using Ctrl+Shift+Z+<key>. I'm not actually sure what undo is supposed to do in hyper (or the moveLeft1/2/3/4 options). Are they bound to any actions?

It may also be worthwhile to review Atom's shortcuts for linux + windows for better defaults. Windows version here https://github.com/atom/atom/blob/master/keymaps/win32.cson and linux version here https://github.com/atom/atom/blob/master/keymaps/linux.cson.

From hyper's pull request template

  • To help whoever reviews your PR, it'd be extremely helpful for you to list whether your PR is ready to be merged:
    • This PR is ready to be merged
  • If there's anything left to do and if there are any related PRs
    • Someone should review the new defaults to make sure they don't break conventions. I'm also unsure what to do with the 4 cases where I used Z instead of Shift, though it didn't seem to break anything for me.
    • Pull requests related to shortcuts would need to be updated, particularly keymap feature #925
  • It'd also be extremely helpful to enable us to update your PR incase we need to rebase or what-not by checking Allow edits from maintainers
    • Done!

…inux, windows, etc) to prevent conflict with common terminal application shortcuts (e.g. ctrl+x in nano).
@fungiboletus
Copy link

I tested the diff on Hyper 1.0.0 Windows. The new shortcuts don't seem to work, Ctrl+Shift+T or Ctrl+Shift+A for example. Ctrl+A still selects the text and doesn't send the command.

It's on the good track though 😃

@nbolten
Copy link
Author

nbolten commented Dec 14, 2016

Hi @yellowiscool! I did a build on Windows 10 (npm run dev + npm run pack + running the dist Hyper.exe) and Ctrl+Shift+T and Ctrl+Shift+W worked fine.

Is Ctrl+A supposed to select all the text in the current cursor? Neither that nor Ctrl+Shift+A work for me, nor does Menu>Edit>Select All.

This is running off of a downloaded zip copy of my fork.

@nbolten
Copy link
Author

nbolten commented Dec 16, 2016

Ah, @yellowiscool, the accelerators.js file didn't exist in v1.0.0, so applying the diff won't do anything (the app won't even read accelerators.js). accelerators.js was added in commit be286c0.

This was referenced Dec 20, 2016
@hahuang65
Copy link

I'd love for this to make it into a build sooner rather than later! I really want to use Hyper on Linux, but I need ctrl-a and ctrl-v to not be swallowed by the app and passed on to the shell.

@AlinaNova21
Copy link

Same here, I use ctrl+a in tmux and ctrl+w and ctrl+x in nano, this is currently a blocker for me using hyper until this is merged.

@celevra
Copy link

celevra commented Dec 31, 2016

same here, this is bug not a feature, the fix has to be released asap. For me its also a show stopper

@hahuang65
Copy link

Any chance we can get a review on this? Would really love to use this...

@alanzanattadev
Copy link

An update on the current state of this issue ?

@internalfx
Copy link

Awwww....I assumed this had been merged when 1.1 came out.....back to running @nbolten fork again.

@sambanks
Copy link

I am also waiting on this one, would love to start using on my Linux desktop

@anaisbetts
Copy link
Contributor

I'm 👍 on this idea, Ctrl-[LETTER] has too much going on to try to assign all of the shortcuts to it

Copy link
Contributor

@anaisbetts anaisbetts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally into this change, just a few suggestions


// Window menu
minimize: 'M',
showPreviousTab: 'Alt+Left',
showNextTab: 'Alt+Right',
showPreviousTab: isMac ? 'Alt+Left' : '+Ctrl+PageDown',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just be Ctrl-Shift-Tab?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinions from me on this, but Ctrl+PageDown/Up is used by VS Code, Gnome Terminal, and Atom.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, Ctrl+Tab and Ctrl+Shift+Tab is also used by Atom/Chrome, so maybe both could be used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

showPreviousTab: 'Alt+Left',
showNextTab: 'Alt+Right',
showPreviousTab: isMac ? 'Alt+Left' : '+Ctrl+PageDown',
showNextTab: isMac ? 'Alt+Right' : '+Ctrl+PageUp',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ctrl-Tab?

showPreviousTab: 'Alt+Left',
showNextTab: 'Alt+Right',
showPreviousTab: isMac ? 'Alt+Left' : '+Ctrl+PageDown',
showNextTab: isMac ? 'Alt+Right' : '+Ctrl+PageUp',
selectNextPane: 'Ctrl+Alt+Tab',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shortcut doesn't work on Windows, it will invoke Alt-Tab

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ctrl+Alt+Tab/Ctrl+Shift+Alt+Tab also conflicts with the Accessibility Switcher on Gnome, that's no good...

iTerm uses Cmd+]/[.

tmux, screen, and Atom use special pane modes for pane manipulation, so e.g 'next pane' in Atom is Ctrl+k to enter pane mode, then Ctrl+n for 'next pane'.

I don't use panes and also don't have strong opinions on this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Ctrl-Shift-LeftArrow + Ctrl-Shift-RightArrow?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just throwing out some ideas.. in atom, the panes are indexed, and we can switch between them using ctrl+[index].. would it be possible to have something similar?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ignatiusreza hyper-pane plugin has been made for this purpose: https://www.npmjs.com/package/hyper-pane

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow! looking good! thanks @chabou 👍

@m-dango
Copy link

m-dango commented Jan 27, 2017

Would it be possible to change the 'update plugins' shortcut? Ctrl+Shift+U is for inserting unicode characters in Ubuntu. Perhaps Ctrl+Shift+P?

@ppot
Copy link
Contributor

ppot commented Feb 11, 2017

Some are missing. But can you update those key map with the desired keys in comments.
ppot#11

{
 "devtools":"Cmd+Alt+I",
 "window:new":"Cmd+N",
 "window:minimize": "Cmd+M",
 "window:close":"Cmd+Shift+W",
 "tab:new":"Cmd+T",
 "tab:next":"Cmd+shift+]",
 "tab:prev":"Cmd+shift+[",
 "pane:next":"Cmd+]",
 "pane:prev":"Cmd+[",
 "pane:left":"Cmd+Alt+Left",
 "pane:right":"Cmd+Alt+Right",
 "pane:up":"Cmd+Alt+Up",
 "pane:down":"Cmd+Alt+Down",
 "pane:vertical":"Cmd+D",
 "pane:horizontal":"Cmd+Shift+D",
 "pane:close":"Cmd+W",
 "editor:undo":"Cmd+Z",
 "editor:redo":"Cmd+Shift+Z",
 "editor:cut":"Cmd+X",
 "editor:copy":"Cmd+C",
 "editor:paste":"Cmd+V",
 "editor:selectAll":"Cmd+A",
 "editor:clearBuffer":"Cmd+K"
}

@celevra
Copy link

celevra commented Feb 16, 2017

why is this not in the actual release?
hyper is not usable under windows and linux without this pull request!
and no, ctrl+shift is no solution!

@nandak522
Copy link

👍

@Stanzilla
Copy link
Collaborator

@ppot any chance to get this in any time soon?

@ppot
Copy link
Contributor

ppot commented Feb 24, 2017

@Stanzilla You mean my version? See #1509

Still need to make flexible Keymap before merging

@Stanzilla
Copy link
Collaborator

Oh, I see

@celevra
Copy link

celevra commented Mar 11, 2017

three weaks gone, 2 new release and this isn't merged... again so much time lost where i cant use hyper, and i want to use hyper!

@ppot
Copy link
Contributor

ppot commented Mar 11, 2017

@celevra Look at Keymap PR

@RobertWHurst
Copy link

Would love to see this merged.

@apinter
Copy link

apinter commented Mar 21, 2017

I just started to use Hyper a few hours ago on my Arch box and love it so much I put it on my F25 daily workstation as well, BUT the keybindings are making it... well not useless, but difficult to live with.
Frankly I can't use vim and having difficulties with tmux as well.
Do you happened to have an estimate when can this fix go public?

EDIT: Me honestly could live with a super as cmd key, is there any way to change it? I think can do that in accelerators.js, but not sure if takes effect?

@ignatiusreza
Copy link

any plan on making the shortcuts configurable?

@ppot
Copy link
Contributor

ppot commented Mar 24, 2017

@ignatiusreza Yes, #1509

@alexellis
Copy link

How can I fix the key-bindings to match with the Gnome Terminal? Is there somewhere I need to paste the {} json block quoted above?

@ppot
Copy link
Contributor

ppot commented Apr 8, 2017

@alexellis PR listed above Keymaps

@alexellis
Copy link

@ppot my question is how to apply / set those keycaps. It's not clear to me from the documentation / PR. Please could you explain?

@ppot
Copy link
Contributor

ppot commented Apr 11, 2017

@alexellis Take a look at my PR Keymaps #1509

@nbolten
Copy link
Author

nbolten commented May 18, 2017

Since a cooler keymaps implementation is on the way, I'm closing this PR. Hope #1509 gets reviewed/merged soon!

@nbolten nbolten closed this May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.