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

[jvikstrom]

Summary

Just reapplies highlightings for all files when visible text editors change. Could find the correct text editor that should be reapplied but going for a simple implementation instead.
Removes the associated highlighting entry from the Colorizer when a text document is closed.

[hokein]

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

94

I think we can do it like vscode.window.onDidChangeVisibleTextEditors((TextEditor[] editors) => { // call applyHighlight for each text editor }).

179

the name is a bit weird onDidCloseTextDocument in the Highlighter, I would name it removeFileHighlights

[jvikstrom]

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

94

There might be TextEditors that are not a c++ file/haven't gotten highlightings yet.

So we'd still need to do the this.files.has(fileUri) check which we also do in the initialize function.

Maybe I should just expose the reapplyAllHighlightings function and call that one instead?

[hokein]

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

94

if the file is not a c++, the current implementation of applyHighlights is tolerant with that I think, it doesn't do anything (we get an empty decoration ranges).

I think we can do the this.files.has(fileUri) check in applyHighlights.

[jvikstrom]

Address comments.

[jvikstrom]

Fixed changes that weren't supposed to be made.

[hokein]

I think there is one more case -- we need to cleanup the highlighting cache if clangd crashes, the extension will automatically restart clangd up to 5 times if it sees clangd crashes, you can see how filestatus handles it.

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

177

I think remove all highlightings for the file with the given fileUri. is enough.

212–216

nit: the trailing If?

213

could you move this method after highlight?

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

initialize(capabilities: vscodelc.ServerCapabilities,
     if (!serverCapabilities.semanticHighlighting)
       return;
     this.scopeLookupTable = serverCapabilities.semanticHighlighting.scopes;
     // Important that highlighter is created before the theme is loading as
     // otherwise it could try to update the themeRuleMatcher without the
     // 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.removeFileHighlightings(doc.uri.toString()));
   }
 
   handleNotification(params: SemanticHighlightingParams) {
     const lines: SemanticHighlightingLine[] = params.lines.map(
         (line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
     this.highlighter.highlight(params.textDocument.uri, lines);
   }
 }
this.decorationTypes = this.scopeLookupTable.map((scopes) => {
         // empty decorations.
         color : themeRuleMatcher.getBestThemeRule(scopes[0]).foreground,
         // If the rangeBehavior is set to Open in any direction the
         // highlighting becomes weird in certain cases.
         rangeBehavior : vscode.DecorationRangeBehavior.ClosedClosed,
       };
       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
   // with fileUri. Also applies the highlightings to any associated
   // TextEditor(s).
   public highlight(fileUri: string,
                    highlightingLines: SemanticHighlightingLine[]) {
     if (!this.files.has(fileUri)) {
       this.files.set(fileUri, new Map());
     }
     const fileHighlightings = this.files.get(fileUri);
     highlightingLines.forEach((line) => fileHighlightings.set(line.line, line));
     this.applyHighlights(fileUri);
   }
 
+  // Called when a text document is closed. Removes any highlighting entries for
+  // the text document that was closed.
+  public removeFileHighlightings(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) =>
                                                     e.document.uri.toString());
   }
 
   // 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(() => []);
     lines.forEach((line) => {
       line.tokens.forEach((token) => {
         decorations[token.scopeIndex].push(new vscode.Range(
             new vscode.Position(line.line, token.character),
             new vscode.Position(line.line, token.character + token.length)));
       });
     });
     return decorations;
   }
 
   // Applies all the highlightings currently stored for a file with fileUri.
-  protected applyHighlights(fileUri: string) {
+  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;
     // This must always do a full re-highlighting due to the fact that
     // TextEditorDecorationType are very expensive to create (which makes
     // incremental updates infeasible). For this reason one
     // TextEditorDecorationType is used per scope.
     const ranges = this.getDecorationRanges(fileUri);

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

test('Colorizer groups decorations correctly', async () => {
     const tm = new semanticHighlighting.ThemeRuleMatcher([
       {scope : 'variable', foreground : '1'},
       {scope : 'entity.type', foreground : '2'},
     ]);
     // Recolorizes when initialized.
     highlighter.highlight('file1', []);
     assert.deepEqual(highlighter.applicationUriHistory, [ 'file1' ]);
     highlighter.initialize(tm);
-    assert.deepEqual(highlighter.applicationUriHistory, [ 'file1', 'file1' ]);
+    assert.deepEqual(highlighter.applicationUriHistory, [ 'file1', 'file1', 'file2' ]);
     // Groups decorations into the scopes used.
     let highlightingsInLine: semanticHighlighting.SemanticHighlightingLine[] = [
       {
         line : 1,
         tokens : [
           {character : 1, length : 2, scopeIndex : 1},
           {character : 10, length : 2, scopeIndex : 2},
         ]
       },
       {
         line : 2,
         tokens : [
           {character : 3, length : 2, scopeIndex : 1},
           {character : 6, length : 2, scopeIndex : 1},
           {character : 8, length : 2, scopeIndex : 2},
         ]
       },
     ];
 
     highlighter.highlight('file1', highlightingsInLine);
     assert.deepEqual(highlighter.applicationUriHistory,
-                     [ 'file1', 'file1', 'file1' ]);
+                     [ 'file1', 'file1', 'file2', 'file1' ]);
     assert.deepEqual(highlighter.getDecorationRanges('file1'),
                      createHighlightingScopeRanges(highlightingsInLine));
     // Keeps state separate between files.
     const highlightingsInLine1:
         semanticHighlighting.SemanticHighlightingLine = {
       line : 1,
       tokens : [
         {character : 2, length : 1, scopeIndex : 0},
       ]
     };
     highlighter.highlight('file2', [ highlightingsInLine1 ]);
     assert.deepEqual(highlighter.applicationUriHistory,
-                     [ 'file1', 'file1', 'file1', 'file2' ]);
+                     [ 'file1', 'file1', 'file2', 'file1', 'file2' ]);
     assert.deepEqual(highlighter.getDecorationRanges('file2'),
                      createHighlightingScopeRanges([ highlightingsInLine1 ]));
     // Does full colorizations.
     highlighter.highlight('file1', [ highlightingsInLine1 ]);
     assert.deepEqual(highlighter.applicationUriHistory,
-                     [ 'file1', 'file1', 'file1', 'file2', 'file1' ]);
+                     [ 'file1', 'file1', 'file2', 'file1', 'file2', 'file1' ]);
     // After the incremental update to line 1, the old highlightings at line 1
     // will no longer exist in the array.
     assert.deepEqual(
         highlighter.getDecorationRanges('file1'),
         createHighlightingScopeRanges(
             [ highlightingsInLine1, ...highlightingsInLine.slice(1) ]));
+    // Closing a text document removes all highlightings for the file and no
+    // other files.
+    highlighter.removeFileHighlightings('file1');
+    assert.deepEqual(highlighter.getDecorationRanges('file1'), []);
+    assert.deepEqual(highlighter.getDecorationRanges('file2'),
+                     createHighlightingScopeRanges([ highlightingsInLine1 ]));
   });
 });