Remove duplicated shortcuts (#12122)

* Remove duplicated shortcuts Fixes #12103 * Allow both Cmd + Enter and Ctrl + Enter on MacOS * Set shortcut on OS bases for `notebook:run-cell` * Fix for non-macOS * Restore two entries for run cell command * Remove shortcuts with `Cmd` on non Mac platform * Add unit tests * Update packages/settingregistry/src/tokens.ts Co-authored-by: Jason Grout <jasongrout@users.noreply.github.com> * Fix integrity and prettify * Explicitly use mac-specific key bindings, rather than relying on new implicit behavior Co-authored-by: Jason Grout <jasongrout@users.noreply.github.com> Co-authored-by: Jason Grout <jason@jasongrout.org>
jupyterlab · Mar 9, 2022 · f64fed3 · f64fed3
1 parent e89ba66
commit f64fed3
Show file tree

Hide file tree

Showing 11 changed files with 283 additions and 17 deletions.
diff --git a/packages/notebook-extension/schema/tracker.json b/packages/notebook-extension/schema/tracker.json
@@ -558,12 +558,14 @@
     },
     {
       "command": "notebook:run-cell",
-      "keys": ["Ctrl Enter"],
+      "macKeys": ["Ctrl Enter"],
+      "keys": [],
       "selector": ".jp-Notebook:focus"
     },
     {
       "command": "notebook:run-cell",
-      "keys": ["Ctrl Enter"],
+      "macKeys": ["Ctrl Enter"],
+      "keys": [],
       "selector": ".jp-Notebook.jp-mod-editMode"
     },
     {

diff --git a/packages/settingeditor-extension/schema/plugin.json b/packages/settingeditor-extension/schema/plugin.json
@@ -8,12 +8,6 @@
       "keys": ["Accel ,"],
       "selector": "body"
     },
-    {
-      "command": "settingeditor:open-json",
-      "args": {},
-      "keys": ["Shift Accel ,"],
-      "selector": "body"
-    },
     {
       "command": "settingeditor:save",
       "args": {},

diff --git a/packages/settingregistry/src/plugin-schema.json b/packages/settingregistry/src/plugin-schema.json
@@ -197,23 +197,53 @@
     "shortcut": {
       "properties": {
         "args": {
+          "title": "The arguments for the command",
           "type": "object"
         },
         "command": {
+          "title": "The command id",
+          "description": "The command executed when the binding is matched.",
           "type": "string"
         },
         "disabled": {
+          "description": "Whether this shortcut is disabled or not.",
           "type": "boolean",
           "default": false
         },
         "keys": {
+          "title": "The key sequence for the binding",
+          "description": "The key shortcut like `Accel A` or the sequence of shortcuts to press like [`Accel A`, `B`]",
+          "items": {
+            "type": "string"
+          },
+          "type": "array"
+        },
+        "macKeys": {
+          "title": "The key sequence for the binding on macOS",
+          "description": "The key shortcut like `Cmd A` or the sequence of shortcuts to press like [`Cmd A`, `B`]",
+          "items": {
+            "type": "string"
+          },
+          "type": "array"
+        },
+        "winKeys": {
+          "title": "The key sequence for the binding on Windows",
+          "description": "The key shortcut like `Ctrl A` or the sequence of shortcuts to press like [`Ctrl A`, `B`]",
+          "items": {
+            "type": "string"
+          },
+          "type": "array"
+        },
+        "linuxKeys": {
+          "title": "The key sequence for the binding on Linux",
+          "description": "The key shortcut like `Ctrl A` or the sequence of shortcuts to press like [`Ctrl A`, `B`]",
           "items": {
             "type": "string"
           },
-          "minItems": 1,
           "type": "array"
         },
         "selector": {
+          "title": "CSS selector",
           "type": "string"
         }
       },

diff --git a/packages/settingregistry/src/tokens.ts b/packages/settingregistry/src/tokens.ts
@@ -592,7 +592,12 @@ export namespace ISettingRegistry {
     disabled?: boolean;
 
     /**
-     * The key combination of the shortcut.
+     * The key sequence of the shortcut.
+     *
+     * ### Notes
+     *
+     * If this is a list like `['Ctrl A', 'B']`, the user needs to press
+     * `Ctrl A` followed by `B` to trigger the shortcuts.
      */
     keys: string[];
 

diff --git a/packages/shortcuts-extension/.vscode/launch.json b/packages/shortcuts-extension/.vscode/launch.json
@@ -0,0 +1,15 @@
+{
+    "version": "0.2.0",
+    "configurations": [
+        {
+            "type": "node",
+            "request": "attach",
+            "name": "Attach to jest",
+            // Usage:
+            // Open the parent directory in VSCode
+            // Run `jlpm test:debug:watch` in a terminal
+            // Run this debugging task
+            "port": 9229
+        }
+    ]
+}
diff --git a/packages/shortcuts-extension/babel.config.js b/packages/shortcuts-extension/babel.config.js
@@ -0,0 +1 @@
+module.exports = require('@jupyterlab/testutils/lib/babel.config');
diff --git a/packages/shortcuts-extension/jest.config.js b/packages/shortcuts-extension/jest.config.js
@@ -0,0 +1,2 @@
+const func = require('@jupyterlab/testutils/lib/jest-config');
+module.exports = func(__dirname);
diff --git a/packages/shortcuts-extension/package.json b/packages/shortcuts-extension/package.json
@@ -30,8 +30,13 @@
   ],
   "scripts": {
     "build": "tsc -b",
+    "build:test": "tsc --build tsconfig.test.json",
     "clean": "rimraf lib && rimraf tsconfig.tsbuildinfo",
     "docs": "typedoc src",
+    "test": "jest",
+    "test:cov": "jest --collect-coverage",
+    "test:debug": "node --inspect-brk node_modules/.bin/jest --runInBand",
+    "test:debug:watch": "node --inspect-brk node_modules/.bin/jest --runInBand --watch",
     "watch": "tsc -b --watch"
   },
   "dependencies": {
@@ -50,7 +55,11 @@
     "typestyle": "^2.0.4"
   },
   "devDependencies": {
+    "@jupyterlab/testutils": "^4.0.0-alpha.5",
+    "@types/jest": "^26.0.10",
+    "jest": "^26.4.2",
     "rimraf": "~3.0.0",
+    "ts-jest": "^26.3.0",
     "typedoc": "~0.22.10",
     "typescript": "~4.5.2"
   },

diff --git a/packages/shortcuts-extension/src/index.ts b/packages/shortcuts-extension/src/index.ts
@@ -10,7 +10,7 @@ import {
   JupyterFrontEndPlugin
 } from '@jupyterlab/application';
 import { ISettingRegistry, SettingRegistry } from '@jupyterlab/settingregistry';
-import { ITranslator } from '@jupyterlab/translation';
+import { ITranslator, nullTranslator } from '@jupyterlab/translation';
 import { IFormComponentRegistry } from '@jupyterlab/ui-components';
 import { CommandRegistry } from '@lumino/commands';
 import {
@@ -19,6 +19,7 @@ import {
   ReadonlyPartialJSONValue
 } from '@lumino/coreutils';
 import { DisposableSet, IDisposable } from '@lumino/disposable';
+import { Platform } from '@lumino/domutils';
 import { Menu } from '@lumino/widgets';
 import { IShortcutUIexternal } from './components';
 import { renderShortCut } from './renderer';
@@ -75,23 +76,24 @@ function getExternalForJupyterLab(
  */
 const shortcuts: JupyterFrontEndPlugin<void> = {
   id: '@jupyterlab/shortcuts-extension:shortcuts',
-  requires: [ISettingRegistry, ITranslator],
-  optional: [IFormComponentRegistry],
+  requires: [ISettingRegistry],
+  optional: [ITranslator, IFormComponentRegistry],
   activate: async (
     app: JupyterFrontEnd,
     registry: ISettingRegistry,
-    translator: ITranslator,
+    translator: ITranslator | null,
     editorRegistry: IFormComponentRegistry | null
   ) => {
-    const trans = translator.load('jupyterlab');
+    const translator_ = translator ?? nullTranslator;
+    const trans = translator_.load('jupyterlab');
     const { commands } = app;
     let canonical: ISettingRegistry.ISchema | null;
     let loaded: { [name: string]: ISettingRegistry.IShortcut[] } = {};
 
     if (editorRegistry) {
       editorRegistry.addRenderer('shortcuts', (props: any) => {
         return renderShortCut({
-          external: getExternalForJupyterLab(registry, app, translator),
+          external: getExternalForJupyterLab(registry, app, translator_),
           ...props
         });
       });
@@ -112,7 +114,23 @@ const shortcuts: JupyterFrontEndPlugin<void> = {
           return shortcuts;
         })
         .concat([schema.properties!.shortcuts.default as any[]])
-        .reduce((acc, val) => acc.concat(val), []) // flatten one level
+        .reduce((acc, val) => {
+          if (Platform.IS_MAC) {
+            return acc.concat(val);
+          } else {
+            // If platform is not MacOS, remove all shortcuts containing Cmd
+            // as they will be modified; e.g. `Cmd A` becomes `A`
+            return acc.concat(
+              val.filter(
+                shortcut =>
+                  !shortcut.keys.some(key => {
+                    const { cmd } = CommandRegistry.parseKeystroke(key);
+                    return cmd;
+                  })
+              )
+            );
+          }
+        }, []) // flatten one level
         .sort((a, b) => a.command.localeCompare(b.command));
 
       schema.properties!.shortcuts.description = trans.__(

diff --git a/packages/shortcuts-extension/test/shortcuts.spec.ts b/packages/shortcuts-extension/test/shortcuts.spec.ts
@@ -0,0 +1,166 @@
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import { ISettingRegistry, SettingRegistry } from '@jupyterlab/settingregistry';
+import * as plugin from '@jupyterlab/shortcuts-extension';
+import { IDataConnector } from '@jupyterlab/statedb';
+import { CommandRegistry } from '@lumino/commands';
+import { Platform } from '@lumino/domutils';
+import pluginSchema from '../schema/shortcuts.json';
+
+describe('@jupyterlab/shortcut-extension', () => {
+  const pluginId = 'test-plugin:settings';
+
+  let dummySettings: ISettingRegistry.IPlugin;
+
+  beforeEach(() => {
+    dummySettings = {
+      data: {
+        composite: {},
+        user: {}
+      },
+      id: plugin.default.id,
+      raw: '{}',
+      schema: pluginSchema as any,
+      version: 'test'
+    };
+  });
+
+  describe('shorcuts list', () => {
+    it('should ignored Cmd on non-Mac platform', async () => {
+      const bar: ISettingRegistry.IPlugin = {
+        data: {
+          composite: {},
+          user: {}
+        },
+        id: pluginId,
+        raw: '{}',
+        schema: {
+          'jupyter.lab.shortcuts': [
+            {
+              command: 'notebook:run-cell',
+              keys: ['Ctrl Enter'],
+              selector: '.jp-Notebook.jp-mod-editMode'
+            },
+            {
+              command: 'notebook:run-cell',
+              keys: ['Cmd Enter'],
+              selector: '.jp-Notebook.jp-mod-editMode'
+            }
+          ],
+          type: 'object'
+        },
+        version: 'test'
+      };
+
+      const connector: IDataConnector<
+        ISettingRegistry.IPlugin,
+        string,
+        string,
+        string
+      > = {
+        fetch: jest.fn().mockImplementation((id: string) => {
+          switch (id) {
+            case bar.id:
+              return bar;
+            case plugin.default.id:
+              return dummySettings;
+            default:
+              return {};
+          }
+        }),
+        list: jest.fn(),
+        save: jest.fn(),
+        remove: jest.fn()
+      };
+
+      const settingRegistry = new SettingRegistry({
+        connector,
+        timeout: Infinity
+      });
+
+      await settingRegistry.load(bar.id);
+
+      plugin.default.activate(
+        {
+          commands: new CommandRegistry()
+        } as any,
+        settingRegistry
+      );
+
+      const settings = await settingRegistry.load(plugin.default.id);
+      const shortcuts = (await settings.get('shortcuts')
+        .composite) as ISettingRegistry.IShortcut[];
+
+      expect(shortcuts).toHaveLength(Platform.IS_MAC ? 2 : 1);
+    });
+
+    it('should ignore colliding shortcuts', async () => {
+      const pluginId = 'test-plugin:settings';
+      const bar: ISettingRegistry.IPlugin = {
+        data: {
+          composite: {},
+          user: {}
+        },
+        id: pluginId,
+        raw: '{}',
+        schema: {
+          'jupyter.lab.shortcuts': [
+            {
+              command: 'notebook:run-cell',
+              keys: ['Accel Enter'],
+              selector: '.jp-Notebook.jp-mod-editMode'
+            },
+            {
+              command: 'another-colliding-command',
+              keys: ['Accel Enter'],
+              selector: '.jp-Notebook.jp-mod-editMode'
+            }
+          ],
+          type: 'object'
+        },
+        version: 'test'
+      };
+
+      const connector: IDataConnector<
+        ISettingRegistry.IPlugin,
+        string,
+        string,
+        string
+      > = {
+        fetch: jest.fn().mockImplementation((id: string) => {
+          switch (id) {
+            case bar.id:
+              return bar;
+            case plugin.default.id:
+              return dummySettings;
+            default:
+              return {};
+          }
+        }),
+        list: jest.fn(),
+        save: jest.fn(),
+        remove: jest.fn()
+      };
+
+      const settingRegistry = new SettingRegistry({
+        connector
+      });
+
+      await settingRegistry.load(bar.id);
+
+      plugin.default.activate(
+        {
+          commands: new CommandRegistry()
+        } as any,
+        settingRegistry
+      );
+
+      const settings = await settingRegistry.load(plugin.default.id);
+      const shortcuts = (await settings.get('shortcuts')
+        .composite) as ISettingRegistry.IShortcut[];
+
+      expect(shortcuts).toHaveLength(1);
+    });
+  });
+});