Page MenuHomePhabricator

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

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #215112)

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 ↗(On Diff #215112)

could you add some documentation for these fields?

146 ↗(On Diff #215112)

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 ↗(On Diff #215112)

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
134 ↗(On Diff #215886)

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...) {
}
176 ↗(On Diff #215886)

I'd name it Initialize.

182 ↗(On Diff #215886)

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.

199 ↗(On Diff #215886)

could we name it fileUri? and elsewhere

217 ↗(On Diff #215886)

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

219 ↗(On Diff #215886)

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
128 ↗(On Diff #216624)

nit: mention the index is the scopeIndex?

135 ↗(On Diff #216624)

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.

137 ↗(On Diff #216624)

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.

139 ↗(On Diff #216624)

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?

157 ↗(On Diff #216624)

nit: tokens => HighlightingLines.

190 ↗(On Diff #216624)

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 ↗(On Diff #216624)

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

93 ↗(On Diff #216624)

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

94 ↗(On Diff #216624)

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
135 ↗(On Diff #216624)

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.

139 ↗(On Diff #216624)

vscode indeed does not do anything on empty colors.

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
94 ↗(On Diff #216624)

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
135 ↗(On Diff #216624)

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

139 ↗(On Diff #216624)

could you add a comment for this?

58 ↗(On Diff #217099)

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

170 ↗(On Diff #217099)

I think we should comment this method as normal methods.

171 ↗(On Diff #217099)

nit: spell the return type.

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
84 ↗(On Diff #217099)

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

105 ↗(On Diff #217099)

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

108 ↗(On Diff #217099)

nit: inline the rules here.

139 ↗(On Diff #217099)

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

160 ↗(On Diff #217099)

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