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

Hide keybinding in cmd pallet when the when clause doesnt apply #27611

Closed
Tyriar opened this issue May 30, 2017 · 8 comments
Closed

Hide keybinding in cmd pallet when the when clause doesnt apply #27611

Tyriar opened this issue May 30, 2017 · 8 comments
Assignees
Labels
*as-designed Described behavior is as designed keybindings VS Code keybinding issues

Comments

@Tyriar
Copy link
Member

Tyriar commented May 30, 2017

  • VSCode Version: Code - Insiders 1.13.0-insider (c2a17a7, 2017-05-30T05:12:15.312Z)
  • OS Version: Linux x64 4.10.0-21-generic
  • Extensions:
Extension Author Version
EditorConfig EditorConfig 0.9.3
lorem-ipsum Tyriar 1.0.0
sort-lines Tyriar 1.3.0
theme-sapphire Tyriar 0.2.1
vscode-svgviewer cssho 1.4.1
tslint eg2 0.15.0
git-project-manager felipecaputo 1.3.2
md-navigate jrieken 0.0.1
vscode-scss mrmlnc 0.6.2
seti-icons qinjia 0.1.3

#27316

The command palette says tab:

image

But pressing tab in this state does nothing:

image

keybindings.json entry:

{ "key": "tab",                   "command": "editor.emmet.action.expandAbbreviation",
                                     "when": "config.emmet.triggerExpansionOnTab && editorTextFocus && !config.emmet.useModules && !editorHasMultipleSelections && !editorHasSelection && !editorReadonly && !editorTabMovesFocus" },

Maybe it's an issue with the keybindings system? Why is it saying tab in the command palette when the context is wrong (emmet.useModules is true)

@Tyriar Tyriar added the emmet Emmet related issues label May 30, 2017
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented May 31, 2017

Yes, looks like an issue with the keybindings system.

In stable, set emmet.triggerExpansionOnTab to false and you will still see Tab next to the same command

cc @sandy081

@ramya-rao-a ramya-rao-a added keybindings VS Code keybinding issues and removed emmet Emmet related issues labels May 31, 2017
@ramya-rao-a ramya-rao-a added this to the May 2017 milestone May 31, 2017
@sandy081
Copy link
Member

@alexandrudima FYI

@alexdima
Copy link
Member

@ramya-rao-a Sorry I didn't understand what is the issue with the keybindings system ?

@ramya-rao-a
Copy link
Contributor

@alexandrudima

The Emmet: Expand Abbreviation command is bound to the tab key on the following conditions: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/emmet/electron-browser/actions/expandAbbreviation.ts#L30

According to this tab will not be bound to the command when emmet.triggerExpansionOnTab is set to false (among other conditions)

Now, when you bring up this command in the command palette, tab is shown as the keybinding for the command regardless of the value of the setting emmet.triggerExpansionOnTab (or the result of the other conditions)

This is the issue that @Tyriar brought up.

Now I get that it might be an overhead to calculate the when clause of a command to decide whether or not to show the keybinding in the cmd palette. If so, then we can close this issue as "by design".

This issue applies to all commands that are bound to a keybinding with a when clause which has config settings

@ramya-rao-a ramya-rao-a modified the milestones: June 2017, May 2017 Jun 1, 2017
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jun 7, 2017

@alexandrudima ping

cc @rebornix

@rebornix
Copy link
Member

rebornix commented Jun 7, 2017

My personal take is we should not render key shortcut in this case but still show this command in the list.

But I found another interesting behavior. Let's say we are working on a TypeScript file and Emmet should not work in this case. However you can still run Emmet Expand from command palette and it will insert a Tab. My expected result is this command does nothing in this case. (I may be talking about the same thing as #18517 . )

@ramya-rao-a ramya-rao-a changed the title Emmet expand abbreviation is bound to tab but it doesn't work Hide keybinding in cmd pallet when the when clause doesnt apply Jun 7, 2017
@alexdima
Copy link
Member

alexdima commented Jun 8, 2017

Thanks, I understand now your point. The problem is that evaluating when expressions in the context of the Command Palette would remove more than ~50% of the keybindings.

e.g.

{
	"key": "alt+cmd+up",
	"command": "editor.action.insertCursorAbove",
	"when": "editorTextFocus"
}

screen shot 2017-06-08 at 08 01 45

The command editor.action.insertCursorAbove is bound to alt+cmd+up only when the editor text has focus. When opening the Command Palette, that context is lost, e.g. the focus moves to the Command Palette input box, where editorTextFocus does not hold true. The same is true of very many other when expressions, which hold true under special circumstances. e.g.:

{
	"key": "f12",
	"command": "editor.action.goToDeclaration",
	"when": "editorHasDefinitionProvider && editorTextFocus && !isInEmbeddedEditor"
}

Consider having opened two files, one Plain Text, and one which is a TypeScript file, the TypeScript file defines editorHasDefinitionProvider, while the Plain Text one does not. What should the Command Palette do in this case, i.e. which context should it choose to evaluate the commands in. Would it be expected that launching the Command Palette from different contexts present different keybindings. i.e. launching the Command Palette from the terminal or the debug console, etc.

The current presented keybindings are those that are proven to be reachable. Consider the following: e.g.:

{
	"key": "f12",
	"command": "command1",
	"when": "editorTextFocus && !isInEmbeddedEditor"
},
{
	"key": "f12",
	"command": "command2",
	"when": "editorTextFocus"
}

Only in such cases, where we can statically prove that a keybinding rule is shadowed by another rule, we don't show the keybinding. i.e. in this case, the binding for command2 is obviously always going to trap the f12 key when editorTextFocus and there is no possible context under which this does not happen. So we will show no keybinding for command1.

But in other cases e.g.:

{
	"key": "f12",
	"command": "command1",
	"when": "editorTextFocus"
},
{
	"key": "f12",
	"command": "command2",
	"when": "editorTextFocus && b"
}

In such cases it is not possible to statically prove that there exists no context where f12 does not bind to command1 (i.e. when !b, the first rule wins because the second rule's when is false). In such cases, we present f12 to both commands in the Command Palette.

I am open for ideas to how to improve this.

@ramya-rao-a ramya-rao-a modified the milestones: July 2017, June 2017 Jun 23, 2017
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jun 27, 2017

@alexandrudima No bright ideas from me here :(

Closing this issue for now.

The concern specifically around Emmet command and tab will be taken care of once we move to new emmet model and tab will no longer be associated with emmet

Edit:
With VS Code 1.15 released, this is no longer an issue as we have moved to the new emmet model

@ramya-rao-a ramya-rao-a added the *as-designed Described behavior is as designed label Jun 27, 2017
@ramya-rao-a ramya-rao-a removed this from the July 2017 milestone Jun 27, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
*as-designed Described behavior is as designed keybindings VS Code keybinding issues
Projects
None yet
Development

No branches or pull requests

5 participants