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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, looks better now. Some more comments on the test code.



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:3
 import * as vscodelc from 'vscode-languageclient';
-
+import * as SM from './semantic-highlighting';
 /**
----------------
I'd name it `semanticHighlighting`


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:128
+  // DecorationTypes for the current theme that are used when highlighting.
+  private decorationTypes: vscode.TextEditorDecorationType[];
+  // The clangd TextMate scope lookup table.
----------------
nit: mention the index is the scopeIndex?


================
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) {
----------------
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.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:137
+  public initialize(themeRuleMatcher: ThemeRuleMatcher) {
+    this.decorationTypes = this.scopeLookupTable.map((scopes) => {
+      const options: vscode.DecorationRenderOptions = {
----------------
if this.decorationsTypes is not empty (when this API is called multiple times), I think we need to call the `dispose` method to release the resource they hold.


================
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
----------------
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?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:157
+  // TextEditor(s).
+  public highlight(fileUri: string, tokens: SemanticHighlightingLine[]) {
+    if (!this.files.has(fileUri)) {
----------------
nit: tokens => HighlightingLines.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:190
+  protected applyHighlights(fileUri: string) {
+    if (!this.decorationTypes)
+      return;
----------------
nit: add a comment, we don't apply highlights when the class is not ready/initialized.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:80
+      }
+      getDecorationRanges(fileUri: string) {
+        return super.getDecorationRanges(fileUri);
----------------
could you add a comment you override this so that it can be access from the test?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:93
+    // Groups decorations into the scopes used.
+    let line = [
+      {character : 1, length : 2, scopeIndex : 1},
----------------
nit: please use a more descriptive name, e.g. `HightlightsInLine`.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:94
+    let line = [
+      {character : 1, length : 2, scopeIndex : 1},
+      {character : 5, length : 2, scopeIndex : 1},
----------------
I believe the test code is correct, but the code here/below is complex and hard to follow. I think we need make the code more readable/understandable.

some suggestions:

- the line number is implicitly indicated by the index of the array, I think we can add one more field `line` to the structure (call HighlightingTokens).
- and creates some helper functions (with proper names) that take the `HighlightingTokens` as parameter and generate the data you need e.g. `SemanticHighlightingLine[]`, `vscode.Range`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66219