Implement external buffer document provider (#1443)

* feat(buffer): add external buffer doc provider

This opens a document with a proper buffer name and matches it to the
appropriate neovim external buffer.

The document is "readonly" as far as VSCode is concerned, but it can
still be edited by setting `modifiable`, so we still need to sync
buffer changes whenever it updates. This can break highlighting, however.

* fix(buffer): scroll to selection on buffer open

We already set the selection based on the neovim cursor from the
external buffer, but we should also scroll so that it's visible. This might
be better fixed with proper syncing like #1261 but for now it makes :help work
a lot nicer.

* refactor(buffer): improve logs, change URI scheme

* fix(buffer): escape external buffer name on Windows

Building a vscode.Uri throws an exception if the `path` component
doesn't match a certain expected format, which Windows file paths do
not. Uri.file() properly escapes/converts file paths as needed.

* fix(test): end nvim client connection gracefully

Instead of forcefully destroying the client connection, it's nicer to
let it end with FIN first, allowing the server to gracefully close. This
prevents a lot of noisy errors that make it hard to read test output.

* fix(buffer): notify doc change after buffer event

Ensure we notify for document change after other buffer event
processing. Enforcing order like this  seems to help with asynchronous
ordering / timing issues on Windows.

* fix(buffer): catch error on external buffer open

src/buffer_manager.ts

@@ -16,7 +16,10 @@ import {
     Uri,
     ViewColumn,
     window,
+    TextDocumentContentProvider,
     workspace,
+    EventEmitter,
+    TextEditorRevealType,
 } from "vscode";
 
 import { Logger } from "./logger";
@@ -37,6 +40,8 @@ const LOG_PREFIX = "BufferManager";
 
 const BUFFER_NAME_PREFIX = "__vscode_neovim__-";
 
+const BUFFER_SCHEME = "vscode-neovim";
+
 /**
  * Manages neovim buffers and windows and maps them to vscode editors & documents
  */
@@ -72,6 +77,10 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
      * Tab configuration for each editor
      */
     private editorTabConfiguration: WeakMap<TextEditor, { tabSize: number; insertSpaces: boolean }> = new WeakMap();
+    /**
+     * Provider for external buffers' document contents (e.g. `:help`)
+     */
+    private bufferProvider: BufferProvider;
 
     /**
      * Buffer event delegate
@@ -97,6 +106,9 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
         this.disposables.push(window.onDidChangeActiveTextEditor(this.onDidChangeActiveTextEditor));
         this.disposables.push(workspace.onDidCloseTextDocument(this.onDidCloseTextDocument));
         this.disposables.push(window.onDidChangeTextEditorOptions(this.onDidChangeEditorOptions));
+
+        this.bufferProvider = new BufferProvider(logger, client, this.receivedBufferEvent);
+        this.disposables.push(workspace.registerTextDocumentContentProvider(BUFFER_SCHEME, this.bufferProvider));
     }
 
     public dispose(): void {
@@ -233,7 +245,7 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
                 }
                 const id = parseInt(idStr, 10);
                 if (!(name && this.isVscodeUriName(name))) {
-                    this.logger.debug(`${LOG_PREFIX}: Attaching new external buffer: ${name}, id: ${id}`);
+                    this.logger.debug(`${LOG_PREFIX}: Attaching new external buffer: '${name}', id: ${id}`);
                     if (id === 1) {
                         this.logger.debug(`${LOG_PREFIX}: ${id} is the first neovim buffer, skipping`);
                         return;
@@ -587,6 +599,16 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
         more: boolean,
     ): void => {
         this.onBufferEvent && this.onBufferEvent(buffer.id, tick, firstLine, lastLine, linedata, more);
+        // Ensure the receivedBufferEvent callback finishes before we fire
+        // the event notifying the doc provider of any changes
+        (async () => {
+            const uri = this.buildExternalBufferUri(await buffer.name, buffer.id);
+            this.logger.debug(`${LOG_PREFIX}: received buffer event for ${uri}`);
+            this.bufferProvider.documentDidChange.fire(uri);
+            return uri;
+        })().then(undefined, (e) => {
+            this.logger.error(`${LOG_PREFIX}: failed to notify document change: ${e}`);
+        });
     };
 
     /**
@@ -721,29 +743,32 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
         return workspace.textDocuments.find((d) => d.uri.toString() === uri);
     }
 
+    private buildExternalBufferUri(name: string, id: number): Uri {
+        // These might not *always* be file names, but they often are (e.g. for :help) so
+        // make sure we properly convert slashes for the path component, especially on Windows
+        return Uri.file(name).with({ scheme: BUFFER_SCHEME, authority: id.toString() });
+    }
+
     private async attachNeovimExternalBuffer(
         name: string,
         id: number,
         expandTab: boolean,
         tabStop: number,
     ): Promise<void> {
-        const buffers = await this.client.buffers;
-        const buf = buffers.find((b) => b.id === id);
-        if (!buf) {
-            return;
-        }
-        // don't bother with displaying empty buffer
-        const lines = await buf.lines;
-        if (!lines.length || (lines.length === 1 && !lines[0])) {
-            this.logger.debug(`${LOG_PREFIX}: Skipping empty external buffer ${id}`);
+        const uri = this.buildExternalBufferUri(name, id);
+        this.logger.debug(`${LOG_PREFIX}: opening external buffer ${uri}`);
+
+        let doc: TextDocument;
+        try {
+            doc = await workspace.openTextDocument(uri);
+        } catch (error) {
+            this.logger.debug(`${LOG_PREFIX}: unable to open external buffer: ${error}`);
             return;
         }
-        const doc = await workspace.openTextDocument({ content: lines.join("\n") });
+
         this.externalTextDocuments.add(doc);
         this.textDocumentToBufferId.set(doc, id);
         this.onBufferInit && this.onBufferInit(id, doc);
-        buf.listen("lines", this.receivedBufferEvent);
-        await buf[ATTACH](true);
 
         const windows = await this.client.windows;
         let closeWinId = 0;
@@ -787,7 +812,10 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
                     } catch (e) {
                         this.logger.warn(`${LOG_PREFIX}: Unable to get cursor pos for external buffer: ${id}`);
                     }
-                    editor.selections = [new Selection(finalLine, finalCol, finalLine, finalCol)];
+
+                    const selection = new Selection(finalLine, finalCol, finalLine, finalCol);
+                    editor.selections = [selection];
+                    editor.revealRange(selection, TextEditorRevealType.AtTop);
                 }
             }, 1000);
 
@@ -808,3 +836,46 @@ export class BufferManager implements Disposable, NeovimRedrawProcessable, Neovi
         }
     }
 }
+
+/**
+ * Implements the VSCode document provider API for external buffers from neovim.
+ */
+class BufferProvider implements TextDocumentContentProvider {
+    /**
+     * Fire this event to update the document contents (i.e. re-evaluate the provider).
+     */
+    public documentDidChange: EventEmitter<Uri> = new EventEmitter();
+
+    onDidChange = this.documentDidChange.event;
+
+    public constructor(
+        private logger: Logger,
+        private client: NeovimClient,
+        private receivedBufferEvent: BufferManager["receivedBufferEvent"],
+    ) {}
+
+    async provideTextDocumentContent(uri: Uri, token: CancellationToken): Promise<string | undefined> {
+        this.logger.debug(`${LOG_PREFIX}: trying to provide content for ${uri}`);
+
+        const id = parseInt(uri.authority, 10);
+
+        const buffers = await this.client.buffers;
+        const buf = buffers.find((b) => b.id === id);
+        if (!buf || token.isCancellationRequested) {
+            this.logger.debug(`${LOG_PREFIX}: external buffer ${id} not found`);
+            return;
+        }
+
+        // don't bother with displaying empty buffer
+        const lines = await buf.lines;
+        if (!lines.length || (lines.length === 1 && !lines[0])) {
+            this.logger.debug(`${LOG_PREFIX}: Skipping empty external buffer ${id}`);
+            return;
+        }
+
+        buf.listen("lines", this.receivedBufferEvent);
+        await buf[ATTACH](true);
+
+        return lines.join("\n");
+    }
+}

src/test/runTest.ts

@@ -13,7 +13,12 @@ async function main(): Promise<void> {
         const extensionTestsPath = path.resolve(__dirname, "./suite/index");
 
         // Download VS Code, unzip it and run the integration test
-        await runTests({ extensionDevelopmentPath, extensionTestsPath });
+        await runTests({
+            extensionDevelopmentPath,
+            extensionTestsPath,
+            // Tell vscode-neovim to create a debug connection
+            extensionTestsEnv: { NEOVIM_DEBUG: "1" },
+        });
     } catch (err) {
         console.error(err);
         console.error("Failed to run tests");

src/test/suite/index.ts

@@ -5,8 +5,6 @@ import glob from "glob";
 import "source-map-support/register";
 
 export async function run(): Promise<void> {
-    // Tell vscode-neovim to create a debug connection
-    process.env.NEOVIM_DEBUG = "1";
     // Create the mocha test
     const mocha = new Mocha({
         ui: "bdd",

src/test/utils.ts

@@ -57,9 +57,14 @@ export async function attachTestNvimClient(): Promise<NeovimClient> {
 export async function closeNvimClient(client: NeovimClient): Promise<void> {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const conn: net.Socket = (client as any).testConn;
-    conn.destroy();
-    // client.quit();
+
+    // Try to gracefully close the socket first, this prevents noisy errors if it works.
+    // The Neovim server seems well-behaved normally and will close the connection.
+    conn.end();
+    // After giving the server some time to respond for graceful shutdown,
     await wait(500);
+    // destroy the connection forcefully if it hasn't already been closed.
+    conn.resetAndDestroy();
 }
 
 export async function getCurrentBufferName(client: NeovimClient): Promise<string> {