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