This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Added a colorizer to the vscode extension.
ClosedPublic

Authored by jvikstrom on Aug 14 2019, 7:03 AM.

Details

Summary

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 too 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.

Event Timeline

jvikstrom created this revision.Aug 14 2019, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 7:03 AM
jvikstrom edited the summary of this revision. (Show Details)Aug 14 2019, 8:08 AM
nridge added a subscriber: nridge.Aug 15 2019, 11:33 AM
hokein added inline comments.Aug 19 2019, 3:05 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
144

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?

145

could you add some documentation for these fields?

146

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.

164

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?

jvikstrom updated this revision to Diff 215886.Aug 19 2019, 7:08 AM
jvikstrom marked 4 inline comments as done.

Renamed colorizer to highlighter and added FIXME about highlightings below eof.

hokein added inline comments.Aug 21 2019, 3:44 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
135

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 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...) {
}
177

I'd name it Initialize.

183

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.

200

could we name it fileUri? and elsewhere

218

nit: I think we can just do const lines: SemanticHighlightingLine[] = Array.from(this.files.get(uriString).values())?

220

nit: pull out this part into a new method e.g. getDecorations

jvikstrom updated this revision to Diff 216621.Aug 22 2019, 7:43 AM
jvikstrom marked 6 inline comments as done.

Rewrote the Highlighter class as we can override the highlighting method for the tests.

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.

jvikstrom updated this revision to Diff 216624.Aug 22 2019, 7:48 AM

Added missing protected and comment.

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

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
3 ↗(On Diff #216624)

I'd name it semanticHighlighting

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
129

nit: mention the index is the scopeIndex?

136

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.

138

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.

140

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?

158

nit: tokens => HighlightingLines.

191

nit: add a comment, we don't apply highlights when the class is not ready/initialized.

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
80

could you add a comment you override this so that it can be access from the test?

93

nit: please use a more descriptive name, e.g. HightlightsInLine.

94

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.
jvikstrom updated this revision to Diff 217099.Aug 26 2019, 2:48 AM
jvikstrom marked 12 inline comments as done.

Made tests more readable.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
136

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.

140

vscode indeed does not do anything on empty colors.

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
94

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

hokein accepted this revision.Aug 26 2019, 4:06 AM

thanks, looks great, looking forward to use it, just a few more nits.

Please update the patch description before committing.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
58–60

nit: I think we can remove this field since it is not used in this class.

136

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

140

could you add a comment for this?

171

I think we should comment this method as normal methods.

172

nit: spell the return type.

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
82

nit: I'd inline the createRange here, since we only use it once.

103

nit: use some readable name, e.g. file1, file2?

106

nit: inline the rules here.

137

the array contains a single element, I'd use semanticHighlighting.SemanticHighlightingLine and name it HighlightingsInLine1

158

maybe just [HighlightingsInLine1, ...highlightingsInLine.slice(1)]?

This revision is now accepted and ready to land.Aug 26 2019, 4:06 AM
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 10 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 4:35 AM