Fix keyboard shortcuts on Linux and Windows (#1058)

* `command` => `mod`

* `Option` => `Alt`

* Allow hterm to consume a keyboard event only if it's not a Hyper accelerator

* Remove `console.log`s

* Say no to bikeshedding

* We already clear the selection on `onKeyDown`

* Add comments

* Remove meaningless comment

* Add fullscreen shortcut for Windows and Linux

* Use the accelerators defined in `accelerators.js` for the app menu

app/accelerators.js

@@ -0,0 +1,138 @@
+const platform = process.platform;
+
+const isMac = platform === 'darwin';
+
+const prefix = isMac ? 'Cmd' : 'Ctrl';
+
+const applicationMenu = { // app/menu.js
+  preferences: ',',
+  quit: isMac ? 'Q' : '',
+
+  // Shell/File menu
+  newWindow: 'N',
+  newTab: 'T',
+  splitVertically: isMac ? 'D' : 'Shift+E',
+  splitHorizontally: isMac ? 'Shift+D' : 'Shift+O',
+  closeSession: 'W',
+  closeWindow: 'Shift+W',
+
+  // Edit menu
+  undo: 'Z',
+  redo: 'Shift+Z',
+  cut: 'X',
+  copy: isMac ? 'C' : 'Shift+C',
+  paste: 'V',
+  selectAll: 'A',
+  clear: 'K',
+  emojis: isMac ? 'Ctrl+Cmd+Space' : '',
+
+  // View menu
+  reload: 'R',
+  fullReload: 'Shift+R',
+  toggleDevTools: isMac ? 'Alt+I' : 'Shift+I',
+  resetZoom: '0',
+  zoomIn: 'plus',
+  zoomOut: '-',
+
+  // Plugins menu
+  updatePlugins: 'Shift+U',
+
+  // Window menu
+  minimize: 'M',
+  showPreviousTab: 'Alt+Left',
+  showNextTab: 'Alt+Right',
+  selectNextPane: 'Ctrl+Alt+Tab',
+  selectPreviousPane: 'Ctrl+Shift+Alt+Tab',
+  enterFullScreen: isMac ? 'Ctrl+Cmd+F' : 'F11'
+};
+
+const mousetrap = { // lib/containers/hyper.js
+  moveTo1: '1',
+  moveTo2: '2',
+  moveTo3: '3',
+  moveTo4: '4',
+  moveTo5: '5',
+  moveTo6: '6',
+  moveTo7: '7',
+  moveTo8: '8',
+  moveToLast: '9',
+
+  // here `1`, `2` etc are used to "emulate" something like `moveLeft: ['...', '...', etc]`
+  moveLeft1: 'Shift+Left',
+  moveRight1: 'Shift+Right',
+  moveLeft2: 'Shift+{',
+  moveRight2: 'Shift+}',
+  moveLeft3: 'Alt+Left',
+  moveRight3: 'Alt+Right',
+  moveLeft4: 'Ctrl+Shift+Tab',
+  moveRight4: 'Ctrl+Tab',
+
+  // here we add `+` at the beginning to prevent the prefix from being added
+  moveWordLeft: '+Alt+Left',
+  moveWordRight: '+Alt+Right',
+  deleteWordLeft: '+Alt+Backspace',
+  deleteWordRight: '+Alt+Delete',
+  deleteLine: 'Backspace',
+  moveToStart: 'Left',
+  moveToEnd: 'Right',
+  selectAll: 'A'
+};
+
+const allAccelerators = Object.assign({}, applicationMenu, mousetrap);
+const cache = [];
+// ^ here we store the shortcuts so we don't need to
+// look into the `allAccelerators` everytime
+
+for (const key in allAccelerators) {
+  if ({}.hasOwnProperty.call(allAccelerators, key)) {
+    let value = allAccelerators[key];
+    if (value) {
+      if (value.startsWith('+')) {
+        // we don't need to add the prefix to accelerators starting with `+`
+        value = value.slice(1);
+      } else if (!value.startsWith('Ctrl')) { // nor to the ones starting with `Ctrl`
+        value = `${prefix}+${value}`;
+      }
+      cache.push(value.toLowerCase());
+      allAccelerators[key] = value;
+    }
+  }
+}
+
+// decides if a keybard event is a Hyper Accelerator
+function isAccelerator(e) {
+  let keys = [];
+  console.log(e);
+  if (!e.ctrlKey && !e.metaKey && !e.altKey) {
+    // all accelerators needs Ctrl or Cmd or Alt
+    return false;
+  }
+
+  if (e.ctrlKey) {
+    keys.push('ctrl');
+  }
+  if (e.metaKey && isMac) {
+    keys.push('cmd');
+  }
+  if (e.shiftKey) {
+    keys.push('shift');
+  }
+  if (e.altKey) {
+    keys.push('alt');
+  }
+
+  if (e.key === ' ') {
+    keys.push('space');
+  } else {
+    // we need `toLowerCase` for when the shortcut has `shift`
+    // we need to replace `arrow` when the shortcut uses the arrow keys
+    keys.push(e.key.toLowerCase().replace('arrow', ''));
+  }
+
+  keys = keys.join('+');
+
+  return cache.includes(keys);
+}
+
+module.exports.isAccelerator = isAccelerator;
+module.exports.accelerators = allAccelerators;

app/menu.js

@@ -2,6 +2,8 @@ const os = require('os');
 const path = require('path');
 const {app, shell, dialog} = require('electron');
 
+const {accelerators} = require('./accelerators');
+
 const isMac = process.platform === 'darwin';
 const appName = app.getName();
 
@@ -22,7 +24,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Preferences...',
-        accelerator: 'Cmd+,',
+        accelerator: accelerators.preferences,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('preferences');
@@ -64,14 +66,14 @@ module.exports = ({createWindow, updatePlugins}) => {
     submenu: [
       {
         label: 'New Window',
-        accelerator: 'CmdOrCtrl+N',
+        accelerator: accelerators.newWindow,
         click() {
           createWindow();
         }
       },
       {
         label: 'New Tab',
-        accelerator: 'CmdOrCtrl+T',
+        accelerator: accelerators.newTab,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('termgroup add req');
@@ -85,7 +87,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Split Vertically',
-        accelerator: isMac ? 'Cmd+D' : 'Ctrl+Shift+E',
+        accelerator: accelerators.splitVertically,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('split request vertical');
@@ -94,7 +96,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Split Horizontally',
-        accelerator: isMac ? 'Cmd+Shift+D' : 'Ctrl+Shift+O',
+        accelerator: accelerators.splitHorizontally,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('split request horizontal');
@@ -106,7 +108,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Close Session',
-        accelerator: 'CmdOrCtrl+W',
+        accelerator: accelerators.closeSession,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('termgroup close req');
@@ -116,7 +118,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       {
         label: isMac ? 'Close Window' : 'Quit',
         role: 'close',
-        accelerator: 'CmdOrCtrl+Shift+W'
+        accelerator: accelerators.closeWindow
       }
     ]
   };
@@ -125,32 +127,38 @@ module.exports = ({createWindow, updatePlugins}) => {
     label: 'Edit',
     submenu: [
       {
-        role: 'undo'
+        role: 'undo',
+        accelerator: accelerators.undo
       },
       {
-        role: 'redo'
+        role: 'redo',
+        accelerator: accelerators.redo
       },
       {
         type: 'separator'
       },
       {
-        role: 'cut'
+        role: 'cut',
+        accelerator: accelerators.cut
       },
       {
-        role: 'copy'
+        role: 'copy',
+        accelerator: accelerators.copy
       },
       {
-        role: 'paste'
+        role: 'paste',
+        accelerator: accelerators.paste
       },
       {
-        role: 'selectall'
+        role: 'selectall',
+        accelerator: accelerators.selectAll
       },
       {
         type: 'separator'
       },
       {
         label: 'Clear',
-        accelerator: 'CmdOrCtrl+K',
+        accelerator: accelerators.clear,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('session clear req');
@@ -165,7 +173,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       {type: 'separator'},
       {
         label: 'Preferences...',
-        accelerator: 'Cmd+,',
+        accelerator: accelerators.preferences,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('preferences');
@@ -182,7 +190,7 @@ module.exports = ({createWindow, updatePlugins}) => {
     submenu: [
       {
         label: 'Reload',
-        accelerator: 'CmdOrCtrl+R',
+        accelerator: accelerators.reload,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('reload');
@@ -191,7 +199,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Full Reload',
-        accelerator: 'CmdOrCtrl+Shift+R',
+        accelerator: accelerators.fullReload,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.reload();
@@ -200,7 +208,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Toggle Developer Tools',
-        accelerator: isMac ? 'Alt+Command+I' : 'Ctrl+Shift+I',
+        accelerator: accelerators.toggleDevTools,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.webContents.toggleDevTools();
@@ -212,7 +220,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Reset Zoom Level',
-        accelerator: 'CmdOrCtrl+0',
+        accelerator: accelerators.resetZoom,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('reset fontSize req');
@@ -221,7 +229,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Zoom In',
-        accelerator: 'CmdOrCtrl+plus',
+        accelerator: accelerators.zoomIn,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('increase fontSize req');
@@ -230,7 +238,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Zoom Out',
-        accelerator: 'CmdOrCtrl+-',
+        accelerator: accelerators.zoomOut,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('decrease fontSize req');
@@ -245,7 +253,7 @@ module.exports = ({createWindow, updatePlugins}) => {
     submenu: [
       {
         label: 'Update All Now',
-        accelerator: 'CmdOrCtrl+Shift+U',
+        accelerator: accelerators.updatePlugins,
         click() {
           updatePlugins();
         }
@@ -257,7 +265,8 @@ module.exports = ({createWindow, updatePlugins}) => {
     role: 'window',
     submenu: [
       {
-        role: 'minimize'
+        role: 'minimize',
+        accelerator: accelerators.minimize
       },
       {
         role: 'zoom'
@@ -267,7 +276,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Show Previous Tab',
-        accelerator: 'CmdOrCtrl+Option+Left',
+        accelerator: accelerators.showPreviousTab,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('move left req');
@@ -276,7 +285,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Show Next Tab',
-        accelerator: 'CmdOrCtrl+Option+Right',
+        accelerator: accelerators.showNextTab,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('move right req');
@@ -288,7 +297,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Select Next Pane',
-        accelerator: 'Ctrl+Alt+Tab',
+        accelerator: accelerators.selectNextPane,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('next pane req');
@@ -297,7 +306,7 @@ module.exports = ({createWindow, updatePlugins}) => {
       },
       {
         label: 'Select Previous Pane',
-        accelerator: 'Ctrl+Shift+Alt+Tab',
+        accelerator: accelerators.selectPreviousPane,
         click(item, focusedWindow) {
           if (focusedWindow) {
             focusedWindow.rpc.emit('prev pane req');

lib/containers/hyper.js

@@ -44,22 +44,22 @@ class Hyper extends Component {
     const lastIndex = this.terms.getLastTermIndex();
     const document = term.getTermDocument();
     const keys = new Mousetrap(document);
-    keys.bind('command+1', moveTo.bind(this, 0));
-    keys.bind('command+2', moveTo.bind(this, 1));
-    keys.bind('command+3', moveTo.bind(this, 2));
-    keys.bind('command+4', moveTo.bind(this, 3));
-    keys.bind('command+5', moveTo.bind(this, 4));
-    keys.bind('command+6', moveTo.bind(this, 5));
-    keys.bind('command+7', moveTo.bind(this, 6));
-    keys.bind('command+8', moveTo.bind(this, 7));
-    keys.bind('command+9', moveTo.bind(this, lastIndex));
+    keys.bind('mod+1', moveTo.bind(this, 0));
+    keys.bind('mod+2', moveTo.bind(this, 1));
+    keys.bind('mod+3', moveTo.bind(this, 2));
+    keys.bind('mod+4', moveTo.bind(this, 3));
+    keys.bind('mod+5', moveTo.bind(this, 4));
+    keys.bind('mod+6', moveTo.bind(this, 5));
+    keys.bind('mod+7', moveTo.bind(this, 6));
+    keys.bind('mod+8', moveTo.bind(this, 7));
+    keys.bind('mod+9', moveTo.bind(this, lastIndex));
 
-    keys.bind('command+shift+left', moveLeft);
-    keys.bind('command+shift+right', moveRight);
-    keys.bind('command+shift+[', moveLeft);
-    keys.bind('command+shift+]', moveRight);
-    keys.bind('command+alt+left', moveLeft);
-    keys.bind('command+alt+right', moveRight);
+    keys.bind('mod+shift+left', moveLeft);
+    keys.bind('mod+shift+right', moveRight);
+    keys.bind('mod+shift+[', moveLeft);
+    keys.bind('mod+shift+]', moveRight);
+    keys.bind('mod+alt+left', moveLeft);
+    keys.bind('mod+alt+right', moveRight);
     keys.bind('ctrl+shift+tab', moveLeft);
     keys.bind('ctrl+tab', moveRight);
 
@@ -68,10 +68,10 @@ class Hyper extends Component {
     keys.bind('alt+right', bound('moveWordRight'));
     keys.bind('alt+backspace', bound('deleteWordLeft'));
     keys.bind('alt+del', bound('deleteWordRight'));
-    keys.bind('command+backspace', bound('deleteLine'));
-    keys.bind('command+left', bound('moveToStart'));
-    keys.bind('command+right', bound('moveToEnd'));
-    keys.bind('command+a', bound('selectAll'));
+    keys.bind('mod+backspace', bound('deleteLine'));
+    keys.bind('mod+left', bound('moveToStart'));
+    keys.bind('mod+right', bound('moveToEnd'));
+    keys.bind('mod+a', bound('selectAll'));
     this.keys = keys;
   }
 

lib/hterm.js

@@ -1,6 +1,8 @@
 import {hterm, lib} from 'hterm-umdjs';
 import runes from 'runes';
 
+import {isAccelerator} from '../app/accelerators';
+
 import fromCharCode from './utils/key-code';
 import selection from './utils/selection';
 
@@ -164,7 +166,8 @@ hterm.Keyboard.prototype.onKeyDown_ = function (e) {
     e.preventDefault();
   }
 
-  if (e.metaKey || e.altKey || (e.ctrlKey && e.code === 'Tab')) {
+  if (isAccelerator(e)) {
+    // hterm shouldn't consume a hyper accelerator
     return;
   }
   if ((!e.ctrlKey || e.code !== 'ControlLeft') &&
@@ -176,15 +179,6 @@ hterm.Keyboard.prototype.onKeyDown_ = function (e) {
   return oldKeyDown.call(this, e);
 };
 
-const oldKeyPress = hterm.Keyboard.prototype.onKeyPress_;
-hterm.Keyboard.prototype.onKeyPress_ = function (e) {
-  if (e.metaKey) {
-    return;
-  }
-  selection.clear(this.terminal);
-  return oldKeyPress.call(this, e);
-};
-
 // we re-implement `wipeContents` to preserve the line
 // and cursor position that the client is in.
 // otherwise the user ends up with a completely clear