keymap feature

[iamstarkov]

PR needs feedback

I want flexible keymap for Hyper #872. First approach to use single RPC to communicate all the didnt really work out well, because of two separate RPCs, one for Electron and on for Renderer windows. So there is another approach to use Menu as kind of one entry-point to bind all hotkeys.

• 995bfa0 Remove all possible to implement via Menu hotkeys
• f629cc1 Change hard coded Menu accelerators to "convention"ed command field, which can be used to identify what for this menu item is responsible
• 69e4fd8 Add hyper-keymap as standard plugin

🤔 Suggestion:

• keep keymap management as a default, but still separate plugin _{Atom is doing it with everything}
• Move hyper-keymap into hyper org _{should we create one?}
• Find a way to to notify end users about need to have hyper-keymap in their configs. Might be possible to use question messageBox with "add it" or "dont bother me" buttons.

😟 Cons (temporarily):

• at the time PR is being submitted hyper-keymap is not working fully, due to existing key bindings declared in lib/containers/hyper.js. Though, this pull-request almost fixes it.
• remaining key bindings, like move-word-{left,right} needs to be implemented the same way as everything else. I think this is possible, because as far as it is hotkey, there is no need in passing additional arguments, so it should possible to implement. That said, i seek for help to do that.

😎 Pros:

• 🎉 Configured keymap, yay!
• 👍 Even in one place
• 🆓 Documentation about all existing hotkeys in one place, for free!

I tried my best, to bring keymap management to Hyper and now looking for feedback and discussion. What do you think about proposed approach?

[flybayer]

This looks promising to me! 👍

[iamstarkov]

@codetheory @matheuss what do you think?

[ekmartin]

Cool :) Is it necessary to couple keybindings so tightly to Menu though? I think being able to have keybindings that aren't listed in the menu is a good thing, especially if you consider wanting to have multiple keybindings per action (which I'm not sure is even possible with accelerator).

Couldn't an alternative be to just treat it as a regular part of the config? That way you could do something like accelerator: keyBindings['new-window'], and still have keys.bind(keyBindings['move-left'], moveLeft) in hyperterm.js. Could still have menu on the form you've suggested though, but just transform it to explicitly use the keybindings from the config - like you've done.

Also, regarding the keymap, I think it would really help in terms of user experience if all the default keybindings are listed explicitly where you're able to change them, so that you don't have to look up the names to know how to make modifications (seeing as we don't have an insane amount of different combinations). This might make the config too large though, which is why I previously argued for having it in a separate file (and bundle everything below a .hyper folder).

In regards to if it should be implemented in the core or as a default plugin, I think it would definitely make more sense to have this in Hyper itself, as it wouldn't be too different from the already existing stuff that makes use of values from the config.

[Melonbwead]

Very excited to see this being implemented! I found this PR while looking to add ketbindings to hyper. i have a suggestions for a Hyper command, I would love to be able to have a global key binding that would bring up the hyper app to the foreground in focused where ever I was, not sure how you would name that command tho.

[iamstarkov]

@chennybaby i believe there are several plugins with that idea in mind. check out https://github.com/bnb/awesome-hyper

[rye]

This looks an awful lot like what I was looking for in #783. A couple of suggestions:

• I'd like to make sure that the user has the option to send the actual keycode through to the shell. For me, personally, I'd much rather have access to Emacs keybinds in my terminal like C-w, M-w (which is CmdOrControl-w or the close command in Hyper in my setup).
  • What this really means is the option to disable things altogether. Sure, it might be lame to disable creating new tabs for most users, but I'd like to have the option for my own setup, and I'm sure many would as well. Perhaps allow users to bind keybinds to null, then have the internal system simply forward keycodes on if the keybind is null?

If the PR can fulfill these requirements, then it's a huge win and I'd be proud to switch to Hyper full-time. I'm stuck on iTerm because of the lack of disable-ability, if that makes any sense.

[rye]

I think it might also make sense to think about generalizing everything and letting users specify which keys correspond to which keybinds—I quite like the C-, M-, and s- prefixes that Emacs uses for commands, and I think they'd be good for using to write keybinds.

[jritsema]

Great idea @iamstarkov.

I'm interested in pc-style word navigation inside the terminal as well as pc-style copy/paste (would obviously need to remap ctrl+c to something else)...

something like...

keymap: {
  'Ctrl+Left': 'move-to-beginning-of-word',
  'Ctrl+C': 'copy',
  'Ctrl+V': 'paste',
  'Alt+C': 'break',
}

Would this PR support that?

[iamstarkov]

@ekmartin

Cool :) Is it necessary to couple keybindings so tightly to Menu though?

Correct, Its not necessary to couple keybindings to Menu. Though, its the only working RPC-ish way to emit actions to both Renderer and Electron windows.

[iamstarkov]

@ekmartin

I think being able to have keybindings that aren't listed in the menu is a good thing, especially if you consider wanting to have multiple keybindings per action (which I'm not sure is even possible with accelerator).

Correct, Its not possible with accelerators.

[iamstarkov]

@ekmartin

Couldn't an alternative be to just treat it as a regular part of the config? That way you could do something like accelerator: keyBindings['new-window'], and still have keys.bind(keyBindings['move-left'], moveLeft) in hyperterm.js. Could still have menu on the form you've suggested though, but just transform it to explicitly use the keybindings from the config - like you've done.

I finally see what you are saying, thats probably gonna work out

[iamstarkov]

@ekmartin

Also, regarding the keymap, I think it would really help in terms of user experience if all the default keybindings are listed explicitly where you're able to change them, so that you don't have to look up the names to know how to make modifications (seeing as we don't have an insane amount of different combinations). This might make the config too large though, which is why I previously argued for having it in a separate file (and bundle everything below a .hyper folder).

Its debatable, user's keymap config rarely would grow much. And default keybindings will be configured in default config in Hyper itself.

[iamstarkov]

@ekmartin

In regards to if it should be implemented in the core or as a default plugin, I think it would definitely make more sense to have this in Hyper itself, as it wouldn't be too different from the already existing stuff that makes use of values from the config.

will copy our discussion from slack:

1:24 AM @ekmartin
it makes more sense to have it in core imo

1:25 AM @codetheory
I agree

1:26 AM @iamstarkov
ehm

☯ apm ls | grep key
├── keybinding-resolver@0.35.0

atom did it via plugin
core plugin for sure

1:27 AM @codetheory
Most of everything that makes atom is via a plugin

1:27 AM @ekmartin
^
1:27 AM @codetheory
Hyper is different, it should be in the core

1:27 AM @iamstarkov
yep
why its that different?

1:28 AM @ekmartin
keybindings really isn’t that different from anything else that gets read from the config though

1:28 AM @iamstarkov
im not arguing against it, just want to know details, before i can say im sold with this approach

1:29 AM
@ekmartin and? plugins and themes utilise config just fine

1:29 AM @ekmartin
I think if you’d want to split things into plugins it would make more sense to rewrite some of the existing core with that in mind as well

1:29 AM @iamstarkov
probably

1:29 AM @codetheory
Yeah, for now I think it's best in the core since that's how Hyper's architecture is

1:29 AM @iamstarkov
okay

1:29 AM @codetheory
We could split things out into plugins later if we decide to go that way
although, in my opinion, it'd be cool to see Hyper created with core plugins

1:30 AM @ekmartin
@matheus is more up to date on hyper than I am now though 🙂

1:30 AM @codetheory
since it'd allow the user to get acquainted with plugins
But Hyper's config is a bit hard for everyone to use
as we can see from the issues

1:30 AM @iamstarkov
i cant rewrite core in one pr, even if i could it would be horrible to review such big pr

1:30 AM @codetheory
No, no. We shouldn't do that anyway 😛

1:31 AM @iamstarkov
so i thought i'd start with one core plugin

1:31 AM @codetheory
If @rauchg or @leo were around, I'd ask them
They're the ultimate architects
But I do agree that core plugins would be a cool thing
for many reasons

1:33 AM @iamstarkov
@codetheory

We could split things out into plugins later if we decide to go that way

thats legit point as well

[PSNAppz]

Great Work @iamstarkov 👍

[megalithic]

@iamstarkov mind resolving the merge conflicts? @PSNAppz are you able to merge once conflicts are accurately resolved? this would be great and open up even more doors for hyper via https://github.com/iamstarkov/hyper-keymap#-caveats

[PSNAppz]

@megalithic Yes.

[iamstarkov]

heads up to @megalithic and everybody in this thread: yay, i will be working on this on this holyday

[macobo]

@iamstarkov To make your life easier (and to get this wonderful feature out faster), I took a stab at rebasing this. Feel free to peruse!

https://github.com/zeit/hyper/compare/master...macobo:iamstarkov-feat/keymap-2?expand=1

[macobo]

Bump on this as without this hyper is unusable on linux - should I open a separate PR with the rebase?

[iamstarkov]

@macobo can i ask you to make a pull-request to my fork with your rebase?

[iamstarkov]

sorry, i have to say this: i couldnt make it and have no energy to fix this issue on my own. That said, I will close this pull-request. Another reason to close PR is this branch and code is outdated.

Please refer to original issue #872

[ppot]

Plz add request inside issue: ppot#11