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

kittaakos · 2020-08-24T09:51:04Z

What it does

This PR makes sure the command is not executed via a keybinding if it has no active handlers.
Use @theia/core/lib/common/keybinding instead of @theia/core/lib/browser/keybinding to eliminate a few linter warnings.
Closes [shell] Error when toggling the bottom panel with Cltr/Cmd+j when there isn't any views in the bottom panel #8413.

How to test

Reset your workbench layout.
Toggle the bottom panel: Ctrl/Cmd+J.
Close the Problems view.
Toggle the bottom panel: Ctrl/Cmd+J
You must not see the error:

root ERROR Failed to execute command: Error: The command 'core.toggle.bottom.panel' cannot be executed. There are no active handlers available for the command. (args: [null])
    at CommandRegistry.<anonymous> (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134569:45)
    at step (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134338:23)
    at Object.next (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134319:53)
    at file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134313:71
    at new Promise (<anonymous>)
    at ../../packages/core/lib/common/command.js.__awaiter (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134309:12)
    at CommandRegistry.../../packages/core/lib/common/command.js.CommandRegistry.executeCommand (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134551:16)
    at KeybindingRegistry.../../packages/core/lib/browser/keybinding.js.KeybindingRegistry.executeKeyBinding (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:111317:38)
    at KeybindingRegistry.../../packages/core/lib/browser/keybinding.js.KeybindingRegistry.run (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:111409:22)
    at HTMLDocument.<anonymous> (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:109575:35)

Reset your workbench layout.
Open one or two editors.
Close the editors in the main area: Ctrl/Cmd+K Ctrl/Cmd+W in electron or Alt+Shift+W in browser.
Editors are closed.
Close the editors in the main area: Ctrl/Cmd+K Ctrl/Cmd+W in electron or Alt+Shift+W in browser.
You must not see the errors:

root ERROR Failed to execute command: Error: The command 'core.close.all.tabs' cannot be executed. There are no active handlers available for the command. (args: [null])
    at CommandRegistry.<anonymous> (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134569:45)
    at step (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134338:23)
    at Object.next (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134319:53)
    at file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134313:71
    at new Promise (<anonymous>)
    at ../../packages/core/lib/common/command.js.__awaiter (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134309:12)
    at CommandRegistry.../../packages/core/lib/common/command.js.CommandRegistry.executeCommand (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:134551:16)
    at KeybindingRegistry.../../packages/core/lib/browser/keybinding.js.KeybindingRegistry.executeKeyBinding (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:111317:38)
    at KeybindingRegistry.../../packages/core/lib/browser/keybinding.js.KeybindingRegistry.run (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:111409:22)
    at HTMLDocument.<anonymous> (file:///Users/akos.kitta/git/theia/examples/electron/lib/bundle.js:109575:35)

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

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>

Signed-off-by: Akos Kitta <kittaakos@typefox.io>

akosyakov · 2020-08-24T11:58:01Z

It breaks lazy activation of VS Code extensions on command. When a command is about to execute it fires onWillExecuteCommand event which gives a chance to participate other clients (like plugin system) and contribute handlers:

theia/packages/core/src/common/command.ts

Line 293 in c297d72

await this.fireWillExecuteCommand(commandId, args);

theia/packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

Line 192 in c297d72

    
           this.commands.onWillExecuteCommand(event => this.ensureCommandHandlerRegistration(event));

Ignore, I will test it. It seems the plugin system contributes fake handler anyway.

kittaakos · 2020-08-24T12:01:41Z

breaks lazy activation of VS Code extensions on command

I did not know this. Any recommendations on how to fix the bug?

akosyakov · 2020-08-24T12:02:22Z

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.

akosyakov · 2020-08-24T12:05:49Z

packages/core/src/browser/keybinding.ts

        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)) {


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.

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.

ok, let's try

akosyakov

it seems to be minimal change

vince-fugnitto · 2020-08-27T12:30:49Z

@kittaakos @akosyakov do we merge for the release?

kittaakos · 2020-08-27T12:38:56Z

@kittaakos @akosyakov do we merge for the release?

No.

Akos Kitta added 2 commits August 24, 2020 11:33

GH-8413: Execute the command if has active handler

6b5bcd8

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>

[linter]: Use Keybinding from core.

c297d72

Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos requested a review from akosyakov August 24, 2020 09:51

akosyakov reviewed Aug 24, 2020

View reviewed changes

akosyakov added the keybindings issues related to keybindings label Aug 24, 2020

akosyakov approved these changes Aug 24, 2020

View reviewed changes

kittaakos merged commit ca6754e into master Aug 28, 2020

kittaakos deleted the GH-8413 branch August 28, 2020 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kittaakos commented Aug 24, 2020 •

edited

Loading

akosyakov commented Aug 24, 2020 •

edited

Loading

kittaakos commented Aug 24, 2020

akosyakov commented Aug 24, 2020

akosyakov Aug 24, 2020

kittaakos Aug 24, 2020

akosyakov Aug 24, 2020

akosyakov left a comment

vince-fugnitto commented Aug 27, 2020

kittaakos commented Aug 27, 2020

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

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

Conversation

kittaakos commented Aug 24, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Aug 24, 2020 • edited Loading

kittaakos commented Aug 24, 2020

akosyakov commented Aug 24, 2020

akosyakov Aug 24, 2020

Choose a reason for hiding this comment

kittaakos Aug 24, 2020

Choose a reason for hiding this comment

akosyakov Aug 24, 2020

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Aug 27, 2020

kittaakos commented Aug 27, 2020

kittaakos commented Aug 24, 2020 •

edited

Loading

akosyakov commented Aug 24, 2020 •

edited

Loading