Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-8413: Execute the command if has active handler #8420

Merged
merged 2 commits into from
Aug 28, 2020
Merged

GH-8413: Execute the command if has active handler #8420

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b5bcd8
GH-8413: Execute the command if has active handler
Aug 24, 2020
c297d72
[linter]: Use `Keybinding` from `core`.
Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 22 additions & 20 deletions packages/core/src/browser/keybinding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export namespace KeybindingScope {
export type Keybinding = common.Keybinding;
export const Keybinding = common.Keybinding;

export interface ResolvedKeybinding extends Keybinding {
export interface ResolvedKeybinding extends common.Keybinding {
/**
* The KeyboardLayoutService may transform the `keybinding` depending on the
* user's keyboard layout. This property holds the transformed keybinding that
Expand All @@ -53,7 +53,7 @@ export interface ResolvedKeybinding extends Keybinding {
resolved?: KeyCode[];
}

export interface ScopedKeybinding extends Keybinding {
export interface ScopedKeybinding extends common.Keybinding {
/** Current keybinding scope */
scope: KeybindingScope;
}
Expand All @@ -77,7 +77,7 @@ export interface KeybindingContext {
*/
readonly id: string;

isEnabled(arg: Keybinding): boolean;
isEnabled(arg: common.Keybinding): boolean;
}
export namespace KeybindingContexts {

Expand Down Expand Up @@ -170,7 +170,7 @@ export class KeybindingRegistry {
*
* @param binding
*/
registerKeybinding(binding: Keybinding): Disposable {
registerKeybinding(binding: common.Keybinding): Disposable {
return this.doRegisterKeybinding(binding);
}

Expand All @@ -179,7 +179,7 @@ export class KeybindingRegistry {
*
* @param bindings
*/
registerKeybindings(...bindings: Keybinding[]): Disposable {
registerKeybindings(...bindings: common.Keybinding[]): Disposable {
return this.doRegisterKeybindings(bindings, KeybindingScope.DEFAULT);
}

Expand All @@ -188,15 +188,15 @@ export class KeybindingRegistry {
*
* @param binding
*/
unregisterKeybinding(binding: Keybinding): void;
unregisterKeybinding(binding: common.Keybinding): void;
/**
* Unregister keybinding from the registry
*
* @param key
*/
unregisterKeybinding(key: string): void;
unregisterKeybinding(keyOrBinding: Keybinding | string): void {
const key = Keybinding.is(keyOrBinding) ? keyOrBinding.keybinding : keyOrBinding;
unregisterKeybinding(keyOrBinding: common.Keybinding | string): void {
const key = common.Keybinding.is(keyOrBinding) ? keyOrBinding.keybinding : keyOrBinding;
const keymap = this.keymaps[KeybindingScope.DEFAULT];
const bindings = keymap.filter(el => el.keybinding === key);

Expand All @@ -208,15 +208,15 @@ export class KeybindingRegistry {
});
}

protected doRegisterKeybindings(bindings: Keybinding[], scope: KeybindingScope = KeybindingScope.DEFAULT): Disposable {
protected doRegisterKeybindings(bindings: common.Keybinding[], scope: KeybindingScope = KeybindingScope.DEFAULT): Disposable {
const toDispose = new DisposableCollection();
for (const binding of bindings) {
toDispose.push(this.doRegisterKeybinding(binding, scope));
}
return toDispose;
}

protected doRegisterKeybinding(binding: Keybinding, scope: KeybindingScope = KeybindingScope.DEFAULT): Disposable {
protected doRegisterKeybinding(binding: common.Keybinding, scope: KeybindingScope = KeybindingScope.DEFAULT): Disposable {
try {
this.resolveKeybinding(binding);
const scoped = Object.assign(binding, { scope });
Expand All @@ -228,7 +228,7 @@ export class KeybindingRegistry {
}
});
} catch (error) {
this.logger.warn(`Could not register keybinding:\n ${Keybinding.stringify(binding)}\n${error}`);
this.logger.warn(`Could not register keybinding:\n ${common.Keybinding.stringify(binding)}\n${error}`);
return Disposable.NULL;
}
}
Expand Down Expand Up @@ -258,7 +258,7 @@ export class KeybindingRegistry {
}
}

containsKeybindingInScope(binding: Keybinding, scope = KeybindingScope.USER): boolean {
containsKeybindingInScope(binding: common.Keybinding, scope = KeybindingScope.USER): boolean {
const bindingKeySequence = this.resolveKeybinding(binding);
const collisions = this.getKeySequenceCollisions(this.keymaps[scope], bindingKeySequence)
.filter(b => b.context === binding.context && !b.when && !binding.when);
Expand All @@ -277,7 +277,7 @@ export class KeybindingRegistry {
/**
* Return a user visible representation of a keybinding.
*/
acceleratorFor(keybinding: Keybinding, separator: string = ' '): string[] {
acceleratorFor(keybinding: common.Keybinding, separator: string = ' '): string[] {
const bindingKeySequence = this.resolveKeybinding(keybinding);
return this.acceleratorForSequence(bindingKeySequence, separator);
}
Expand Down Expand Up @@ -399,7 +399,7 @@ export class KeybindingRegistry {
return result;
}

protected isActive(binding: Keybinding): boolean {
protected isActive(binding: common.Keybinding): boolean {
/* Pseudo commands like "passthrough" are always active (and not found
in the command registry). */
if (this.isPseudoCommand(binding.command)) {
Expand All @@ -416,14 +416,16 @@ export class KeybindingRegistry {
* @param binding to execute
* @param event keyboard event.
*/
protected executeKeyBinding(binding: Keybinding, event: KeyboardEvent): void {
protected executeKeyBinding(binding: common.Keybinding, event: KeyboardEvent): void {
if (this.isPseudoCommand(binding.command)) {
/* Don't do anything, let the event propagate. */
} else {
const command = this.commandRegistry.getCommand(binding.command);
if (command) {
this.commandRegistry.executeCommand(binding.command, binding.args)
.catch(e => console.error('Failed to execute command:', e));
if (this.commandRegistry.isEnabled(binding.command, binding.args)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only wonder whether it is a proper place. We have a method isEnabled in this class, which actually used to match keybindings. I am not sure whether it should be moved there? i.e. if keybinding is matching, but there is no active handler, should we skip keybinding and try next? With current proposal we pick keybinding and do nothing. There is also isActive method which is not used anymore. Maybe during refactoring we lost it and the actual fix is to call isActive from isEnabled if keybinding is matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is fair reasoning.

With current proposal we pick keybinding and do nothing

Well, with the master state, we pick the keybinding, and we equally do nothing as an error is thrown.

With the proposed change:

  • we do not fire the fireWillExecuteCommand if there are no active handlers. (same on master)
  • we preventDefault and stopPropagation no matter there were any active handlers or not. (same as on master)

The only behavioral change is that we do not throw an error.

Maybe during refactoring we lost it

OK, I will look into the commit history.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's try

this.commandRegistry.executeCommand(binding.command, binding.args)
.catch(e => console.error('Failed to execute command:', e));
}

/* Note that if a keybinding is in context but the command is
not active we still stop the processing here. */
Expand All @@ -436,7 +438,7 @@ export class KeybindingRegistry {
/**
* Only execute if it has no context (global context) or if we're in that context.
*/
protected isEnabled(binding: Keybinding, event: KeyboardEvent): boolean {
protected isEnabled(binding: common.Keybinding, event: KeyboardEvent): boolean {
const context = binding.context && this.contexts[binding.context];
if (context && !context.isEnabled(binding)) {
return false;
Expand Down Expand Up @@ -567,7 +569,7 @@ export class KeybindingRegistry {
return commandId === KeybindingRegistry.PASSTHROUGH_PSEUDO_COMMAND;
}

setKeymap(scope: KeybindingScope, bindings: Keybinding[]): void {
setKeymap(scope: KeybindingScope, bindings: common.Keybinding[]): void {
this.resetKeybindingsForScope(scope);
this.toResetKeymap.set(scope, this.doRegisterKeybindings(bindings, scope));
this.keybindingsChanged.fire(undefined);
Expand Down Expand Up @@ -629,7 +631,7 @@ export namespace KeybindingRegistry {
* @param fn callback filter on the results
* @return filtered new result
*/
filter(fn: (binding: Keybinding) => boolean): KeybindingsResult {
filter(fn: (binding: common.Keybinding) => boolean): KeybindingsResult {
const result = new KeybindingsResult();
result.full = this.full.filter(fn);
result.partial = this.partial.filter(fn);
Expand Down