Repository: vscode-neovim/vscode-neovim (ts)

Issue:
bug: vim-sandwich stops working since 1.14
### Check the following:

- [X] I have checked the behavior after setting "vscode-neovim.neovimClean" in VSCode settings.
- [X] I have read the vscode-neovim docs.
- [X] I have searched existing issues.

### Neovim version (nvim -v)

0.9.1

### Operating system/version

Windows 7

### Describe the bug

I have been using https://github.com/machakann/vim-sandwich in neovim and vscode-neovim since day 1. It has been working perfectly. But it stops working since v1.14 (the last version it is still working is 1.13.1).

When try to delete / replace pair of parentheses, it just alter the texts outside of the selected scope.   

### Steps To Reproduce

1. Install vim-sandwich => `Plug 'machakann/vim-sandwich'`
2. Create a *.md file with 

```
"test_1"
"test_2"
"test_3"
```
3. Run `sr"'` in normal mode. It should replace `"` pair into `'` pair. 
4. Run `sd"` in normal mode. It should delete `"` pair

![vim-sandwich](https://github.com/vscode-neovim/vscode-neovim/assets/34254408/7c6894c5-06fc-4012-b102-16dd92ab41d4)



### Expected Behavior


Run `sr"'` in normal mode. It should replace `"` pair into `'` pair. 
Run `sd"` in normal mode. It should delete `"` pair

Pull request:
fix: rapid cmdline_show/cmdline_hides would not be handled logically
Replaces #2081
Fixes #2079 


I came up with [this idea](https://github.com/vscode-neovim/vscode-neovim/pull/2081#issuecomment-2157087672) a little while ago, but haven't had the chance to sit down and implement it. Ultimately, most problems with our two-way cmdline sync originats from the fact that a cmdline_hide can be sent before the quickpick actually executes a hide event, or vice versa. Attempting to  manipulate the events to "ignore" is error prone, given this ordering problem. 

To repeat myself a bit from the linked comment, the "idea" is that we can handle cmdline events in batches. Once we receive a cmdline_hide event at level 1 (i.e. no nested command lines), we can consider the current batch "terminated", and begin queueing up events. Then, when we get a VSCode "hide" event, we can flush the batch, and continue processing events. 

A screencast showing some usage of the vim sandwich plugin, and various simple commandline usages


https://github.com/vscode-neovim/vscode-neovim/assets/977151/9fb86d8c-d180-470d-910f-ab18d046c574



Patch:
diff --git a/src/cmdline/cmdline_queue.ts b/src/cmdline/cmdline_queue.ts
new file mode 100644
index 0000000..2fc99a9
--- /dev/null
+++ b/src/cmdline/cmdline_queue.ts
@@ -0,0 +1,69 @@
+import { type EventBusData } from "../eventBus";
+
+/**
+ * A queue for "batching" cmdline events.
+ *
+ * In most cases (simple command-line usage like :w<CR>), events will be passed through and not queued. However, in more
+ * complicated cases, we need to take "batch" different cmdline events. Due to how the JS event loop works, VSCode's
+ * QuickPick onHide event will not execute until after we have processed all of our events, even if they belong to
+ * different instances of the cmdline. Worse, an onHide may even precede our cmdline_hide. As such, we queue up
+ * batches of events to retransmit back to the CmdlineManager.
+ */
+export class CmdlineQueue {
+    private pendingBatches: EventBusData<"redraw">[][] = [];
+    private needFlush: boolean = false;
+    private lastSeenLevel: number | null = null;
+
+    /**
+     * Given an nvim redraw event, checks whether or not the caller should handle this event. If this returns false, the
+     * event is enqueued for a future call to `flushBatch`
+     *
+     * @param event The redraw event received from nvim
+     * @returns Whether or not this event should be processed immediately
+     */
+    handleNvimRedrawEvent(event: EventBusData<"redraw">): boolean {
+        const shouldProcess = !this.needFlush;
+        if (this.needFlush) {
+            this.addToBatch(event);
+        }
+
+        if (event.name === "cmdline_show") {
+            const [_content, _pos, _firstc, _prompt, _indent, level] = event.args[0];
+            this.lastSeenLevel = level;
+        } else if (event.name === "cmdline_hide" && this.lastSeenLevel === 1) {
+            // Only make a new batch when we're preforming a hide for a known level 1 cmdline
+            this.prepareBatch();
+        }
+
+        return shouldProcess;
+    }
+
+    /**
+     * Flush the currently pending batch, and remove it from the queue
+     *
+     * @returns The next batch of pending events, or null if none are pending
+     */
+    flushBatch(): EventBusData<"redraw">[] | null {
+        const result = this.pendingBatches.shift() ?? null;
+        this.needFlush = false;
+        this.lastSeenLevel = null;
+
+        return result;
+    }
+
+    private prepareBatch() {
+        this.pendingBatches.push([]);
+        this.needFlush = true;
+        this.lastSeenLevel = null;
+    }
+
+    private addToBatch(event: EventBusData<"redraw">) {
+        if (this.pendingBatches.length === 0) {
+            // No batch to add this to. We can't safely assume we should make a new batch (but this should never happen)
+            throw new Error("Invalid cmdline state");
+        }
+
+        const batch = this.pendingBatches[this.pendingBatches.length - 1];
+        batch.push(event);
+    }
+}
diff --git a/src/cmdline_manager.ts b/src/cmdline_manager.ts
index b338a0e..4395028 100644
--- a/src/cmdline_manager.ts
+++ b/src/cmdline_manager.ts
@@ -6,6 +6,7 @@ import { disposeAll } from "./utils";
 import { createLogger } from "./logger";
 import { calculateInputAfterTextChange } from "./utils/cmdline_text";
 import { GlyphChars } from "./constants";
+import { CmdlineQueue } from "./cmdline/cmdline_queue";
 
 const logger = createLogger("CmdLine", false);
 
@@ -39,6 +40,9 @@ export class CommandLineManager implements Disposable {
 
     private input: QuickPick<QuickPickItem>;
     private state = new CmdlineState();
+    // A queue of incoming events. See docblock for more details, but this is used to resolve an inherent
+    // race condition in the way we handle events.
+    private queue = new CmdlineQueue();
 
     public constructor(private main: MainController) {
         eventBus.on("redraw", this.handleRedraw, this, this.disposables);
@@ -80,7 +84,21 @@ export class CommandLineManager implements Disposable {
         this.input.activeItems = [];
     }
 
-    private handleRedraw({ name, args }: EventBusData<"redraw">) {
+    private handleRedraw(event: EventBusData<"redraw">) {
+        const allowedEvents = ["cmdline_show", "cmdline_hide", "popupmenu_show", "popupmenu_select", "popupmenu_hide"];
+        if (allowedEvents.indexOf(event.name) === -1) {
+            // Drop any events not relevant to the cmdline; there is no sense in wasting memory queuing them up
+            // if they'll just be dropped anyway.
+            return;
+        }
+
+        const handle = this.queue.handleNvimRedrawEvent(event);
+        if (handle) {
+            this.handleRedrawEvent(event);
+        }
+    }
+
+    private handleRedrawEvent({ name, args }: EventBusData<"redraw">) {
         switch (name) {
             case "cmdline_show": {
                 const [content, _pos, firstc, prompt, _indent, level] = args[0];
@@ -187,11 +205,19 @@ export class CommandLineManager implements Disposable {
         if (this.state.ignoreHideEvent) {
             logger.debug("onHide: skipping event");
             this.state.ignoreHideEvent = false;
-            return;
+        } else {
+            logger.debug("onHide: entering <ESC>");
+            await this.main.client.input("<Esc>");
         }
 
-        logger.debug("onHide: entering <ESC>");
-        await this.main.client.input("<Esc>");
+        const batch = this.queue.flushBatch();
+        if (batch !== null) {
+            logger.debug("onHide: flushing events");
+            batch.forEach((event) => {
+                // Process the events we we're waiting for
+                this.handleRedrawEvent(event);
+            });
+        }
     };
 
     private onSelection = async (e: readonly QuickPickItem[]): Promise<void> => {


Test patch:
diff --git a/src/test/integ/command-line.test.ts b/src/test/integ/command-line.test.ts
index cd002aa..d670f0c 100644
--- a/src/test/integ/command-line.test.ts
+++ b/src/test/integ/command-line.test.ts
@@ -10,6 +10,7 @@ import {
     closeNvimClient,
     openTextDocument,
     assertContent,
+    sendNeovimKeys,
 } from "./integrationUtils";
 
 describe("Command line", () => {
@@ -23,6 +24,9 @@ describe("Command line", () => {
     });
 
     it("Navigates history", async () => {
+        // Clear history so external history does not affect this test
+        await sendNeovimKeys(client, ":lua vim.fn.histdel(':')<CR>");
+
         await openTextDocument({ content: "abc" });
 
         await sendVSCodeKeys(":");
@@ -152,4 +156,72 @@ describe("Command line", () => {
             client,
         );
     });
+
+    // #2079 - plugins can send cmdline inputs very quickly
+    it("Should not insert text into the buffer if nvim sends two :s very quickly", async () => {
+        await openTextDocument({ content: "some text" });
+        // Once the feedkeys is executed, we will immediately hit :
+        //
+        // the 'ch' in "echo" will put us into insert mode. Broken code would insert " 'hello, world'"
+        // NOTE: the string concat of "<" . "CR>" is deliberate, to prevent nvim from sending a <CR> too early.
+        await sendNeovimKeys(client, String.raw`:call feedkeys(":echo 'hello, world'<" . "CR>")<CR>`);
+        await assertContent(
+            {
+                content: ["some text"],
+            },
+            client,
+        );
+    });
+
+    it("Should not hide the input for an incomplete command if nvim sends two :s very quickly", async () => {
+        await openTextDocument({ content: "some text" });
+
+        // We do this with sendNeovimKeys so everything is done quickly
+        // A bad implementation will close the command line too early before the commit-cmdline takes effect
+        await sendNeovimKeys(client, String.raw`:call feedkeys(":call setline(1, 'hello, world')")<CR>`);
+        await sendVSCodeCommand("vscode-neovim.commit-cmdline");
+        await assertContent(
+            {
+                content: ["hello, world"],
+            },
+            client,
+        );
+    });
+
+    it("Should allow finishing a command that was set up via neovim inputs", async () => {
+        await openTextDocument({ content: "some text" });
+
+        // Set up the command. This could be done by something like a plugin or a mapped key
+        await sendNeovimKeys(client, String.raw`:call setline(1, 'hello, world'`);
+        await sendVSCodeCommand("vscode-neovim.test-cmdline", ")");
+        await sendVSCodeCommand("vscode-neovim.commit-cmdline");
+
+        await assertContent(
+            {
+                content: ["hello, world"],
+            },
+            client,
+        );
+    });
+
+    it("Should allow finishing a command that was set up via neovim inputs, even if it was aborted", async () => {
+        await openTextDocument({ content: "some text" });
+        // Set up the command. This could be done by something like a plugin or a mapped key
+        // Put in the command and press esc, so that it's the "last" command, even though we abort
+        await sendNeovimKeys(client, String.raw`:call setline(1, 'hello, world'`);
+        await sendNeovimKeys(client, String.raw`<ESC>`);
+
+        // Set up the command again
+        await sendNeovimKeys(client, String.raw`:call setline(1, 'hello, world'`);
+        // A user presses )
+        await sendVSCodeCommand("vscode-neovim.test-cmdline", ")");
+        await sendVSCodeCommand("vscode-neovim.commit-cmdline");
+
+        await assertContent(
+            {
+                content: ["hello, world"],
+            },
+            client,
+        );
+    });
 });
diff --git a/src/test/unit/cmdline/cmdline_queue.test.ts b/src/test/unit/cmdline/cmdline_queue.test.ts
new file mode 100644
index 0000000..6ff55dc
--- /dev/null
+++ b/src/test/unit/cmdline/cmdline_queue.test.ts
@@ -0,0 +1,175 @@
+import { strict as assert } from "assert";
+
+import { CmdlineQueue } from "../../../cmdline/cmdline_queue";
+import { type EventBusData } from "../../../eventBus";
+
+describe("handleNvimEvent", () => {
+    it("returns true for cmdline_show/cmdline_hide events inserted at first", () => {
+        const queue = new CmdlineQueue();
+
+        const events: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "wq"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        events.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), true, `Event ${i} did not return true`);
+        });
+    });
+
+    it("returns false for subsequent cmdline_shows given after a cmdline_hide", () => {
+        const queue = new CmdlineQueue();
+
+        const batch1: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        const batch2: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+        ];
+
+        batch1.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), true, `Event ${i} did not return true`);
+        });
+
+        batch2.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), false, `Event ${i} did not return false`);
+        });
+    });
+
+    it("should stage the event for when flushBatch is called", () => {
+        const queue = new CmdlineQueue();
+
+        const batch1: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        const batch2: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+        ];
+
+        batch1.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), true, `Event ${i} did not return true`);
+        });
+
+        batch2.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), false, `Event ${i} did not return false`);
+        });
+
+        assert.deepEqual(queue.flushBatch(), batch2);
+    });
+
+    it("does not return false after cmdline_hides for level changes", () => {
+        const queue = new CmdlineQueue();
+
+        const batch1: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, "=", "", 0, 2]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        const batch2: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, "value"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        batch1.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), true, `Event ${i} did not return true`);
+        });
+
+        batch2.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), true, `Event ${i} did not return true`);
+        });
+    });
+
+    it("should not consider a lone cmdline_hide as a trigger for a queue", () => {
+        const queue = new CmdlineQueue();
+
+        queue.handleNvimRedrawEvent({ name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] });
+        // Flush the batch externally. Would occur w/o a cmdline_hide if someone hid the quickpick from vscode
+        queue.flushBatch();
+
+        queue.handleNvimRedrawEvent({ name: "cmdline_hide" as const, args: undefined });
+        // A defective implementation would return false here, as it would indicate events are now being queued
+        assert.equal(
+            queue.handleNvimRedrawEvent({ name: "cmdline_show" as const, args: [[[[{}, "value"]], 0, ":", "", 0, 1]] }),
+            true,
+        );
+    });
+});
+
+describe("flushBatch", () => {
+    it("returns null when nothing is queued", () => {
+        const queue = new CmdlineQueue();
+        assert.equal(queue.flushBatch(), null);
+    });
+
+    it("returns null after staged events are flushed", () => {
+        const queue = new CmdlineQueue();
+        const events: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+        ];
+
+        events.forEach((event) => {
+            queue.handleNvimRedrawEvent(event);
+        });
+
+        assert.notEqual(queue.flushBatch(), null);
+        assert.equal(queue.flushBatch(), null);
+    });
+
+    it("allows queueing of events once a flush occurs", () => {
+        const queue = new CmdlineQueue();
+        const events: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        events.forEach((event) => {
+            queue.handleNvimRedrawEvent(event);
+        });
+
+        queue.flushBatch();
+
+        events.forEach((event, i) => {
+            assert.equal(queue.handleNvimRedrawEvent(event), true, `Event ${i} did not return true`);
+        });
+    });
+
+    it("should flush more than one batch", () => {
+        const queue = new CmdlineQueue();
+        const events: EventBusData<"redraw">[] = [
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+            { name: "cmdline_show" as const, args: [[[[{}, ""]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_show" as const, args: [[[[{}, "w"]], 0, ":", "", 0, 1]] },
+            { name: "cmdline_hide" as const, args: undefined },
+        ];
+
+        events.forEach((event) => {
+            queue.handleNvimRedrawEvent(event);
+        });
+
+        assert.notEqual(queue.flushBatch(), null, "Batch 0 did not flush");
+        assert.notEqual(queue.flushBatch(), null, "Batch 1 did not flush");
+        assert.notEqual(queue.flushBatch(), null, "Batch 2 did not flush");
+        assert.equal(queue.flushBatch(), null, "More than 3 batches");
+    });
+});