Conform to new Semantic colorization system #1905
Conversation
ryanbrandenburg
requested review from
ajaybhargavb,
NTaylorMullen and
TanayParikh
as code owners
| @@ -56,77 +56,26 @@ | |||
| "description": "A Razor TagHelper Attribute" | |||
| }, | |||
| { | |||
| "id": "razorTagHelperTransition", | |||
| "description": "A Razor TagHelper transition" | |||
| "id": "razorTransition", | |||
There was a problem hiding this comment.
These didn't actually match what we had on the LSP side.
| "description": "A colon between directive attribute parameters" | ||
| } | ||
| ], | ||
| "semanticTokenStyleDefaults": [ |
There was a problem hiding this comment.
They removed this when it left the proposed API state.
| "dark": { | ||
| "foreground": "#C586C0" | ||
| "scopes": { | ||
| "razorTagHelperElement": [ "entity.name.class.element.taghelper" ], |
There was a problem hiding this comment.
We now have to adhere to existing scopes to get our pretty colors. Still working on also getting bold for element and attribute.
| @@ -58,7 +58,7 @@ export class HtmlTagCompletionProvider { | |||
|
|
|||
| private async onDidChangeTextDocument( | |||
| document: vscode.TextDocument, | |||
| changes: vscode.TextDocumentContentChangeEvent[]) { | |||
| changes: ReadonlyArray<vscode.TextDocumentContentChangeEvent>) { | |||
There was a problem hiding this comment.
Reacting to API changes.
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/ProposedApisFeature.ts
Outdated
Show resolved
Hide resolved
| private languageServiceClient: RazorLanguageServiceClient, | ||
| // @ts-ignore | ||
| private logger: RazorLogger, | ||
| ) { | ||
| } | ||
|
|
||
| public async register(vscodeType: typeof vscodeapi, localRegistrations: vscode.Disposable[]) { | ||
| if (vscodeType.env.appName.endsWith('Insiders')) { |
There was a problem hiding this comment.
I wanted to leave this logic in even though it does nothing so that there's less to re-work when we add the next proposed feature.
| @@ -64,7 +64,7 @@ export class RazorCompletionItemProvider | |||
| (range as any).replacing = new vscode.Range(replacingRangeStart, replacingRangeEnd); | |||
| } | |||
|
|
|||
| if (range.start && range.end) { | |||
| if (range instanceof vscode.Range && range.start && range.end) { | |||
There was a problem hiding this comment.
For type-checking purposes.
|
|
||
| if (semanticTokenResponse) { | ||
| // However we're serializing into Uint32Array doesn't set byteLength, which is checked by some stuff under the covers. | ||
| // Solution? Create a new one, blat it over the old one, go home for the weekend. | ||
| semanticTokenResponse.data = new Uint32Array(semanticTokenResponse.data); |
There was a problem hiding this comment.
.data became read-only.
| @@ -19,51 +19,6 @@ declare module 'vscode' { | |||
|
|
|||
| //#region Semantic tokens: https://github.com/microsoft/vscode/issues/86415 | |||
|
|
|||
| export class SemanticTokensLegend { | |||
There was a problem hiding this comment.
All changes here and to vscodeAdapter.ts are just making sure our local "fake" API matches theirs.
| languageServiceClient, | ||
| logger); | ||
| if (legend) { | ||
| localRegistrations.push(vscodeType.languages.registerDocumentSemanticTokensProvider(RazorLanguage.id, semanticTokenProvider, legend)); |
There was a problem hiding this comment.
I think the LanguageClient that we use for our LangaugeServer does all this now. Meaning if we move our language server to use the appropriate endpoints / broadcast the appropriate capabilities I think we don't need to do any client side registering like this:
The language client hookup for semanticness: https://github.com/microsoft/vscode-languageserver-node/blob/4c0963d6fdfd657044092c5d4db61a87b54471ef/testbed/client/src/extension.ts#L51-L52
Extra details on their impl of the languageClient feature: https://github.com/microsoft/vscode-languageserver-node/blob/aa6592ee4b4e0276a2e7c8974e19dcdf7b0a61f9/client/src/semanticTokens.proposed.ts
There was a problem hiding this comment.
We talked about this offline. Right now I'm kind of thinking it's not worth it to integrate "officially" because none of the libraries we use yet have official support for it, and it's not even in the official LSP spec yet (listed as "Upcoming"), so we'd have to hack together our own capabilities which would eventually be thrown away. I'm going to come at this again when my brain is a bit fresher on Monday and see if I still feel the same way. If we DO decide not to map all the capabilities and what not yet we should prefix our endpoints to make sure they don't collide (so textDocument/SemanticTokens would be _ms_/textDocument/SemanticTokens), and I should communicate that to our VS counterparts.
There was a problem hiding this comment.
We're going with the ms option for now due to the spec for this still being in flux.
ryanbrandenburg
force-pushed
the
rybrande/Colorization
branch
2 times, most recently
from
2ef3b0e to
4f7a155
Compare
|
🆙📅 If I could get re-review from those who aren't green yet I'll try to get this merged in so I don't block @captainsafia's insertion PR. |
| @@ -10,7 +10,7 @@ | |||
| "enableProposedApi": true, | |||
There was a problem hiding this comment.
Do we still need this?
| @@ -18,7 +18,7 @@ | |||
| "@types/mocha": "^5.2.6", | |||
| "@types/node": "^9.4.7", | |||
| "@types/rimraf": "2.0.2", | |||
| "@types/vscode": "^1.30.0", | |||
| "@types/vscode": "^1.45.1", | |||
There was a problem hiding this comment.
Should we pin the dependency version here instead of using ^?
ryanbrandenburg
force-pushed
the
rybrande/Colorization
branch
from
06b1c24 to
75931b5
Compare
ryanbrandenburg
added
the
auto-merge
Fixes https://github.com/dotnet/aspnetcore/issues/21713.
Some of this was changed when the API left the proposed state, so it's a bit more involved than I originally thought. I'm still consulting with the VSCode folk on some finer points (mostly seeing how we can get the colors we want AND bold text).