[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 10 inline comments as done.
Closed by commit rL369893: [clangd] Added a colorizer to the vscode extension.
(authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66219?vs=217099=217115#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66219/new/
https://reviews.llvm.org/D66219
Files:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -1,5 +1,6 @@
import * as vscode from 'vscode';
import * as vscodelc from 'vscode-languageclient';
+import * as semanticHighlighting from './semantic-highlighting';
/**
* Method to get workspace configuration option
@@ -108,6 +109,17 @@
const clangdClient = new ClangdLanguageClient('Clang Language Server',
serverOptions, clientOptions);
+ const semanticHighlightingFeature =
+ new semanticHighlighting.SemanticHighlightingFeature();
+ clangdClient.registerFeature(semanticHighlightingFeature);
+ // The notification handler must be registered after the client is ready or
+ // the client will crash.
+ clangdClient.onReady().then(
+ () => clangdClient.onNotification(
+ semanticHighlighting.NotificationType,
+ semanticHighlightingFeature.handleNotification.bind(
+ semanticHighlightingFeature)));
+
console.log('Clang Language Server is now active!');
context.subscriptions.push(clangdClient.start());
context.subscriptions.push(vscode.commands.registerCommand(
Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -34,6 +34,13 @@
// The TextMate scope index to the clangd scope lookup table.
scopeIndex: number;
}
+// A line of decoded highlightings from the data clangd sent.
+export interface SemanticHighlightingLine {
+ // The zero-based line position in the text document.
+ line: number;
+ // All SemanticHighlightingTokens on the line.
+ tokens: SemanticHighlightingToken[];
+}
// Language server push notification providing the semantic highlighting
// information for a text document.
@@ -47,8 +54,8 @@
// The TextMate scope lookup table. A token with scope index i has the scopes
// on index i in the lookup table.
scopeLookupTable: string[][];
- // The rules for the current theme.
- themeRuleMatcher: ThemeRuleMatcher;
+ // The object that applies the highlightings clangd sends.
+ highlighter: Highlighter;
fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
// Extend the ClientCapabilities type and add semantic highlighting
// capability to the object.
@@ -61,9 +68,10 @@
}
async loadCurrentTheme() {
-this.themeRuleMatcher = new ThemeRuleMatcher(
+const themeRuleMatcher = new ThemeRuleMatcher(
await loadTheme(vscode.workspace.getConfiguration('workbench')
.get('colorTheme')));
+this.highlighter.initialize(themeRuleMatcher);
}
initialize(capabilities: vscodelc.ServerCapabilities,
@@ -76,10 +84,18 @@
if (!serverCapabilities.semanticHighlighting)
return;
this.scopeLookupTable = serverCapabilities.semanticHighlighting.scopes;
+// Important that highlighter is created before the theme is loading as
+// otherwise it could try to update the themeRuleMatcher without the
+// highlighter being created.
+this.highlighter = new Highlighter(this.scopeLookupTable);
this.loadCurrentTheme();
}
- handleNotification(params: SemanticHighlightingParams) {}
+ handleNotification(params: SemanticHighlightingParams) {
+const lines: SemanticHighlightingLine[] = params.lines.map(
+(line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
+this.highlighter.highlight(params.textDocument.uri, lines);
+ }
}
// Converts a string of base64 encoded tokens into the corresponding array of
@@ -101,6 +117,100 @@
return retTokens;
}
+// The main class responsible for processing of highlightings that clangd
+// sends.
+export class Highlighter {
+ // Maps uris with currently open TextDocuments to the current highlightings.
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
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
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom added inline comments.
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) {
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.
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
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.
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},
hokein wrote:
> 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`.
>
Started exporting the `SemanticHighlightingLine` interface and use that format
in the tests to not create more interfaces (also means we don't need a helper
for creating the SemanticHighlightingLine).
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
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom updated this revision to Diff 217099.
jvikstrom marked 12 inline comments as done.
jvikstrom added a comment.
Made tests more readable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66219/new/
https://reviews.llvm.org/D66219
Files:
clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -1,13 +1,15 @@
import * as assert from 'assert';
import * as path from 'path';
+import * as vscode from 'vscode';
-import * as SM from '../src/semantic-highlighting';
+import * as semanticHighlighting from '../src/semantic-highlighting';
suite('SemanticHighlighting Tests', () => {
test('Parses arrays of textmate themes.', async () => {
const themePath =
path.join(__dirname, '../../test/assets/includeTheme.jsonc');
-const scopeColorRules = await SM.parseThemeFile(themePath);
+const scopeColorRules =
+await semanticHighlighting.parseThemeFile(themePath);
const getScopeRule = (scope: string) =>
scopeColorRules.find((v) => v.scope === scope);
assert.equal(scopeColorRules.length, 3);
@@ -32,8 +34,9 @@
{character : 10, scopeIndex : 0, length : 1}
]
];
-testCases.forEach((testCase, i) => assert.deepEqual(
- SM.decodeTokens(testCase), expected[i]));
+testCases.forEach(
+(testCase, i) => assert.deepEqual(
+semanticHighlighting.decodeTokens(testCase), expected[i]));
});
test('ScopeRules overrides for more specific themes', () => {
const rules = [
@@ -44,7 +47,7 @@
{scope : 'storage', foreground : '5'},
{scope : 'variable.other.parameter', foreground : '6'},
];
-const tm = new SM.ThemeRuleMatcher(rules);
+const tm = new semanticHighlighting.ThemeRuleMatcher(rules);
assert.deepEqual(tm.getBestThemeRule('variable.other.cpp').scope,
'variable.other');
assert.deepEqual(tm.getBestThemeRule('storage.static').scope,
@@ -57,4 +60,103 @@
assert.deepEqual(tm.getBestThemeRule('variable.other.parameter.cpp').scope,
'variable.other.parameter');
});
+ test('Colorizer groups decorations correctly', async () => {
+// Helper for creating a vscode Range.
+const createRange = (line: number, startCharacter: number,
+ length: number) =>
+new vscode.Range(new vscode.Position(line, startCharacter),
+ new vscode.Position(line, startCharacter + length));
+const scopeTable = [
+ [ 'variable' ], [ 'entity.type.function' ],
+ [ 'entity.type.function.method' ]
+];
+// Create the scope source ranges the highlightings should be highlighted
+// at. Assumes the scopes used are the ones in the "scopeTable" variable.
+const createHighlightingScopeRanges =
+(highlightingLines:
+ semanticHighlighting.SemanticHighlightingLine[]) => {
+ // Initialize the scope ranges list to the correct size. Otherwise
+ // scopes that don't have any highlightings are missed.
+ let scopeRanges: vscode.Range[][] = scopeTable.map(() => []);
+ highlightingLines.forEach((line) => {
+line.tokens.forEach((token) => {
+ scopeRanges[token.scopeIndex].push(
+ createRange(line.line, token.character, token.length));
+});
+ });
+ return scopeRanges;
+};
+
+const rules = [
+ {scope : 'variable', foreground : '1'},
+ {scope : 'entity.type', foreground : '2'},
+];
+class MockHighlighter extends semanticHighlighting.Highlighter {
+ applicationUriHistory: string[] = [];
+ // Override to make the highlighting calls accessible to the test. Also makes the test not depend on visible text editors.
+ applyHighlights(fileUri: string) {
+this.applicationUriHistory.push(fileUri);
+ }
+ // Override to make it accessible from the test.
+ getDecorationRanges(fileUri: string) {
+return super.getDecorationRanges(fileUri);
+ }
+ // Override to make tests not depend on visible text editors.
+ getVisibleTextEditorUris() { return [ 'a', 'b' ]; }
+}
+const highlighter = new MockHighlighter(scopeTable);
+const tm = new semanticHighlighting.ThemeRuleMatcher(rules);
+// Recolorizes when initialized.
+highlighter.highlight('a', []);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a' ]);
+
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom updated this revision to Diff 216624.
jvikstrom added a comment.
Added missing protected and comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66219/new/
https://reviews.llvm.org/D66219
Files:
clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -1,5 +1,6 @@
import * as assert from 'assert';
import * as path from 'path';
+import * as vscode from 'vscode';
import * as SM from '../src/semantic-highlighting';
@@ -57,4 +58,71 @@
assert.deepEqual(tm.getBestThemeRule('variable.other.parameter.cpp').scope,
'variable.other.parameter');
});
+ test('Colorizer groups decorations correctly', async () => {
+// Helper for creating a vscode Range.
+const createRange = (line: number, startCharacter: number,
+ length: number) =>
+new vscode.Range(new vscode.Position(line, startCharacter),
+ new vscode.Position(line, startCharacter + length));
+const scopeTable = [
+ [ 'variable' ], [ 'entity.type.function' ],
+ [ 'entity.type.function.method' ]
+];
+const rules = [
+ {scope : 'variable', foreground : '1'},
+ {scope : 'entity.type', foreground : '2'},
+];
+class MockHighlighter extends SM.Highlighter {
+ applicationUriHistory: string[] = [];
+ applyHighlights(fileUri: string) {
+this.applicationUriHistory.push(fileUri);
+ }
+ getDecorationRanges(fileUri: string) {
+return super.getDecorationRanges(fileUri);
+ }
+ getVisibleTextEditorUris() { return [ 'a', 'b' ]; }
+}
+const highlighter = new MockHighlighter(scopeTable);
+const tm = new SM.ThemeRuleMatcher(rules);
+// Recolorizes when initialized.
+highlighter.highlight('a', []);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a' ]);
+highlighter.initialize(tm);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a', 'a' ]);
+// Groups decorations into the scopes used.
+let line = [
+ {character : 1, length : 2, scopeIndex : 1},
+ {character : 5, length : 2, scopeIndex : 1},
+ {character : 10, length : 2, scopeIndex : 2}
+];
+highlighter.highlight(
+'a', [ {line : 1, tokens : line}, {line : 2, tokens : line} ]);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a', 'a', 'a' ]);
+assert.deepEqual(highlighter.getDecorationRanges('a'), [
+ [],
+ [
+createRange(1, 1, 2), createRange(1, 5, 2), createRange(2, 1, 2),
+createRange(2, 5, 2)
+ ],
+ [ createRange(1, 10, 2), createRange(2, 10, 2) ],
+]);
+// Keeps state separate between files.
+highlighter.highlight('b', [
+ {line : 1, tokens : [ {character : 1, length : 1, scopeIndex : 0} ]}
+]);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a', 'a', 'a', 'b' ]);
+assert.deepEqual(highlighter.getDecorationRanges('b'),
+ [ [ createRange(1, 1, 1) ], [], [] ]);
+// Does full colorizations.
+highlighter.highlight('a', [
+ {line : 1, tokens : [ {character : 2, length : 1, scopeIndex : 0} ]}
+]);
+assert.deepEqual(highlighter.applicationUriHistory,
+ [ 'a', 'a', 'a', 'b', 'a' ]);
+assert.deepEqual(highlighter.getDecorationRanges('a'), [
+ [ createRange(1, 2, 1) ],
+ [ createRange(2, 1, 2), createRange(2, 5, 2) ],
+ [ createRange(2, 10, 2) ],
+]);
+ });
});
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -34,6 +34,13 @@
// The TextMate scope index to the clangd scope lookup table.
scopeIndex: number;
}
+// A line of decoded highlightings from the data clangd sent.
+interface SemanticHighlightingLine {
+ // The zero-based line position in the text document.
+ line: number;
+ // All SemanticHighlightingTokens on the line.
+ tokens: SemanticHighlightingToken[];
+}
// Language server push notification providing the semantic highlighting
// information for a text document.
@@ -49,6 +56,8 @@
scopeLookupTable: string[][];
// The rules for the current theme.
themeRuleMatcher: ThemeRuleMatcher;
+ // The object that applies the highlightings clangd sends.
+
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom added a comment. I had completely forgotten we could just override the applyHighlightings method in the tests, everything is much simpler now. Basically this entire CL was rewritten just now. 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
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom updated this revision to Diff 216621.
jvikstrom marked 6 inline comments as done.
jvikstrom added a comment.
Rewrote the Highlighter class as we can override the highlighting method for
the tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66219/new/
https://reviews.llvm.org/D66219
Files:
clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -1,5 +1,6 @@
import * as assert from 'assert';
import * as path from 'path';
+import * as vscode from 'vscode';
import * as SM from '../src/semantic-highlighting';
@@ -57,4 +58,71 @@
assert.deepEqual(tm.getBestThemeRule('variable.other.parameter.cpp').scope,
'variable.other.parameter');
});
+ test('Colorizer groups decorations correctly', async () => {
+// Helper for creating a vscode Range.
+const createRange = (line: number, startCharacter: number,
+ length: number) =>
+new vscode.Range(new vscode.Position(line, startCharacter),
+ new vscode.Position(line, startCharacter + length));
+const scopeTable = [
+ [ 'variable' ], [ 'entity.type.function' ],
+ [ 'entity.type.function.method' ]
+];
+const rules = [
+ {scope : 'variable', foreground : '1'},
+ {scope : 'entity.type', foreground : '2'},
+];
+class MockHighlighter extends SM.Highlighter {
+ applicationUriHistory: string[] = [];
+ applyHighlights(fileUri: string) {
+this.applicationUriHistory.push(fileUri);
+ }
+ getDecorationRanges(fileUri: string) {
+return super.getDecorationRanges(fileUri);
+ }
+ getVisibleTextEditorUris() { return [ 'a', 'b' ]; }
+}
+const highlighter = new MockHighlighter(scopeTable);
+const tm = new SM.ThemeRuleMatcher(rules);
+// Recolorizes when initialized.
+highlighter.highlight('a', []);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a' ]);
+highlighter.initialize(tm);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a', 'a' ]);
+// Groups decorations into the scopes used.
+let line = [
+ {character : 1, length : 2, scopeIndex : 1},
+ {character : 5, length : 2, scopeIndex : 1},
+ {character : 10, length : 2, scopeIndex : 2}
+];
+highlighter.highlight(
+'a', [ {line : 1, tokens : line}, {line : 2, tokens : line} ]);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a', 'a', 'a' ]);
+assert.deepEqual(highlighter.getDecorationRanges('a'), [
+ [],
+ [
+createRange(1, 1, 2), createRange(1, 5, 2), createRange(2, 1, 2),
+createRange(2, 5, 2)
+ ],
+ [ createRange(1, 10, 2), createRange(2, 10, 2) ],
+]);
+// Keeps state separate between files.
+highlighter.highlight('b', [
+ {line : 1, tokens : [ {character : 1, length : 1, scopeIndex : 0} ]}
+]);
+assert.deepEqual(highlighter.applicationUriHistory, [ 'a', 'a', 'a', 'b' ]);
+assert.deepEqual(highlighter.getDecorationRanges('b'),
+ [ [ createRange(1, 1, 1) ], [], [] ]);
+// Does full colorizations.
+highlighter.highlight('a', [
+ {line : 1, tokens : [ {character : 2, length : 1, scopeIndex : 0} ]}
+]);
+assert.deepEqual(highlighter.applicationUriHistory,
+ [ 'a', 'a', 'a', 'b', 'a' ]);
+assert.deepEqual(highlighter.getDecorationRanges('a'), [
+ [ createRange(1, 2, 1) ],
+ [ createRange(2, 1, 2), createRange(2, 5, 2) ],
+ [ createRange(2, 10, 2) ],
+]);
+ });
});
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -34,6 +34,13 @@
// The TextMate scope index to the clangd scope lookup table.
scopeIndex: number;
}
+// A line of decoded highlightings from the data clangd sent.
+interface SemanticHighlightingLine {
+ // The zero-based line position in the text document.
+ line: number;
+ // All SemanticHighlightingTokens on the line.
+ tokens: SemanticHighlightingToken[];
+}
// Language server push notification providing the semantic highlighting
// information for a text document.
@@ -49,6 +56,8 @@
scopeLookupTable: string[][];
// The rules for the current theme.
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
hokein added inline comments.
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:134
+ // The decoration datatypes used last time highlight was called.
+ private oldDecorations: DecorationRangePair[] = [];
+ // Apply decorations to the textEditor with uri and remove the old ones.
I'm not convinced that we need this interface.
With the current implementation (create new TextEditorDecorationTypes every
time we do a re-highlighting), we'd need to save the old decorations in order
to dispose them when applying new decorations.
but if we create the TextEditorDecorationTypes at Initialization and reuse them
in re-highlighting, then we probably don't need to depose them manually, the
vscode
[API](https://code.visualstudio.com/api/references/vscode-api#TextEditor.setDecorations)
seems do that for us, so I think we'd better to do that in this patch as it
would simplify the current implementation.
So the APIs would looks like below. In unittest, we just override
applyDecorations method.
```
class Highlighter {
public Initialize(themeRuleMatcher: ThemeRuleMatcher) {
// build a scopeIndex => TextEditorDecorationTypes table
}
public highlight(fileUri: string, highlightings: SemanticHighlightingLine[]) {
// update and patch the highlighting cache.
// invoke getDecorations
// invoke applyDecorations
}
protect applyDecorations(decorations...) {
// invoke vscode API to apply the decorations.
}
private getDecorations(hightligtings...) {
}
```
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:176
+ // recolorization for all current highlighters.
+ public updateThemeRuleMatcher(themeRuleMatcher: ThemeRuleMatcher) {
+this.themeRuleMatcher = themeRuleMatcher;
I'd name it `Initialize`.
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:182
+ // Called when clangd sends an incremental update of highlightings.
+ public setFileLines(uriString: string, tokens: SemanticHighlightingLine[]) {
+// Patch in the new highlightings to the highlightings cache. If this is
the
the name of this public API is confusing, the name indicates it just **sets**
the data, but actually it does more stuff like doing the highlightings.
I think we may get rid of this function, by just exposing the highlight API.
see my above comment.
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:199
+ // Applies all highlightings to the file with uri uriString.
+ private highlight(uriString: string) {
+if (!this.highlighters.has(uriString)) {
could we name it fileUri? and elsewhere
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:217
+Array.from(this.files.get(uriString).values())
+.forEach((line) => lines.push(line));
+// Maps scopeIndexes -> the DecorationRangePair used for the scope.
nit: I think we can just do ` const lines: SemanticHighlightingLine[] =
Array.from(this.files.get(uriString).values())`?
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:219
+// Maps scopeIndexes -> the DecorationRangePair used for the scope.
+const decorations: Map = new Map();
+lines.forEach((line) => {
nit: pull out this part into a new method e.g. `getDecorations`
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
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom updated this revision to Diff 215886.
jvikstrom marked 4 inline comments as done.
jvikstrom added a comment.
Renamed colorizer to highlighter and added FIXME about highlightings below eof.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66219/new/
https://reviews.llvm.org/D66219
Files:
clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -1,5 +1,6 @@
import * as assert from 'assert';
import * as path from 'path';
+import * as vscode from 'vscode';
import * as SM from '../src/semantic-highlighting';
@@ -57,4 +58,72 @@
assert.deepEqual(tm.getBestThemeRule('variable.other.parameter.cpp').scope,
'variable.other.parameter');
});
+ test('Colorizer groups decorations correctly', () => {
+const colorizations: {uri: string, decorations: any[]}[] = [];
+// Mock of a colorizer that saves the parameters in the colorizations array.
+class MockFileColorizer extends SM.FileHighlighter {
+ public highlight(uri: string, decorationRangePairs: any[]) {
+colorizations.push({uri : uri, decorations : decorationRangePairs});
+ }
+ public dispose() {}
+}
+// Helper for creating a vscode Range.
+const createRange = (line: number, startCharacter: number,
+ length: number) =>
+new vscode.Range(new vscode.Position(line, startCharacter),
+ new vscode.Position(line, startCharacter + length));
+const scopeTable = [
+ [ 'variable' ], [ 'entity.type.function' ],
+ [ 'entity.type.function.method' ]
+];
+const rules = [
+ {scope : 'variable', foreground : '1'},
+ {scope : 'entity.type', foreground : '2'},
+];
+const tm = new SM.ThemeRuleMatcher(rules);
+const colorizer = new SM.Highlighter(MockFileColorizer, scopeTable);
+// No colorization if themeRuleMatcher has not been set.
+colorizer.setFileLines('a', []);
+assert.deepEqual(colorizations, []);
+colorizer.updateThemeRuleMatcher(tm);
+assert.deepEqual(colorizations, [ {decorations : [], uri : 'a'} ]);
+// Groups decorations into the scopes used.
+let line = [
+ {character : 1, length : 2, scopeIndex : 1},
+ {character : 5, length : 2, scopeIndex : 1},
+ {character : 10, length : 2, scopeIndex : 2}
+];
+colorizer.setFileLines(
+'a', [ {line : 1, tokens : line}, {line : 2, tokens : line} ]);
+assert.equal(colorizations[1].uri, 'a');
+assert.equal(colorizations[1].decorations.length, 2);
+// Can't test the actual decorations as vscode does not seem to have an api
+// for getting the actual decoration objects.
+assert.deepEqual(colorizations[1].decorations[0].ranges, [
+ createRange(1, 1, 2), createRange(1, 5, 2), createRange(2, 1, 2),
+ createRange(2, 5, 2)
+]);
+assert.deepEqual(colorizations[1].decorations[1].ranges,
+ [ createRange(1, 10, 2), createRange(2, 10, 2) ]);
+// Keeps state separate between files.
+colorizer.setFileLines('b', [
+ {line : 1, tokens : [ {character : 1, length : 1, scopeIndex : 0} ]}
+]);
+assert.equal(colorizations[2].uri, 'b');
+assert.equal(colorizations[2].decorations.length, 1);
+assert.deepEqual(colorizations[2].decorations[0].ranges,
+ [ createRange(1, 1, 1) ]);
+// Does full colorizations.
+colorizer.setFileLines('a', [
+ {line : 1, tokens : [ {character : 2, length : 1, scopeIndex : 0} ]}
+]);
+assert.equal(colorizations[3].uri, 'a');
+assert.equal(colorizations[3].decorations.length, 3);
+assert.deepEqual(colorizations[3].decorations[0].ranges,
+ [ createRange(1, 2, 1) ]);
+assert.deepEqual(colorizations[3].decorations[1].ranges,
+ [ createRange(2, 1, 2), createRange(2, 5, 2) ]);
+assert.deepEqual(colorizations[3].decorations[2].ranges,
+ [ createRange(2, 10, 2) ]);
+ });
});
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -34,6 +34,13 @@
// The TextMate scope index to the clangd scope lookup table.
scopeIndex: number;
}
+// A line of decoded highlightings from the data clangd sent.
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
hokein added inline comments.
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:144
+// sends.
+export class Colorizer {
+ private files: Map> = new
Map();
The generic `T` doesnt seem to be used?
Regarding the name, the word `colorization` is from vscode cpp-tools extension,
I think in clangd we should keep using `Hightlight` for consistency, maybe use
`Highlighter`?
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:145
+export class Colorizer {
+ private files: Map> = new
Map();
+ private colorizers: Map = new Map();
could you add some documentation for these fields?
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:146
+ private files: Map> = new
Map();
+ private colorizers: Map = new Map();
+ private themeRuleMatcher: ThemeRuleMatcher;
It seems that, all files are sharing with duplicated implementations in this
patch, storing duplications is wasteful.
Maybe we could make the `colorize` method as a virtual method -- in the
unittest, we create a subclass of `Colorizer` and override the `colorize`
method.
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:164
+ public setFileLines(uriString: string, tokens: SemanticHighlightingLine[]) {
+// Patch in the new highlightings to the highlightings cache. If this is
the
+// first time the file should be highlighted a new colorizer and a file
we are patching the incremental diff to get a "global" highlighting for the
whole file, but it maybe not correct -- clangd doesn't emit diff outside of the
max line number of the file, if we delete a few lines at the end of the file,
then we will keep the removed lines in the map.
I think this is a current limitation, could you check with the current behavior
of vscode if we pass a out-of-range to the decoration API?
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
[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.
jvikstrom created this revision.
jvikstrom added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.
Adds the main colorizer component. It colorizes every time clangd sends a
publishSemanticHighlighting notification.
Every time it colorizes it does a full recolorization (removes all decorations
from the editor and applies new ones). The reason for this is that all ranges
for the same scope share a TextEditorDecorationType. This is due to
TextEditorDecorationTypes being very expensive to create. The prototype used
one DecorationType per range but that ran into very big performance problems
(it took >100 ms to apply 600 lines of highlightings which froze the editor).
This version does not share the problem of being extremly slow, but there is
probably potential to optimize it even more.
The Colorizer uses a FileColorizer interface to make it possible to mock out
all the code that applies colorizations to a specific editor so that we can
test it.
No document/texteditor lifecycle management code in this CL, that will come in
the next one.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66219
Files:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -1,5 +1,6 @@
import * as assert from 'assert';
import * as path from 'path';
+import * as vscode from 'vscode';
import * as SM from '../src/semantic-highlighting';
@@ -57,4 +58,73 @@
assert.deepEqual(tm.getBestThemeRule('variable.other.parameter.cpp').scope,
'variable.other.parameter');
});
+ test('Colorizer groups decorations correctly', () => {
+const colorizations: {uri: string, decorations: any[]}[] = [];
+// Mock of a colorizer that saves the parameters in the colorizations array.
+class MockFileColorizer {
+ public colorize(uri: string, decorationRangePairs: any[]) {
+colorizations.push({uri : uri, decorations : decorationRangePairs});
+ }
+ public dispose() {}
+}
+// Helper for creating a vscode Range.
+const createRange = (line: number, startCharacter: number,
+ length: number) =>
+new vscode.Range(new vscode.Position(line, startCharacter),
+ new vscode.Position(line, startCharacter + length));
+const scopeTable = [
+ [ 'variable' ], [ 'entity.type.function' ],
+ [ 'entity.type.function.method' ]
+];
+const rules = [
+ {scope : 'variable', foreground : '1'},
+ {scope : 'entity.type', foreground : '2'},
+];
+const tm = new SM.ThemeRuleMatcher(rules);
+const colorizer =
+new SM.Colorizer(MockFileColorizer, scopeTable);
+// No colorization if themeRuleMatcher has not been set.
+colorizer.setFileLines('a', []);
+assert.deepEqual(colorizations, []);
+colorizer.updateThemeRuleMatcher(tm);
+assert.deepEqual(colorizations, [ {decorations : [], uri : 'a'} ]);
+// Groups decorations into the scopes used.
+let line = [
+ {character : 1, length : 2, scopeIndex : 1},
+ {character : 5, length : 2, scopeIndex : 1},
+ {character : 10, length : 2, scopeIndex : 2}
+];
+colorizer.setFileLines(
+'a', [ {line : 1, tokens : line}, {line : 2, tokens : line} ]);
+assert.equal(colorizations[1].uri, 'a');
+assert.equal(colorizations[1].decorations.length, 2);
+// Can't test the actual decorations as vscode does not seem to have an api
+// for getting the actual decoration objects.
+assert.deepEqual(colorizations[1].decorations[0].ranges, [
+ createRange(1, 1, 2), createRange(1, 5, 2), createRange(2, 1, 2),
+ createRange(2, 5, 2)
+]);
+assert.deepEqual(colorizations[1].decorations[1].ranges,
+ [ createRange(1, 10, 2), createRange(2, 10, 2) ]);
+// Keeps state separate between files.
+colorizer.setFileLines('b', [
+ {line : 1, tokens : [ {character : 1, length : 1, scopeIndex : 0} ]}
+]);
+assert.equal(colorizations[2].uri, 'b');
+assert.equal(colorizations[2].decorations.length, 1);
+assert.deepEqual(colorizations[2].decorations[0].ranges,
+ [ createRange(1, 1, 1) ]);
+// Does full colorizations.
+colorizer.setFileLines('a', [
+ {line : 1, tokens : [ {character : 2, length : 1, scopeIndex : 0} ]}
+]);
+assert.equal(colorizations[3].uri, 'a');
+assert.equal(colorizations[3].decorations.length, 3);
+
