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

Multiple keymaps and mousetrap #2412

Merged
merged 26 commits into from
Nov 3, 2017
Merged

Conversation

chabou
Copy link
Collaborator

@chabou chabou commented Oct 29, 2017

This is a HUGE PR.
I tried to clean my git history but this is really hard to rebase with some merge commits 😞

This PR have 2 goals:

  • Add feature to specify multiple keymaps for a command
  • Use mousetrap to intercept and interpret key events because it is smarter than Electron

I will not explain how worked keymaps but I need to rewrite it a lot. This how it works now:

Keymap configuration

Configuration is now stored in a unique place. A normalized keymaps object directly in main config object:

{
  'tab:new': ['command+t'],
  'tab:jump:prefix': ['command]'],
  ...
}

User can define a command with a single shortcut or an array of shortcuts.

Plugin can add some command/keymaps using a new function: decorateKeymaps that is applied like other decorating functions: keymap is passed to this plugin handler and plugin can add a shortcut to a command or completely overwrite/remove a shortcut. Previous extendKeymaps function has been marked as deprecated.

Normalization consists of these steps :

  • All shortcuts are transform to array if they are a string.
  • All deprecated shortcuts are checked and transformed. For now, only cmd is warned and transformed to command to suit mousetrap requirements.
  • All *:prefix commands are extrapolated to *:1, *:2 ... *:last commands.

Mousetrap interception

When hyper container is mounted, mousetrap is instanciated and all keybindings are made. For now, my fork is declared as dependency. I made a PR to include an option to make mousetrap to intercept keys on capturing phase and not only on bubbling phase. Why? Without this feature, xterm would receive keys before mousetrap and we would need to duplicate key analysis to know if xterm should ignore this event or not. With this feature, every mousetrap binding flags key events and xterm keyboard handler can rely on this flag to ignore events or not.

Command triggering

When a configured shortcut is hit, binded mousetrap function is called. This function verifies if there is an handler registered by a plugin for this command. If there is one, it call this handler with key event in parameter. If not, this function dispatch a redux action that make a RPC call as a side effect. This redux action can be logged, changed or discarded by plugins like any redux action. RPC call is command with command as parameter (like 'pane:splitVertical'). This RPC is binded to commands that can trigger an RPC call to focusedWindow. Some action like copy/paste are handled directly by Electron with its menu-item role mechanism. In order to trigger them, we need to intercept them by mousetrap like other actions, but we can't preventDefault in order to let Electron act. That's why we have a whitelist of actions that should not be prevented.
We need to prevent default for other events because commands are registered with mousetrap binding to have multiple shortcuts and Electron's menuItems to be clicked. If defaults were not prevented, commands would be triggered twice.

Some actions could be trigger directly by mousetrap binded function because this is renderer-only actions (like pane splitting) but I prefer to stick to actual behaviour: command are always triggered by mainProcess even if its source is a shotrcut hit in BrowserWindow.

What is missing?

  • More tests
  • Documentation

@chabou
Copy link
Collaborator Author

chabou commented Oct 30, 2017

cc @rauchg @albinekb


const commands = {
'window:new': () => {
// If window is created on the same tick, it will consume event too
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

jsconfig.json Outdated
@@ -1,6 +1,6 @@
{
"compilerOptions": {
"checkJs": true,
"checkJs": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

that probably slipped in by mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I turned it off because there is a lot of warning that are not relevant but yes: it shouldn't be in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 1c483ea

@rauchg
Copy link
Member

rauchg commented Nov 3, 2017

This PR looks fantastic to me. Let's get it into canary!

@rauchg rauchg merged commit 2af575c into vercel:canary Nov 3, 2017
@chabou chabou deleted the multiple_keymaps_rebased branch November 3, 2017 06:45
@albinekb
Copy link
Contributor

albinekb commented Nov 3, 2017

Great work @chabou

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.

4 participants