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

Allow multiple keybindings to work #134544

Closed
OldStarchy opened this issue Oct 7, 2021 · 2 comments
Closed

Allow multiple keybindings to work #134544

OldStarchy opened this issue Oct 7, 2021 · 2 comments
Assignees
Labels
*as-designed Described behavior is as designed

Comments

@OldStarchy
Copy link

OldStarchy commented Oct 7, 2021

This is not a duplicate of

Problem

Both yzhang.markdown-all-in-one and takumii.markdowntable define keybindings to overwrite the tab key. In both cases, the behavior is dependant on the content and the cursor position.

The yzhang.markdown-all-in-one binding allows for indenting list items. If the cursor is not in a list, a tab character is inserted instead.

The takumii.markdowntable binding moves the cursor between cells in a formatted table. If the cursor is not in a table, a tab character is inserted instead.

Both of these provide a "new" behavior in some cases and falls back to a "default" if the new behaviour is not applicable. The use cases for these two new behaviors are unrelated, but due to the way keybindings work in vscode, they are fully incompatible.

See these issues for more context

New Feature

Keybindings should be able to call a preventDefault function (or for compatibility purposes, an allowDefault or callNext function such that if the current keybinding determines it is ineffective control may be handed back to either the next matched keybinding if one (or more) exists or the default behavior.

Additionally, some way to order the execution of the bindings would likely be beneficial. This would not require a new GUI either, adding a new property to a keybinding definition "priority": 1 which can be used to define the execution order. (Though in this case, the order the keybindings are executed would not matter)

[
	{
		"key": "tab",
		"command": "markdowntable.nextCell",
		"when": "editorTextFocus && !editorReadonly && !editorTabMovesFocus && !inSnippetMode && !suggestWidgetMultipleSuggestions && !suggestWidgetVisible && editorLangId == 'markdown'",
		"priority": 1
	},
	{
		"key": "tab",
		"command": "markdown.extension.onTabKey",
		"when": "editorTextFocus && !editorReadonly && !editorTabMovesFocus && !inSnippetMode && !suggestWidgetMultipleSuggestions && !suggestWidgetVisible && editorLangId == 'markdown'",
		"priority": 2
	}
]

In this case, the higher priority command markdown.extension.onTabKey will execute first, falling back to markdowntable.nextCell, then finally the default action provided by VSCode to insert a tab character (or indentation with spaces).

@alexdima
Copy link
Member

alexdima commented Oct 7, 2021

I understand the kind of system you are proposing, but we don't want to build that. This is the reason we have created context keys and dispatch keybindings based on when conditions. The philosophy of our current keybinding subsystem is that keys need to be dispatched synchronously to the direct command that needs to execute. The main reason we have opted for such a design is that this allows immediately dispatching of keybindings to the relevant command. You can imagine how a system where one can delegate to the next command using a callNext function could end up with a situation where keybindings are dispatched out-of-order. (e.g. user presses ctrl+c, ctrl+v), yet because of a slow handler for ctrl+c that uses just a bit of await, the ctrl+v gets dispatched before ctrl+c. The way to solve that would be to build async queues to ensure in-order dispatching, but that is also problematic because there is nothing worse than an editor that takes arbitrary amount of time to react to keybindings. Imagine a slow handler for ctrl+c (takes 5s) that would block all other subsequent keypresses; that would result in a really bad end-user experience.

The solution here is for the yzhang.markdown-all-in-one extension to create a new context key e.g. named selectionInMarkdownList and for the takumii.markdowntable extension to create a new context key e.g. named selectionInMarkdownTableCell. These keys need to be proactively maintained (set to true or false) based on the current active text editor and the current cursor position (things which are available via API). Once custom context keys are maintained, the when conditions for the keybindings need to be updated to make use of them.

Custom context keys can be created/updated using commands.executeCommand('setContext', 'contextKey', true);. We have an issue #10471 to create better API in this area.

@alexdima alexdima closed this as completed Oct 7, 2021
@alexdima alexdima added the *as-designed Described behavior is as designed label Oct 7, 2021
@OldStarchy
Copy link
Author

Thanks for the clear explanation.

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
Projects
None yet
Development

No branches or pull requests

2 participants