This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Handling text editor/document lifetimes in vscode extension.
ClosedPublic

Authored by jvikstrom on Aug 26 2019, 5:25 AM.

Details

Summary

Just reapplies highlightings for all files when visible text editors change. Could find the correct text editor that should be reapplied but going for a simple implementation instead.
Removes the associated highlighting entry from the Colorizer when a text document is closed.

Diff Detail

Event Timeline

jvikstrom created this revision.Aug 26 2019, 5:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 5:25 AM
hokein added inline comments.Aug 26 2019, 5:49 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
94

I think we can do it like vscode.window.onDidChangeVisibleTextEditors((TextEditor[] editors) => { // call applyHighlight for each text editor }).

179

the name is a bit weird onDidCloseTextDocument in the Highlighter, I would name it removeFileHighlights

jvikstrom marked an inline comment as done.Aug 26 2019, 6:03 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
94

There might be TextEditors that are not a c++ file/haven't gotten highlightings yet.

So we'd still need to do the this.files.has(fileUri) check which we also do in the initialize function.

Maybe I should just expose the reapplyAllHighlightings function and call that one instead?

hokein added inline comments.Aug 26 2019, 6:12 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
94

if the file is not a c++, the current implementation of applyHighlights is tolerant with that I think, it doesn't do anything (we get an empty decoration ranges).

I think we can do the this.files.has(fileUri) check in applyHighlights.

jvikstrom updated this revision to Diff 217131.Aug 26 2019, 6:31 AM

Address comments.

jvikstrom updated this revision to Diff 217134.Aug 26 2019, 6:36 AM

Fixed changes that weren't supposed to be made.

hokein accepted this revision.Aug 26 2019, 6:42 AM

I think there is one more case -- we need to cleanup the highlighting cache if clangd crashes, the extension will automatically restart clangd up to 5 times if it sees clangd crashes, you can see how filestatus handles it.

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

I think remove all highlightings for the file with the given fileUri. is enough.

212–216

nit: the trailing If?

213

could you move this method after highlight?

This revision is now accepted and ready to land.Aug 26 2019, 6:42 AM
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 4 inline comments as done.