[PATCH] D66735: [clangd] Handling text editor/document lifetimes in vscode extension.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 217131.
jvikstrom added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66735/new/

https://reviews.llvm.org/D66735

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts


Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===================================================================
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -155,5 +155,11 @@
         highlighter.getDecorationRanges('file1'),
         createHighlightingScopeRanges(
             [ highlightingsInLine1, ...highlightingsInLine.slice(1) ]));
+    // Closing a text document removes all highlightings for the file and no
+    // other files.
+    highlighter.removeHighlightings('file1');
+    assert.deepEqual(highlighter.getDecorationRanges('file1'), []);
+    assert.deepEqual(highlighter.getDecorationRanges('file2'),
+                     createHighlightingScopeRanges([ highlightingsInLine1 ]));
   });
 });
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===================================================================
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -89,6 +89,13 @@
     // highlighter being created.
     this.highlighter = new Highlighter(this.scopeLookupTable);
     this.loadCurrentTheme();
+    // Event handling for handling with TextDocuments/Editors lifetimes.
+    vscode.window.onDidChangeVisibleTextEditors(
+        (editors: vscode.TextEditor[]) =>
+            editors.forEach((e) => this.highlighter.applyHighlights(
+                                e.document.uri.toString())));
+    vscode.workspace.onDidCloseTextDocument(
+        (doc) => this.highlighter.removeHighlightings(doc.uri.toString()));
   }
 
   handleNotification(params: SemanticHighlightingParams) {
@@ -150,12 +157,8 @@
       };
       return vscode.window.createTextEditorDecorationType(options);
     });
-    this.getVisibleTextEditorUris().forEach((fileUri) => {
-      // A TextEditor might not be a cpp file. So we must check we have
-      // highlightings for the file before applying them.
-      if (this.files.has(fileUri))
-        this.applyHighlights(fileUri);
-    })
+    this.getVisibleTextEditorUris().forEach((fileUri) =>
+                                                this.applyHighlights(fileUri));
   }
 
   // Adds incremental highlightings to the current highlightings for the file
@@ -171,6 +174,13 @@
     this.applyHighlights(fileUri);
   }
 
+  // Called when a text document is closed. Removes any highlighting entries for
+  // the text document that was closed.
+  public removeHighlightings(fileUri: string) {
+    // If there exists no entry the call to delete just returns false.
+    this.files.delete(fileUri);
+  }
+
   // Gets the uris as strings for the currently visible text editors.
   protected getVisibleTextEditorUris(): string[] {
     return vscode.window.visibleTextEditors.map((e) =>
@@ -180,6 +190,11 @@
   // Returns the ranges that should be used when decorating. Index i in the
   // range array has the decoration type at index i of this.decorationTypes.
   protected getDecorationRanges(fileUri: string): vscode.Range[][] {
+    if (!this.files.has(fileUri))
+      // this.files should always have an entry for fileUri if we are here. But
+      // if there isn't one we don't want to crash the extension. This is also
+      // useful for tests.
+      return [];
     const lines: SemanticHighlightingLine[] =
         Array.from(this.files.get(fileUri).values());
     const decorations: vscode.Range[][] = this.decorationTypes.map(() => []);
@@ -193,8 +208,12 @@
     return decorations;
   }
 
-  // Applies all the highlightings currently stored for a file with fileUri.
-  protected applyHighlights(fileUri: string) {
+  // Applies all the highlightings currently stored for a file with fileUri. If
+  public applyHighlights(fileUri: string) {
+    if (!this.files.has(fileUri))
+      // There are no highlightings for this file, must return early or will get
+      // out of bounds when applying the decorations below.
+      return;
     if (!this.decorationTypes.length)
       // Can't apply any decorations when there is no theme loaded.
       return;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66735.217131.patch
Type: text/x-patch
Size: 4262 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190826/11a37215/attachment-0001.bin>