-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Conversation
Before executing the command via a keybinding, we check if the command has an active handler. Closes #8413. Signed-off-by: Akos Kitta <[email protected]>
Signed-off-by: Akos Kitta <[email protected]>
It breaks lazy activation of VS Code extensions on command. When a command is about to execute it fires theia/packages/core/src/common/command.ts Line 293 in c297d72
Ignore, I will test it. It seems the plugin system contributes fake handler anyway. |
I did not know this. Any recommendations on how to fix the bug? |
Ignore it. Plugin system contributes the stub handler so it should be fine. Checked with python extension it is good. |
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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 onmaster
) - we
preventDefault
andstopPropagation
no matter there were any active handlers or not. (same as onmaster
)
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to be minimal change
@kittaakos @akosyakov do we merge for the release? |
No. |
What it does
@theia/core/lib/common/keybinding
instead of@theia/core/lib/browser/keybinding
to eliminate a few linter warnings.Cltr/Cmd+j
when there isn't any views in the bottom panel #8413.How to test
Ctrl/Cmd+J
.Problems
view.Ctrl/Cmd+J
Ctrl/Cmd+K Ctrl/Cmd+W
in electron orAlt+Shift+W
in browser.Ctrl/Cmd+K Ctrl/Cmd+W
in electron orAlt+Shift+W
in browser.Review checklist
Reminder for reviewers