VSpaceCode / vscode-which-key

which-key like menu for Visual Studio Code
https://vspacecode.github.io/docs/whichkey
MIT License
166 stars 18 forks source link

Reliable key sequence handling #85

Open JimmyZJX opened 7 months ago

JimmyZJX commented 7 months ago

Problem

I constantly lose part of my key sequence when the vscode window is not very responsive.

For example, when I type "SPC f s" very fast, sometimes the which-key menu ends up in the state of "SPC s" and leave vscodevim in the "f" mode.

Analysis

Fundamentally, according to my understanding, it is caused by the multi-process design of VSCode. vscode-which-key commands are handled in the extension-host process and the user's keystrokes are asynchronously send via RPC. There's just no way to make the QuickPick.show call reliably happen before the key is "handled".

One can easily verify it via the autohotkey script (preferrably with some wrapper like adding a shortcut and some delay before)

SendInput {Space}
SendInput {p}
SendInput {f}

In my test setup, I've used a virtual machine with 1GB memory with some VSCode windows opened. I can reproduce the misbahave easily.

Solution

Given that VSCode does not allow the extension command to block key handling, we should seek for reliable state tracking within the main process. And the idea that comes to my mind is to rely on the

setContext (_setContext)

command. Calling this command via keybindings.json is extremely reliable as it's just a synchronous function called on the main process.

I wrote a tiny demo extension to prove the concept

import * as vscode from 'vscode';

const CONTEXT = "inWhichKey";

let state: string[] = [];

async function key(key: string) {
    state.push(key);
    if (state.length >= 3) {
        await vscode.commands.executeCommand("setContext", CONTEXT, false);
        console.log("which-key-ex: got keys", state);
        state = [];
    } else {
        console.log("which-key-ex state:", state);
        await vscode.commands.executeCommand("setContext", CONTEXT, true);
    }
}

export function activate(context: vscode.ExtensionContext) {
    let disposable = vscode.commands.registerCommand("whickKeyEx.key", key);

    context.subscriptions.push(disposable);
}

and set up the keybindings like this

[
    {
        "key": "space",
        "command": "runCommands",
        "args": {
            "commands": [
                {
                    "command": "_setContext",
                    "args": [
                        "inWhichKey",
                        true
                    ],
                },
                {
                    "command": "whickKeyEx.key",
                    "args": "space",
                },
            ]
        },
        "when": "vim.mode == 'Normal'",
                // apparently we need better conditions here. This is just for demo.
    },
    {
        "key": "p",
        "command": "whickKeyEx.key",
        "args": "p",
        "when": "inWhichKey",
    },
    {
        "key": "f",
        "command": "whickKeyEx.key",
        "args": "f",
        "when": "inWhichKey",
    },
]

And now, no keys will ever get a chance to escape which-key. Even with no delays sending the keys with autohotkey, it is always reliable!

Limitations

I think it would be impossible to unset the context without any delay involved. My approach does not solve problems like

SPC f <optionally some delay> s SPC f s

being unreliable. I think there's much less need for that to be handled without any race condition. This is also a problem with the current implementation because the quickpick events are sent asynchronously. In general, I think this is just impossible if you want to dynamically quit the context. (Otherwise you can keep track of all the states by encoding the key-sequence state in keybindings.json via setContext)

One possible drawback of my approach is that it can be unreliable when using together with vscodevim, since the space is not handled by vscodevim. However, I think we can just configure both and fallback to the current behavior if e.g. ESC and SPC are typed very fast when the editor was in the insert mode.

Default keybindings

We should declare all possible key strokes that which-key recognizes in VSCode shortcuts. This should be a reasonably enumerable list. Notice that we only need the inWhichKey condition in those bindings and not the complex conditions. Only the trigger key should be carefully defined with a correct set of conditions, and the user should be able to figure out the correct conditions by themselves.

One plus we get from this approch is that now keys with modifiers can also be handled, like ctrl+u, in the middle of a sequence. We just need to also register them as keybindings like any other key. Users can also define their own bindings with modifiers as long as they register the keybinding.

Possible implementations

Without Quickpick

One possibility is to completely get rid of the quickpick menu and read keys only via shortcuts. The main drawback here is that we need to decide when to "cancel" which-key, e.g. on vscode losing focus, or changing active editor. I'm not sure if this can be implemented with a clean solution.

For the UI, we can draw texts using decorations on the active editor like "SPC j w". If there's no active editor, not drawing is acceptable I think, or we fallback to the current quickpick. But that would be some work to completely rewrite the UI and behaviors that relies on the quickpick component.

Hybrid key handling

The other idea I have is to make use of the context to keep track of missing keys before the quickpick is shown while keeping the key sequence handling logic still bundled with the quickpick UI. Missing keys will be kept in a local queue and processed before the UI gets any text change event, and optionally hide the quickpick if it's already a command.

I haven't tried writing any real code on this idea, and I suspect that it might not work because we have no "onDidShown" event for the quickpick. Only by then we can safely unset the context and always handle keys with quickpick. It is also possible that calling runCommand("setContext", ...) after quickpick.show() always gurantees order of execution. If that's the case, then this approach will work and requires less changes on the existing codebase.

Thanks for reading all these text and please let me know about your thoughts! I think making key sequence handling reliable is very valuable in general and should be a top priority.

stevenguh commented 4 months ago

Thanks for writing this up. These days I have been using vscode web and experienced losing key due to slow rendering of initial menu (I can think of a few things to optimize the initial quick pick rendering -- needs profiling to see if those actually matter however). I suspect this may be similar, have you tried adding a delay after the space key in your reproduction steps?

SendInput {Space}
// Delay
SendInput {p}
SendInput {f}

I think once the Quickpick is rendered, no keys should be lost.

Proposed Solutions

IIUC to sum this up, the idea is to use setContext to shorten key handling time, and use the vscode built-in shortcut's when condition to filter out what was entered.

My understanding is that this approach would make the menu UI very hard to implement. Not to mention that it will need to "capture" a lot of keys as shortcuts, and makes it really hard to update for end user.

Without Quickpick

This is what this repo before QuickPick was used: https://github.com/VSpaceCode/VSpaceCode/tree/vscode-vim. This relies on the key already "captured" by vscode vim so it should have any delay. But no menu.

JimmyZJX commented 4 months ago

I think once the Quickpick is rendered, no keys should be lost.

I agree with this according to my understanding of the quickpick API and after reading the current implementation.

slow rendering of initial menu

I'm not an expert on vscode extension development but I think the latency of running commands (via some sort of RPC from the frontend to the extension host) is another contributing factor, even though it runs as "ui". For web version on this latency is probably better.

repo before QuickPick was used

I took a quick look just now and I can imagine that the user experience is much worse than rendering possible commands with the quickpick UI.

JimmyZJX commented 1 week ago

Hey, I've got some interesting updates :)

Recently I've spent quite some time writing a new extension that mimics what VSpaceCode+which-key do. The experience is much better (still not perfect in theory) by not depending on the quickpick menu (which I believe is the main source of problems).

You are welcome to try this with minimal setup, similar to the VSpaceCode extension (see the readme)! https://marketplace.visualstudio.com/items?itemName=JimmyZJX.leaderkey

and the source code is here https://github.com/JimmyZJX/leaderkey

Also for the web vscode you should set it to be run in ui mode

    "remote.extensionKind": {
        "JimmyZJX.leaderkey": [
            "ui"
        ]
    },
stevenguh commented 1 week ago

Thanks for sharing that's very exciting. I love the creative use of the text decorator API which can make it look and style very closely with spacemacs, and the config setting seems simpler.

I did encounter some issues and quirks however:

  1. Use AHK still can't trigger SPC p f in one go for me in a VM with triggerkey. It works once I added a delay after the SendInput {Space}. Not sure what's missing.
  2. When the last line is in the middle, the menu will display in the middle as well image
  3. The menu doesn't work quite well in notebook (not sure how many are actually using it with notebooks) image
  4. When menu is displayed but focus is changed via mouse, the menu is still active even though it may not be visible
JimmyZJX commented 1 week ago

Thanks for the quick feedback!!

  1. is the original motivation that drives me to do the work, but after a while I realize that the reliable way to interact with vim is via the vim settings, and those commands set to vim is executed asynchronously and is unreliable as well. Very sad.

If instead one bind the "space" key with

    {
        "key": "space",
        "command": "runCommands",
        "args": {
            "commands": [
                {
                    "command": "_setContext",
                    "args": [
                        "leaderkeyState",
                        "SPC"
                    ]
                },
                {
                    "command": "leaderkey.render",
                    "args": "SPC"
                }
            ]
        },
        "when": "!leaderkeyState && vim.mode == 'Normal'",
    }

it becomes extremely reliable for keys typed starting from SPC.

Unfortunately, it's not the way vscodevim works, and you might run into problems like f SPC not jumping to the next char and instead shows up the leaderkey menu. This is because vscodevim does not let us know about its state precisely.

I'm still open to new solutions proposed but it's been slightly more reliable for me. I think the reason is that we don't pay additional delay to wait for the quickpick UI to be rendered. Also I feel like the quickpick UI sometimes might enter a weird state say when SPC TAB is typed too fast and the TAB key got processed by some UI components.

JimmyZJX commented 1 week ago

2-4 are minor issues in comparison.

For 2 and 3, the vscode API is not flexible enough for me to get the viewport of the current editor and the UI positioning end up being very dumb in may aspects. I'm thinking about moving the UI to the top instead to resolve 2, but for notebooks I don't think I can really do much.

Btw there is a status bar showing you the current state of leaderkey (e.g. SPC p-). I believe you turned it off for some good reason, but that's a relatively reliable indicator of the state of leader key.

For 4, due to the current implementation of the key states, I'll need register callbacks (e.g. active editor being closed or swapped out) that intuitively should cause the state to quit. That should be somewhat straightfoward in general and I'll address that soon

stevenguh commented 1 week ago

it becomes extremely reliable for keys typed starting from SPC.

This works more consistently for me as well 👍 Q: How come you are using _setContext with a underscore prefix?

Unfortunately, it's not the way vscodevim works, and you might run into problems like f SPC not jumping to the next char and instead shows up the leaderkey menu. This is because vscodevim does not let us know about its state precisely.

It is very unfortunately. IIRC that's also why the default integration for vspacecode is using vscodevim remapping instead of the built-in shortcuts so to not break something like f SPC.

I'm still open to new solutions proposed but it's been slightly more reliable for me. I think the reason is that we don't pay additional delay to wait for the quickpick UI to be rendered.

Agree. But it does make me wondering if the same capture hack should be used in this repo to capture keys before menu is being rendered.

edamagit used a separate readonly editor, but I doubt that would be any faster. Not sure if there's other APIs.

Also I feel like the quickpick UI sometimes might enter a weird state say when SPC TAB is typed too fast and the TAB key got processed by some UI components.

Maybe the TAB was being processed by the editor?

Speaking of TAB, I think you are missed the capturing of TAB in the default shortcuts because SPC TAB doesn't work for me.

2-4 are minor issues in comparison.

For 2 and 3, the vscode API is not flexible enough for me to get the viewport of the current editor and the UI positioning end up being very dumb in may aspects. I'm thinking about moving the UI to the top instead to resolve 2, but for notebooks I don't think I can really do much.

Agree. A lot of these really are the limitation of the APIs.

Btw there is a status bar showing you the current state of leaderkey (e.g. SPC p-). I believe you turned it off for some good reason, but that's a relatively reliable indicator of the state of leader key.

The editor is in the new window that's why there is no status bar I think.

JimmyZJX commented 1 week ago

I'll reply to your other bullets later, but this

Speaking of TAB, I think you are missed the capturing of TAB in the default shortcuts because SPC TAB doesn't work for me.

is likely caused by a vspacecode configuration that looks like

    {
        "key": "tab",
        "command": "extension.vim_tab",
        "when": "editorFocus && vim.active && !inDebugRepl && vim.mode != 'Insert' && editorLangId != 'magit'"
    },
    {
        "key": "tab",
        "command": "-extension.vim_tab",
        "when": "editorFocus && vim.active && !inDebugRepl && vim.mode != 'Insert'"
    },