diff --git a/packages/core/src/browser/keybinding.spec.ts b/packages/core/src/browser/keybinding.spec.ts index f89f9e09b625f..42d93fa671d64 100644 --- a/packages/core/src/browser/keybinding.spec.ts +++ b/packages/core/src/browser/keybinding.spec.ts @@ -253,9 +253,8 @@ describe('keybindings', () => { expect(bindings.partial.length > 0); }); - it('should register a shadowing keybinding', () => { + it('should not register a shadowing keybinding', () => { const validKeyBinding = 'ctrlcmd+b a'; - const otherValidKeyBinding = 'ctrlcmd+b'; const command = TEST_COMMAND_SHADOW.id; const keybindingShadowing: Keybinding[] = [ { @@ -264,19 +263,18 @@ describe('keybindings', () => { }, { command, - keybinding: otherValidKeyBinding + keybinding: 'ctrlcmd+b' } ]; keybindingRegistry.registerKeybindings(...keybindingShadowing); const bindings = keybindingRegistry.getKeybindingsForCommand(command); - expect(bindings.length).to.be.equal(2); + expect(bindings.length).to.be.equal(1); expect(bindings[0].keybinding).to.be.equal(validKeyBinding); - expect(bindings[1].keybinding).to.be.equal(otherValidKeyBinding); }); - it('shadowed bindings should be returned', () => { + it('shadowed bindings should not be returned', () => { const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] }); let bindings: Keybinding[]; @@ -320,7 +318,7 @@ describe('keybindings', () => { // and finally it should fallback to DEFAULT bindings. bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full; - expect(bindings).to.have.lengthOf(2); + expect(bindings).to.have.lengthOf(1); expect(bindings[0].command).to.be.equal(defaultBinding.command); keybindingRegistry.resetKeybindingsForScope(KeybindingScope.DEFAULT); @@ -330,29 +328,6 @@ 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 4cb62ece8afcd..1ea2ef860200a 100644 --- a/packages/core/src/browser/keybinding.ts +++ b/packages/core/src/browser/keybinding.ts @@ -242,8 +242,10 @@ 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}`); } @@ -554,7 +556,7 @@ export class KeybindingRegistry { return false; } - for (const binding of [...bindings].reverse()) { + for (const binding of bindings) { if (this.isEnabled(binding, event)) { if (this.isPseudoCommand(binding.command)) { /* Don't do anything, let the event propagate. */ @@ -592,9 +594,6 @@ export class KeybindingRegistry { if (binding.when && !this.whenContextService.match(binding.when, event.target)) { return false; } - if (binding.command.startsWith('-')) { - return false; - } return true; } @@ -654,6 +653,7 @@ 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 a8af5f8d4b353..2be9fe05c5787 100644 --- a/packages/keymaps/src/browser/keybindings-widget.tsx +++ b/packages/keymaps/src/browser/keybindings-widget.tsx @@ -440,18 +440,19 @@ 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); - 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); - } + 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); } // Sort the keybinding item by label. const sorted: KeybindingItem[] = items.sort((a: KeybindingItem, b: KeybindingItem) => this.compareItem(a.command, b.command)); @@ -499,7 +500,6 @@ 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,10 +508,6 @@ 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 }); - } } }); } @@ -539,8 +535,7 @@ export class KeybindingWidget extends ReactWidget { const rawCommand = this.getRawValue(item.command); const confirmed = await this.confirmResetKeybinding(rawCommand); if (confirmed) { - await this.keymapsService.removeKeybinding(rawCommandId); - await this.keymapsService.removeKeybinding(`-${rawCommandId}`); + this.keymapsService.removeKeybinding(rawCommandId); } } @@ -557,10 +552,14 @@ 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 83e5459ce3934..c4ac1b7f78f6b 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 } from '@theia/core/lib/browser/keybinding'; +import { Keybinding, KeybindingRegistry, KeybindingScope } 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.registerKeybindings(...keybindings); + this.keybindingRegistry.setKeymap(KeybindingScope.USER, 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.when === undefined || keybinding.when === partial.context) { + if (keybinding.context === undefined || keybinding.context === partial.context) { this.logger.warn(`Partial keybinding is ignored; ${Keybinding.stringify(keybinding)} shadows ${Keybinding.stringify(partial)}`); } }); @@ -84,6 +84,8 @@ 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)}`); } });