[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:134
+  // The decoration datatypes used last time highlight was called.
+  private oldDecorations: DecorationRangePair[] = [];
+  // Apply decorations to the textEditor with uri and remove the old ones.
----------------
I'm not convinced that we need this interface.

With the current implementation (create new TextEditorDecorationTypes every time we do a re-highlighting), we'd need to save the old decorations in order to dispose them when applying new decorations. 

but if we create the TextEditorDecorationTypes at Initialization and reuse them in re-highlighting, then we probably don't need to depose them manually, the vscode [API](https://code.visualstudio.com/api/references/vscode-api#TextEditor.setDecorations) seems do that for us, so I think we'd better to do that in this patch as it would simplify the current implementation. 

So the APIs would looks like below. In unittest, we just override applyDecorations method.

```
class Highlighter {

public Initialize(themeRuleMatcher: ThemeRuleMatcher) {
   // build a scopeIndex => TextEditorDecorationTypes table
}

public highlight(fileUri: string, highlightings: SemanticHighlightingLine[]) {
   // update and patch the highlighting cache.
   // invoke getDecorations
   // invoke applyDecorations
}

protect applyDecorations(decorations...) {
   // invoke vscode API to apply the decorations.
}

private getDecorations(hightligtings...) {
}
``` 


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:176
+  // recolorization for all current highlighters.
+  public updateThemeRuleMatcher(themeRuleMatcher: ThemeRuleMatcher) {
+    this.themeRuleMatcher = themeRuleMatcher;
----------------
I'd name it `Initialize`.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:182
+  // Called when clangd sends an incremental update of highlightings.
+  public setFileLines(uriString: string, tokens: SemanticHighlightingLine[]) {
+    // Patch in the new highlightings to the highlightings cache. If this is the
----------------
the name of this public API is confusing, the name indicates it just **sets**  the data, but actually it does more stuff like doing the highlightings.

I think we may get rid of this function, by just exposing the highlight API. see my above comment.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:199
+  // Applies all highlightings to the file with uri uriString.
+  private highlight(uriString: string) {
+    if (!this.highlighters.has(uriString)) {
----------------
could we name it fileUri? and elsewhere


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:217
+    Array.from(this.files.get(uriString).values())
+        .forEach((line) => lines.push(line));
+    // Maps scopeIndexes -> the DecorationRangePair used for the scope.
----------------
nit: I think we can just do ` const lines: SemanticHighlightingLine[] =   Array.from(this.files.get(uriString).values())`?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:219
+    // Maps scopeIndexes -> the DecorationRangePair used for the scope.
+    const decorations: Map<number, DecorationRangePair> = new Map();
+    lines.forEach((line) => {
----------------
nit: pull out this part into a new method e.g. `getDecorations`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66219