fix #7836: respect keybinding scope and registration order during eva…

…luation

Also match only first keybinding on keydown, don't traverse over all keybindings.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

CHANGELOG.md

@@ -8,6 +8,7 @@ Breaking changes:
 
 - [scm] support file tree mode in Source Control view.  Classes that extend ScmWidget will likely require changes [#7505](https://github.com/eclipse-theia/theia/pull/7505)
 - [task] removed `taskId` from `TaskTerminalWidgetOpenerOptions` [#7765](https://github.com/eclipse-theia/theia/pull/7765)
+- [core] `KeybindingRegistry` registers a new keybinding with a higher priority than previously in the same scope [#7839](https://github.com/eclipse-theia/theia/pull/7839)
 
 ## v1.1.0
 

examples/api-tests/src/keybindings.spec.js

@@ -20,6 +20,8 @@ describe('Keybindings', function () {
     const { assert } = chai;
 
     const { Disposable, DisposableCollection } = require('@theia/core/lib/common/disposable');
+    const { isOSX } = require('@theia/core/lib/common/os');
+    const { CommonCommands } = require('@theia/core/lib/browser/common-frontend-contribution');
     const { TerminalService } = require('@theia/terminal/lib/browser/base/terminal-service');
     const { TerminalCommands } = require('@theia/terminal/lib/browser/terminal-frontend-contribution');
     const { ApplicationShell } = require('@theia/core/lib/browser/shell/application-shell');
@@ -30,7 +32,6 @@ describe('Keybindings', function () {
     const { EditorManager } = require('@theia/editor/lib/browser/editor-manager');
     const Uri = require('@theia/core/lib/common/uri');
     const { WorkspaceService } = require('@theia/workspace/lib/browser/workspace-service');
-    const { MonacoEditor } = require('@theia/monaco/lib/browser/monaco-editor');
 
     /** @type {import('inversify').Container} */
     const container = window['theia'].container;
@@ -55,23 +56,23 @@ describe('Keybindings', function () {
         toTearDown.push(commands.onWillExecuteCommand(e => waitForCommand.resolve(e.commandId)));
         keybindings.dispatchKeyDown({
             code: Key.KEY_K.code,
-            metaKey: true,
-            ctrlKey: true
+            metaKey: isOSX,
+            ctrlKey: !isOSX
         }, terminal.node);
         const executedCommand = await waitForCommand.promise;
         assert.equal(executedCommand, TerminalCommands.TERMINAL_CLEAR.id);
     });
 
-    it("disabled keybinding should not override enabled", async () => {
+    it('disabled keybinding should not override enabled', async () => {
         const id = '__test:keybindings.left';
         toTearDown.push(commands.registerCommand({ id }, {
             execute: () => { }
         }));
         toTearDown.push(keybindings.registerKeybinding({
-            command: '__test:keybindings.left',
+            command: id,
             keybinding: 'left',
             when: 'false'
-        }, true));
+        }));
 
         const editor = await editorManager.open(new Uri.default(workspaceService.tryGetRoots()[0].uri).resolve('package.json'), {
             mode: 'activate',
@@ -93,4 +94,25 @@ describe('Keybindings', function () {
         assert.notEqual(executedCommand, id);
     });
 
+    it('later registered keybinding should has higher priority', async () => {
+        const id = '__test:keybindings.copy';
+        toTearDown.push(commands.registerCommand({ id }, {
+            execute: () => { }
+        }));
+        const keybiding = keybindings.getKeybindingsForCommand(CommonCommands.COPY.id)[0];
+        toTearDown.push(keybindings.registerKeybinding({
+            command: id,
+            keybinding: keybiding.keybinding
+        }));
+        const waitForCommand = new Deferred();
+        toTearDown.push(commands.onWillExecuteCommand(e => waitForCommand.resolve(e.commandId)));
+        keybindings.dispatchKeyDown({
+            code: Key.KEY_C.code,
+            metaKey: isOSX,
+            ctrlKey: !isOSX
+        });
+        const executedCommand = await waitForCommand.promise;
+        assert.equal(executedCommand, id);
+    });
+
 });

examples/api-tests/src/typescript.spec.js

@@ -455,6 +455,48 @@ module.exports = (port, host, argv) => Promise.resolve()
         assert.equal(activeEditor.getControl().getModel().getWordAtPosition({ lineNumber, column }).word, 'Container');
     });
 
+    it('editor.action.triggerSuggest navigate', async function () {
+        const editor = await openEditor(serverUri);
+        // const { [|Container] } = require('inversify');
+        editor.getControl().setPosition({ lineNumber: 5, column: 9 });
+        editor.getControl().setSelection({ startLineNumber: 5, startColumn: 9, endLineNumber: 5, endColumn: 18 });
+        // @ts-ignore
+        assert.equal(editor.getControl().getModel().getWordAtPosition(editor.getControl().getPosition()).word, 'Container');
+
+        const suggest = editor.getControl()._contributions['editor.contrib.suggestController'];
+        const getFocusedLabel = () => {
+            const focusedItem = suggest.widget.getValue().getFocusedItem();
+            return focusedItem && focusedItem.item.completion.label;
+        };
+
+        assert.isUndefined(getFocusedLabel());
+        assert.isFalse(contextKeyService.match('suggestWidgetVisible'));
+
+        await commands.executeCommand('editor.action.triggerSuggest');
+        await waitForAnimation(() => contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === 'Container');
+
+        assert.equal(getFocusedLabel(), 'Container');
+        assert.isTrue(contextKeyService.match('suggestWidgetVisible'));
+
+        keybindings.dispatchKeyDown('ArrowDown');
+        await waitForAnimation(() => contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === 'ContainerModule');
+
+        assert.equal(getFocusedLabel(), 'ContainerModule');
+        assert.isTrue(contextKeyService.match('suggestWidgetVisible'));
+
+        keybindings.dispatchKeyDown('ArrowUp');
+        await waitForAnimation(() => contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === 'Container');
+
+        assert.equal(getFocusedLabel(), 'Container');
+        assert.isTrue(contextKeyService.match('suggestWidgetVisible'));
+
+        keybindings.dispatchKeyDown('Escape');
+        await waitForAnimation(() => !contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === undefined);
+
+        assert.isUndefined(getFocusedLabel());
+        assert.isFalse(contextKeyService.match('suggestWidgetVisible'));
+    });
+
     it('editor.action.rename', async function () {
         const editor = await openEditor(serverUri);
         // const |container = new Container();

packages/core/src/browser/keybinding.spec.ts

@@ -226,15 +226,15 @@ describe('keybindings', () => {
 
         keybindingRegistry.setKeymap(KeybindingScope.WORKSPACE, keybindingsSpecific);
 
-        let bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.CtrlCmd] })]).full;
-        expect(bindings).to.have.lengthOf(1);
+        let match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.CtrlCmd] })]);
+        expect(match && match.kind).to.be.equal('full');
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_B, modifiers: [KeyModifier.CtrlCmd] })]).full;
-        expect(bindings).to.have.lengthOf(1);
+        match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_B, modifiers: [KeyModifier.CtrlCmd] })]);
+        expect(match && match.kind).to.be.equal('full');
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] })]).full;
-        const keyCode = KeyCode.parse(bindings[0].keybinding);
-        expect(keyCode.key).to.be.equal(validKeyCode.key);
+        match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] })]);
+        const keyCode = match && KeyCode.parse(match.binding.keybinding);
+        expect(keyCode?.key).to.be.equal(validKeyCode.key);
     });
 
     it('should return partial keybinding matches', () => {
@@ -249,17 +249,17 @@ describe('keybindings', () => {
         validKeyCodes.push(KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] }));
         validKeyCodes.push(KeyCode.createKeyCode({ first: Key.KEY_T }));
 
-        const bindings = keybindingRegistry.getKeybindingsForKeySequence(KeySequence.parse('ctrlcmd+x'));
-        expect(bindings.partial.length > 0);
+        const match = keybindingRegistry.matchKeybiding(KeySequence.parse('ctrlcmd+x'));
+        expect(match && match.kind).to.be.equal('partial');
     });
 
-    it('should not register a shadowing keybinding', () => {
-        const validKeyBinding = 'ctrlcmd+b a';
+    it('should possible to override keybinding', () => {
+        const overridenKeybinding = 'ctrlcmd+b a';
         const command = TEST_COMMAND_SHADOW.id;
         const keybindingShadowing: Keybinding[] = [
             {
                 command,
-                keybinding: validKeyBinding
+                keybinding: overridenKeybinding
             },
             {
                 command,
@@ -270,63 +270,90 @@ describe('keybindings', () => {
         keybindingRegistry.registerKeybindings(...keybindingShadowing);
 
         const bindings = keybindingRegistry.getKeybindingsForCommand(command);
-        expect(bindings.length).to.be.equal(1);
-        expect(bindings[0].keybinding).to.be.equal(validKeyBinding);
+        expect(bindings.length).to.be.equal(2);
+        expect(bindings[0].keybinding).to.be.equal('ctrlcmd+b');
+        expect(bindings[1].keybinding).to.be.equal(overridenKeybinding);
     });
 
-    it('shadowed bindings should be returned last', () => {
+    it('overriden bindings should be returned last', () => {
         const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] });
-        let bindings: Keybinding[];
 
-        const ignoredDefaultBinding: Keybinding = {
+        const overridenDefaultBinding: Keybinding = {
             keybinding: keyCode.toString(),
-            command: 'test.ignored-command'
+            command: 'test.overriden-default-command'
         };
 
         const defaultBinding: Keybinding = {
             keybinding: keyCode.toString(),
-            command: 'test.workspace-command'
+            command: 'test.default-command'
         };
 
         const userBinding: Keybinding = {
             keybinding: keyCode.toString(),
-            command: 'test.workspace-command'
+            command: 'test.user-command'
         };
 
         const workspaceBinding: Keybinding = {
             keybinding: keyCode.toString(),
             command: 'test.workspace-command'
         };
 
-        keybindingRegistry.setKeymap(KeybindingScope.DEFAULT, [defaultBinding, ignoredDefaultBinding]);
+        keybindingRegistry.setKeymap(KeybindingScope.DEFAULT, [overridenDefaultBinding, defaultBinding]);
         keybindingRegistry.setKeymap(KeybindingScope.USER, [userBinding]);
         keybindingRegistry.setKeymap(KeybindingScope.WORKSPACE, [workspaceBinding]);
         // now WORKSPACE bindings are overriding the other scopes
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.have.lengthOf(3);
-        expect(bindings[0].command).to.be.equal(workspaceBinding.command);
+        let match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(workspaceBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.WORKSPACE);
         // now it should find USER bindings
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.have.lengthOf(2);
-        expect(bindings[0].command).to.be.equal(userBinding.command);
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(userBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.USER);
         // and finally it should fallback to DEFAULT bindings.
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.have.lengthOf(1);
-        expect(bindings[0].command).to.be.equal(defaultBinding.command);
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(defaultBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.DEFAULT);
         // now the registry should be empty
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.be.empty;
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match).to.be.undefined;
+
+    });
+
+    it('should not match disabled keybindings', () => {
+        const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] });
+
+        const defaultBinding: Keybinding = {
+            keybinding: keyCode.toString(),
+            command: 'test.workspace-command'
+        };
+        const disableDefaultBinding: Keybinding = {
+            keybinding: keyCode.toString(),
+            command: '-test.workspace-command'
+        };
+
+        keybindingRegistry.setKeymap(KeybindingScope.DEFAULT, [defaultBinding]);
+        let match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(defaultBinding.command);
 
+        keybindingRegistry.setKeymap(KeybindingScope.USER, [disableDefaultBinding]);
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match).to.be.undefined;
+
+        keybindingRegistry.resetKeybindingsForScope(KeybindingScope.USER);
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(defaultBinding.command);
     });
 });