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.

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

Removing keybindings with when clauses not working anymore #106524

Closed
mogami95 opened this issue Sep 12, 2020 · 8 comments
Closed

Removing keybindings with when clauses not working anymore #106524

mogami95 opened this issue Sep 12, 2020 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug context-keys insiders-released Patch has been released in VS Code Insiders verified Verification succeeded

Comments

@mogami95
Copy link

  • VSCode Version: 1.49.0
  • OS Version: MacOS 10.14.6 (18G6020)

Steps to Reproduce:

Preference -> Keyboard shortcuts -> search "terminal.find next" -> Remove Keybinding -> doesn't work

Does this issue occur when all extensions are disabled?: Yes

@spasche
Copy link
Contributor

spasche commented Sep 13, 2020

Also affects Windows. It's a regression, it used to work with 1.48.
keybindings.json testcase:

    {
        "key": "ctrl+f",
        "command": "-workbench.action.terminal.focusFind",
        "when": "terminalFocus"
    }

@mogami95
Copy link
Author

@spasche Hi Spasche, I found this bug with the 1.49.
It used to work with the 1.48 on MacOS as well.

@thernstig
Copy link
Contributor

thernstig commented Sep 14, 2020

It seems it's not possible to remove other keybindings as well. None of these work to remove ctrl+f when in the terminal:

[
  {
    "key": "ctrl+k f",
    "command": "workbench.action.terminal.focusFind",
    "when": "terminalFindFocused && terminalProcessSupported || terminalFocus && terminalProcessSupported"
  },
  {
    "key": "ctrl+f",
    "command": "-workbench.action.terminal.focusFind",
    "when": "terminalFindFocused && terminalProcessSupported || terminalFocus && terminalProcessSupported"
  },
  {
    "key": "ctrl+f",
    "command": "-actions.find",
    "when": "terminalFocus && terminalProcessSupported"
  }
]

@Tyriar
Copy link
Member

Tyriar commented Sep 14, 2020

This works:

{ "key": "ctrl+f", "command": "-workbench.action.terminal.focusFind" }

I'm guessing the issue is that && and || don't work when you're removing keybindings. Maybe we should have a warning for such removal keybindings if this is by design?

@Tyriar Tyriar changed the title keybinding of terminal.findNext cannot be removed Removing keybindings with && and || in the when clause doesn't work Sep 14, 2020
@Tyriar Tyriar assigned alexdima and unassigned Tyriar Sep 14, 2020
@spasche
Copy link
Contributor

spasche commented Sep 14, 2020

Testcase on #106524 (comment) also doesn't work and doesn't contain && or || in the when condition.

So the issue may not be related to && or || (or not only).

@Tyriar Tyriar changed the title Removing keybindings with && and || in the when clause doesn't work Removing keybindings with when clauses not working anymore Sep 14, 2020
@Tyriar
Copy link
Member

Tyriar commented Sep 14, 2020

@spasche good point, tested and I can see that too. Updated title.

@thernstig
Copy link
Contributor

Yup, this does not work:

    {
        "key": "ctrl+f",
        "command": "-workbench.action.terminal.focusFind",
        "when": "terminalFocus"
    }

This does:

    {
        "key": "ctrl+f",
        "command": "-workbench.action.terminal.focusFind"
    }

@alexdima
Copy link
Member

This is a problem limited for now to the terminal actions and it has to do with the way the default keybindings are constructed in code by using AND between the precondition and the when context. So the registration of workbench.action.terminal.focusFind leads to a when expression looking like:

(terminalFindFocused || terminalFocus) && terminalProcessSupported.

But the context keys do not support parentheses in their serialized form (that is intentional), so we distribute the AND against the OR to create:

terminalFindFocused && terminalProcessSupported || terminalFocus && terminalProcessSupported.

Unfortunately there is a bug in that code path where the normalized form is not fully equal to the deserialized form. The normalized form contains an extra AND expression with a single operand which should be removed:

image

The workaround is to remove the keybindings without a "when" clause.

@alexdima alexdima added bug Issue identified by VS Code Team member as probable bug context-keys labels Sep 16, 2020
@alexdima alexdima added this to the September 2020 milestone Sep 16, 2020
@rebornix rebornix added the verified Verification succeeded label Oct 1, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug context-keys insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@spasche @rebornix @Tyriar @alexdima @mogami95 @thernstig and others