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

Not possible to map multiple keybinds to the same command in Hyper #2233

Open
2 tasks done
lewisl9029 opened this issue Sep 19, 2017 · 11 comments
Open
2 tasks done

Not possible to map multiple keybinds to the same command in Hyper #2233

lewisl9029 opened this issue Sep 19, 2017 · 11 comments
Labels
help wanted Contributions wanted towards the issue 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper

Comments

@lewisl9029
Copy link

lewisl9029 commented Sep 19, 2017

  • I am on the latest Hyper.app version
  • I have searched the issues of this repo and believe that this is not a duplicate

Issue

I was trying to remap both ctrl+e and cmd+e to pane splitting to allow my config to be sync'ed as-is between my Windows and Mac environments, but found out this wasn't possible in Hyper because the Hyper keymap format maps from commands to keybinds as opposed to from keybinds to commands, like in editors like Atom or VSCode:

Hyper:

  // Only `cmd+e` here takes effect because a JS Object can't have duplicate keys.
  'pane:splitHorizontal': 'ctrl+e',
  'pane:splitHorizontal': 'cmd+e',

Atom:

  // Both `cmd-e` and `ctrl-e` will be mapped to the same command.
  'cmd-e': 'pane:split-right-and-copy-active-item'
  'ctrl-e': 'pane:split-right-and-copy-active-item'

The Atom keymap structure seems to make much more sense to me, because mapping multiple keybinds to the same command seems like a much more useful and common use case than mapping multiple commands to the same key (in fact, I would go as far as to consider the fact that we're able to do this in Hyper to be a bug, because of all the confusing behavior this could lead to).

Any particular reason why we're mapping commands to keybinds instead of the other way around in Hyper? If not, would you consider accepting a PR to invert this mapping? I'd totally understand if you'd rather hold off on changing the keymap structure since it'll break people's configs. If anyone has any ideas on how to implement this change in a backwards compatible way, I'd love to hear them. Thanks!

@chabou chabou added help wanted Contributions wanted towards the issue 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper labels Sep 19, 2017
@chabou
Copy link
Contributor

chabou commented Sep 19, 2017

I really prefer our structure because the goal (the left side) is to configure an action, not a key combination.

But you're right we need this feature.
This needs to work as you expect

 'pane:splitHorizontal': 'ctrl+e',
  'pane:splitHorizontal': 'cmd+e',

And that needs to work too

 'pane:splitHorizontal': [
    'ctrl+e',
    'cmd+e
  ],

PR really welcome

@lewisl9029
Copy link
Author

lewisl9029 commented Sep 20, 2017

@chabou Understood. I'll try taking a look over the weekend and see if I can add support for mapping commands to an array of keybinds to support this use case without changing the keymap structure (like in your 2nd example).

As for the first example, I don't think it's really possible to get that version working without changing the keymap format, since there would be multiple keys of the same name in a single JS object, which results in undefined behavior when accessing or iterating over those keys (looks like in our case it just ignores every key of the same name except the last one). Not much we can do about that AFAIK, but I'm open to suggestions if others have any ideas.

@chabou
Copy link
Contributor

chabou commented Sep 20, 2017

ahahahah you're right, my first solution can't work. And I better understand why you wanted to swap keys/values.

Ok to have an optional array to define multiple keys.

@wilsontayar
Copy link
Contributor

@lewisl9029 let me know if you need help with this.
I believe this is where you're going to take a look:
https://github.com/zeit/hyper/blob/master/app/config/keymaps.js#L28

Can't wait to finally sync my keymaps between windows and mac as well hahaha.

@chabou
Copy link
Contributor

chabou commented Sep 20, 2017

Do no hesitate to completely refactor this file.
Not fan of having 2 arrays commands and keys to maintain. We could deduct one from the other one.

@albinekb albinekb mentioned this issue Sep 25, 2017
8 tasks
@lewisl9029
Copy link
Author

lewisl9029 commented Sep 26, 2017

I was playing around with this yesterday and found out that Electron doesn't currently support having more than 1 Accelerator per menu item: electron/electron#4758 (I suspect Atom probably uses its own keybind system since it doesn't have this limitation)

But I also found out my own use case could be mostly satisfied by specifying shortcuts with cmdorctrl instead of cmd or ctrl alone: https://github.com/electron/electron/blob/master/docs/api/accelerator.md

The only blocker I've ran into with this approach is that some keybinds like cmdorctrl+e work fine under MacOS, but end up triggering terminal-specific behavior in Windows instead of the bounded command, since some ctrl key combinations are reserved for terminal behavior (i.e. cmdorctrl+e outputs a ^E).

I couldn't really figure out how to make the bounded command take precedent, and wasn't really sure if that was even something we wanted to do to begin with. To really support this behavior properly we should probably also provide a way to remap those ctrl-key combinations to something else to ensure they're still available when needed (like the new Windows 10 terminal option that lets you use ctrl-c for copying text and ctrl-shift-c to send a raw ctrl-c to the terminal, which I find very handy, but I have no idea how to actually implement it for Hyper).

One workaround we could consider would be to provide a way for users to provide platform specific overrides for the entire config through something like this:

{
  // rest of config above
  keymaps: {
    'pane:next': 'alt+e'
  },
  platformOverrides: {
    darwin: {
      keymaps: {
        'pane:next': 'cmd+e'
      }
    },
    linux: {},
    win32: {}
  }
}

We could then deep merge the platform specific override config with the rest of the config to arrive at the final config map that we actually use. This would be able to handle things like platform-specific plugins, in addition to keybinds, and still allow the entire config to be sync'd across platforms as is.

Any thoughts?

@rauchg
Copy link
Member

rauchg commented Oct 2, 2017

@lewisl9029 we're working on adding support for multiple shortcuts per action regardless of electron's accelerator supporting only one (I wish it supported many!)

@lewisl9029
Copy link
Author

Alright, in that case it sounds like it's probably not going to involve just a simple extension to the existing keymap system, so I'm probably in over my head on this one. I'll leave this to you guys then. Can't wait to see what you come up with!

@chabou
Copy link
Contributor

chabou commented Nov 3, 2017

Fixed by #2412 (landed in v2.1.0)

@darkdreamingdan
Copy link

darkdreamingdan commented Jan 22, 2019

I'm on 2.1.1, but can't get multiple keybinds working in Hyper on Mac. Here are some examples:

  keymaps: {
    "pane:splitVertical": ["ctrl+shift+d", "command+shift+d"],
    "pane:splitHorizontal": ["ctrl+shift+e", "command+shift+e"],
    "editor:copy": ["ctrl+shift+c", "command+c"],
    "editor:paste": ["ctrl+shift+v", "command+v"],
    "editor:deletePreviousWord": ["alt+backspace", "ctrl+backspace"],
    "editor:deleteNextWord": ["alt+delete", "ctrl+delete"],
  }

The pane commands work fine, but the editor commands don't - for them, only the first key works.

@cicorias
Copy link

btw - odd thing in Ubuntu 18.04 normal keymap was giving me "pane:splitVertical command not found" until I tried this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions wanted towards the issue 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper
Projects
None yet
Development

No branches or pull requests

6 participants