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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
