From 16e7ca4f57567d792070f827ec6120fd9bbdb538 Mon Sep 17 00:00:00 2001 From: Josh Pinkney Date: Tue, 28 May 2019 16:27:24 -0400 Subject: [PATCH] Fixed #5303. Aligned keybindings with VSCode Signed-off-by: Josh Pinkney --- packages/core/src/browser/keybinding.spec.ts | 35 +++++++++++++++--- packages/core/src/browser/keybinding.ts | 10 ++--- .../src/browser/keybindings-widget.tsx | 37 ++++++++++--------- .../keybindings-contribution-handler.ts | 8 ++-- 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/packages/core/src/browser/keybinding.spec.ts b/packages/core/src/browser/keybinding.spec.ts index 42d93fa671d64..f89f9e09b625f 100644 --- a/packages/core/src/browser/keybinding.spec.ts +++ b/packages/core/src/browser/keybinding.spec.ts @@ -253,8 +253,9 @@ describe('keybindings', () => { expect(bindings.partial.length > 0); }); - it('should not register a shadowing keybinding', () => { + it('should register a shadowing keybinding', () => { const validKeyBinding = 'ctrlcmd+b a'; + const otherValidKeyBinding = 'ctrlcmd+b'; const command = TEST_COMMAND_SHADOW.id; const keybindingShadowing: Keybinding[] = [ { @@ -263,18 +264,19 @@ describe('keybindings', () => { }, { command, - keybinding: 'ctrlcmd+b' + keybinding: otherValidKeyBinding } ]; keybindingRegistry.registerKeybindings(...keybindingShadowing); const bindings = keybindingRegistry.getKeybindingsForCommand(command); - expect(bindings.length).to.be.equal(1); + expect(bindings.length).to.be.equal(2); expect(bindings[0].keybinding).to.be.equal(validKeyBinding); + expect(bindings[1].keybinding).to.be.equal(otherValidKeyBinding); }); - it('shadowed bindings should not be returned', () => { + it('shadowed bindings should be returned', () => { const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] }); let bindings: Keybinding[]; @@ -318,7 +320,7 @@ describe('keybindings', () => { // and finally it should fallback to DEFAULT bindings. bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full; - expect(bindings).to.have.lengthOf(1); + expect(bindings).to.have.lengthOf(2); expect(bindings[0].command).to.be.equal(defaultBinding.command); keybindingRegistry.resetKeybindingsForScope(KeybindingScope.DEFAULT); @@ -328,6 +330,29 @@ describe('keybindings', () => { expect(bindings).to.be.empty; }); + + it('should register conflicting keybindings', () => { + const keybindings: Keybinding[] = [{ + command: TEST_COMMAND.id, + keybinding: 'ctrl+c' + }, + { + command: TEST_COMMAND2.id, + keybinding: 'ctrl+c' + }]; + + keybindingRegistry.setKeymap(KeybindingScope.USER, keybindings); + + const bindings = keybindingRegistry.getKeybindingsForCommand(TEST_COMMAND.id); + + const keyCode = KeyCode.parse(bindings[0].keybinding); + const bindingsForKey = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full; + if (bindingsForKey) { + expect(bindingsForKey).to.have.lengthOf(2); + expect(bindingsForKey[0]).to.be.equal(keybindings[0]); + expect(bindingsForKey[1]).to.be.equal(keybindings[1]); + } + }); }); const TEST_COMMAND: Command = { diff --git a/packages/core/src/browser/keybinding.ts b/packages/core/src/browser/keybinding.ts index 1ea2ef860200a..4cb62ece8afcd 100644 --- a/packages/core/src/browser/keybinding.ts +++ b/packages/core/src/browser/keybinding.ts @@ -242,10 +242,8 @@ export class KeybindingRegistry { protected doRegisterKeybinding(binding: Keybinding, scope: KeybindingScope = KeybindingScope.DEFAULT) { try { this.resolveKeybinding(binding); - if (this.containsKeybinding(this.keymaps[scope], binding)) { - throw new Error(`"${binding.keybinding}" is in collision with something else [scope:${scope}]`); - } this.keymaps[scope].push(binding); + this.keybindingsChanged.fire(undefined); } catch (error) { this.logger.warn(`Could not register keybinding:\n ${Keybinding.stringify(binding)}\n${error}`); } @@ -556,7 +554,7 @@ export class KeybindingRegistry { return false; } - for (const binding of bindings) { + for (const binding of [...bindings].reverse()) { if (this.isEnabled(binding, event)) { if (this.isPseudoCommand(binding.command)) { /* Don't do anything, let the event propagate. */ @@ -594,6 +592,9 @@ export class KeybindingRegistry { if (binding.when && !this.whenContextService.match(binding.when, event.target)) { return false; } + if (binding.command.startsWith('-')) { + return false; + } return true; } @@ -653,7 +654,6 @@ export class KeybindingRegistry { setKeymap(scope: KeybindingScope, bindings: Keybinding[]): void { this.resetKeybindingsForScope(scope); this.doRegisterKeybindings(bindings, scope); - this.keybindingsChanged.fire(undefined); } /** diff --git a/packages/keymaps/src/browser/keybindings-widget.tsx b/packages/keymaps/src/browser/keybindings-widget.tsx index 2be9fe05c5787..a8af5f8d4b353 100644 --- a/packages/keymaps/src/browser/keybindings-widget.tsx +++ b/packages/keymaps/src/browser/keybindings-widget.tsx @@ -440,19 +440,18 @@ export class KeybindingWidget extends ReactWidget { for (let i = 0; i < commands.length; i++) { // Obtain the keybinding for the given command. const keybindings = this.keybindingRegistry.getKeybindingsForCommand(commands[i].id); - const item: KeybindingItem = { - id: commands[i].id, - // Get the command label if available, else use the keybinding id. - command: commands[i].label || commands[i].id, - keybinding: (keybindings && keybindings[0]) ? keybindings[0].keybinding : '', - context: (keybindings && keybindings[0]) - ? keybindings[0].context - ? keybindings[0].context : keybindings[0].when - : '', - source: (keybindings && keybindings[0] && typeof keybindings[0].scope !== 'undefined') - ? KeybindingScope[keybindings[0].scope!].toLocaleLowerCase() : '', - }; - items.push(item); + for (const keybindingCmd of keybindings) { + const item: KeybindingItem = { + id: commands[i].id, + // Get the command label if available, else use the keybinding id. + command: commands[i].label || commands[i].id, + keybinding: keybindingCmd.keybinding, + context: keybindingCmd.context ? keybindingCmd.context : (keybindingCmd.when ? keybindingCmd.when : ''), + source: typeof keybindingCmd.scope !== 'undefined' + ? KeybindingScope[keybindingCmd.scope!].toLocaleLowerCase() : '', + }; + items.push(item); + } } // Sort the keybinding item by label. const sorted: KeybindingItem[] = items.sort((a: KeybindingItem, b: KeybindingItem) => this.compareItem(a.command, b.command)); @@ -500,6 +499,7 @@ export class KeybindingWidget extends ReactWidget { const id = this.getRawValue(item.id); const keybinding = (item.keybinding) ? this.getRawValue(item.keybinding) : ''; const context = (item.context) ? this.getRawValue(item.context) : ''; + const scope = item.source; const dialog = new EditKeybindingDialog({ title: `Edit Keybinding For ${command}`, initialValue: keybinding, @@ -508,6 +508,10 @@ export class KeybindingWidget extends ReactWidget { dialog.open().then(async newKeybinding => { if (newKeybinding) { await this.keymapsService.setKeybinding({ 'command': id, 'keybinding': newKeybinding, 'context': context }); + if (scope === 'default') { + const removalCommand = `-${id}`; + await this.keymapsService.setKeybinding({ 'command': removalCommand, 'keybinding': keybinding, 'context': context }); + } } }); } @@ -535,7 +539,8 @@ export class KeybindingWidget extends ReactWidget { const rawCommand = this.getRawValue(item.command); const confirmed = await this.confirmResetKeybinding(rawCommand); if (confirmed) { - this.keymapsService.removeKeybinding(rawCommandId); + await this.keymapsService.removeKeybinding(rawCommandId); + await this.keymapsService.removeKeybinding(`-${rawCommandId}`); } } @@ -552,14 +557,10 @@ export class KeybindingWidget extends ReactWidget { return 'keybinding value is required'; } try { - const binding = { 'command': command, 'keybinding': keybinding }; KeySequence.parse(keybinding); if (oldKeybinding === keybinding) { return ' '; // if old and new keybindings match, quietly reject update } - if (this.keybindingRegistry.containsKeybindingInScope(binding)) { - return 'keybinding currently collides'; - } return ''; } catch (error) { return error; diff --git a/packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts b/packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts index c4ac1b7f78f6b..83e5459ce3934 100644 --- a/packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts +++ b/packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts @@ -16,7 +16,7 @@ import { injectable, inject } from 'inversify'; import { PluginContribution, Keybinding as PluginKeybinding } from '../../../common'; -import { Keybinding, KeybindingRegistry, KeybindingScope } from '@theia/core/lib/browser/keybinding'; +import { Keybinding, KeybindingRegistry } from '@theia/core/lib/browser/keybinding'; import { ILogger } from '@theia/core/lib/common/logger'; import { OS } from '@theia/core/lib/common/os'; @@ -48,7 +48,7 @@ export class KeybindingsContributionPointHandler { } } } - this.keybindingRegistry.setKeymap(KeybindingScope.USER, keybindings); + this.keybindingRegistry.registerKeybindings(...keybindings); } protected toKeybinding(pluginKeybinding: PluginKeybinding): Keybinding | undefined { @@ -75,7 +75,7 @@ export class KeybindingsContributionPointHandler { private handlePartialKeybindings(keybinding: Keybinding, partialKeybindings: Keybinding[]) { partialKeybindings.forEach(partial => { - if (keybinding.context === undefined || keybinding.context === partial.context) { + if (keybinding.when === undefined || keybinding.when === partial.context) { this.logger.warn(`Partial keybinding is ignored; ${Keybinding.stringify(keybinding)} shadows ${Keybinding.stringify(partial)}`); } }); @@ -84,8 +84,6 @@ export class KeybindingsContributionPointHandler { private handleShadingKeybindings(keybinding: Keybinding, shadingKeybindings: Keybinding[]) { shadingKeybindings.forEach(shadow => { if (shadow.context === undefined || shadow.context === keybinding.context) { - this.keybindingRegistry.unregisterKeybinding(shadow); - this.logger.warn(`Shadowing keybinding is ignored; ${Keybinding.stringify(shadow)}, shadows ${Keybinding.stringify(keybinding)}`); } });