[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Haven't looked at the patch in details.

Looks like the patch is doing different things, and the test just covers a small set of the code.

1. find and parse the theme definition files `json` ;
2. define related data structures to save the TM scopes and do the transformation;
3. handle changes of the theme;

could we narrow it further? I think we could just implement 1) for this patch.



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:11
+  // Mapping from a clangd scope index to a color hex string.
+  private colors: string[];
+  // The current best matching scope for every index.
----------------
The `color` word here is ambiguous -- we have different colors in DecorationRenderOptions, e.g. `color`, `backgroundColor`, `borderColor`, I believe you meant `color`, maybe we could just a more descriptive name, like `highlightColor`, or `TextColor`? 

I think we may want to use a structure here (`color` is one of the field), so that the code is extensible to support more options in  `DecorationRenderOptions`.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:12
+  private colors: string[];
+  // The current best matching scope for every index.
+  private colorScopes: string[];
----------------
what does the `index` mean here?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:119
+// Gets a TextMate theme by its name and all its included themes.
+async function getFullNamedTheme(themeName: string): Promise<any[]> {
+  const extension =
----------------
I think we may define a type/interface (like `TokenColorRule`) for the entries (tokenColors) in the theme file, and have a function that parsing the content and returning an array of `TokenColorRule`?




================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:148
+    const contents = await readFileText(fullPath);
+    const parsed = jsonc.parse(contents);
+    if (parsed.include)
----------------
note that `json` is one of the theme format files, vscode also supports `.tmTheme`. We probably don't support `tmTheme` for now (just add a fixme).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738