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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

thanks, looks great, looking forward to use it, just a few more nits.

Please update the patch description before committing.



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:135
+  // Update the themeRuleMatcher that is used when highlighting. Also triggers a
+  // recolorization for all current highlighters. Safe to call multiple times.
+  public initialize(themeRuleMatcher: ThemeRuleMatcher) {
----------------
jvikstrom wrote:
> hokein wrote:
> > nit: the comment is stale now.  I believe this function is only called whenever we reload a theme or at the first launch of the extension.
> It's should only called when a theme is loaded (and we happen to load a theme when we initialize, but it's async).
> Don't see why the comment is stale though, it seems to describe what it does to me.
IIRC, the comment here was move from `setThemeRuleMatcher`, but now it's named `initialize`, I'd expect the comment is more specific about *initialization* of the class -- when this method should be called, and what it does (in a high level).


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:139
+      const options: vscode.DecorationRenderOptions = {
+        color : themeRuleMatcher.getBestThemeRule(scopes[0]).foreground,
+        // If the rangeBehavior is set to Open in any direction the
----------------
jvikstrom wrote:
> hokein wrote:
> > just want to confirm: if we fail to find a matched theme rule, we return an empty decoration. I think vscode just doesn't do anything on an empty color?
> vscode indeed does not do anything on empty colors.
could you add a comment for this?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:58
   // The rules for the current theme.
   themeRuleMatcher: ThemeRuleMatcher;
+  // The object that applies the highlightings clangd sends.
----------------
nit: I think we can remove this field since it is not used in this class.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:170
+
+  // Exists to make the initialize method testable.
+  protected getVisibleTextEditorUris() {
----------------
I think we should comment this method as normal methods. 


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:171
+  // Exists to make the initialize method testable.
+  protected getVisibleTextEditorUris() {
+    return vscode.window.visibleTextEditors.map((e) =>
----------------
nit: spell the return type.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:84
+              scopeRanges[token.scopeIndex].push(
+                  createRange(line.line, token.character, token.length));
+            });
----------------
nit: I'd inline the `createRange` here, since we only use it once.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:105
+      // Override to make tests not depend on visible text editors.
+      getVisibleTextEditorUris() { return [ 'a', 'b' ]; }
+    }
----------------
nit: use some readable name, e.g. `file1`, `file2`?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:108
+    const highlighter = new MockHighlighter(scopeTable);
+    const tm = new semanticHighlighting.ThemeRuleMatcher(rules);
+    // Recolorizes when initialized.
----------------
nit: inline the `rules` here.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:139
+    const smallHighlightingsInLine:
+        semanticHighlighting.SemanticHighlightingLine[] = [
+      {
----------------
the array contains a single element, I'd use ` semanticHighlighting.SemanticHighlightingLine` and name it `HighlightingsInLine1`


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:160
+        createHighlightingScopeRanges(
+            [...highlightingsInLine.slice(1), ...smallHighlightingsInLine ]));
+  });
----------------
maybe just `[HighlightingsInLine1, ...highlightingsInLine.slice(1)]`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66219