vercel / hyper

A terminal built on web technologies
https://hyper.is
MIT License
43.27k stars 3.52k forks source link

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

Open lewisl9029 opened 7 years ago

lewisl9029 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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.

lewisl9029 commented 7 years ago

I was playing around with this yesterday and found out that Electron doesn't currently support having more than 1 Accelerator per menu item: https://github.com/electron/electron/issues/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 commented 7 years ago

@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 commented 7 years ago

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 commented 6 years ago

Fixed by https://github.com/zeit/hyper/pull/2412 (landed in v2.1.0)

darkdreamingdan commented 5 years ago

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 commented 5 years ago

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