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

Jump to bottom

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 1 commit
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
Next Next commit
GH-8413: Execute the command if has active handler 
Before executing the command via a keybinding, we check if the command
has an active handler.

Closes #8413.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
  • Loading branch information
Akos Kitta committed Aug 24, 2020
commit 6b5bcd8acdc1e2fc36f85ea80180dcc3a273d358
6 changes: 4 additions & 2 deletions packages/core/src/browser/keybinding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,10 @@ export class KeybindingRegistry {
} 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 Down