Skip to content

Commit

Permalink
Fixed eclipse-theia#5303: Aligned keybindings with VSCode
Browse files Browse the repository at this point in the history 
Signed-off-by: Josh Pinkney <[email protected]>
  • Loading branch information
@JPinkney
JPinkney committed Jul 8, 2019
1 parent e5117b6 commit 9d50407
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 29 deletions.
35 changes: 30 additions & 5 deletions packages/core/src/browser/keybinding.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [
{
Expand All @@ -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[];

Expand Down Expand Up @@ -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);
Expand All @@ -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 = {
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/browser/keybinding.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -594,6 +592,9 @@ export class KeybindingRegistry {
if (binding.when && !this.whenContextService.match(binding.when, <HTMLElement>event.target)) {
return false;
}
if (binding.command.startsWith('-')) {
return false;
}
return true;
}

Expand Down Expand Up @@ -653,7 +654,6 @@ export class KeybindingRegistry {
setKeymap(scope: KeybindingScope, bindings: Keybinding[]): void {
this.resetKeybindingsForScope(scope);
this.doRegisterKeybindings(bindings, scope);
this.keybindingsChanged.fire(undefined);
}

/**
Expand Down
32 changes: 18 additions & 14 deletions packages/keymaps/src/browser/keybindings-widget.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,15 +346,17 @@ export class KeybindingWidget extends ReactWidget {
const items: KeybindingItem[] = [];
for (let i = 0; i < commands.length; i++) {
const keybindings = this.keybindingRegistry.getKeybindingsForCommand(commands[i].id);
const item: KeybindingItem = {
id: commands[i].id,
command: commands[i].label || '',
keybinding: (keybindings && keybindings[0]) ? keybindings[0].keybinding : '',
context: (keybindings && keybindings[0]) ? keybindings[0].context : '',
scope: (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,
command: commands[i].label || '',
keybinding: (keybindings && keybindingCmd) ? keybindingCmd.keybinding : '',
context: (keybindings && keybindingCmd) ? keybindingCmd.context : '',
scope: (keybindings && keybindingCmd && typeof keybindingCmd.scope !== 'undefined')
? KeybindingScope[keybindingCmd.scope!].toLocaleLowerCase() : '',
};
items.push(item);
}
}
return items;
}
Expand All @@ -381,6 +383,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.scope;
const dialog = new SingleTextInputDialog({
title: `Edit Keybinding For ${command}`,
initialValue: keybinding,
Expand All @@ -389,6 +392,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 });
}
}
});
}
Expand All @@ -406,7 +413,8 @@ export class KeybindingWidget extends ReactWidget {
const rawCommand = this.getRawValue(item.command);
const confirmed = await this.confirmResetKeybinding(rawCommand, rawCommandId);
if (confirmed) {
this.keymapsService.removeKeybinding(rawCommandId);
await this.keymapsService.removeKeybinding(rawCommandId);
await this.keymapsService.removeKeybinding(`-${rawCommandId}`);
}
}

Expand All @@ -415,14 +423,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;
Expand Down
8 changes: 3 additions & 5 deletions packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -48,7 +48,7 @@ export class KeybindingsContributionPointHandler {
}
}
}
this.keybindingRegistry.setKeymap(KeybindingScope.USER, keybindings);
this.keybindingRegistry.registerKeybindings(...keybindings);
}

protected toKeybinding(pluginKeybinding: PluginKeybinding): Keybinding | undefined {
Expand All @@ -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)}`);
}
});
Expand All @@ -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)}`);
}
});
Expand Down

0 comments on commit 9d50407

Please sign in to comment.