-
Notifications
You must be signed in to change notification settings - Fork 680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add experimental Semantic Highlighter #3667
Conversation
@ryanbrandenburg since you own our semantic classification you should take a look at this as well. It will also enable us to call through to Roslyn for semantic colorization as well 😄 |
Codecov Report
@@ Coverage Diff @@
## master #3667 +/- ##
==========================================
+ Coverage 87.01% 87.03% +0.02%
==========================================
Files 59 59
Lines 1779 1782 +3
Branches 207 207
==========================================
+ Hits 1548 1551 +3
Misses 176 176
Partials 55 55
Continue to review full report at Codecov.
|
const semanticSpans = response.Spans; | ||
|
||
const builder = new vscode.SemanticTokensBuilder(); | ||
for (let span of semanticSpans) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding the code but why not have the server just return the LSP compatible type vs. re-building it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just following the typescript extension as an example where they request information from their server then perform some post processing. I'll have to think more on whether to keep it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, TypeScript hasn't moved fully over to LSP yet which is why they follow that model. They plan to move fully over to LSP
export default class SemanticTokensProvider extends AbstractProvider implements vscode.DocumentSemanticTokensProvider, vscode.DocumentRangeSemanticTokensProvider { | ||
|
||
getLegend(): vscode.SemanticTokensLegend { | ||
return new vscode.SemanticTokensLegend(tokenTypes, tokenModifiers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server's typically the one that defines the legend. Why re-create it on the client (vs. querying it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah goes back to whether the OmniSharp endpoint should just implement the LSP return types. I'll think on it.
1e2c4d7
to
83ecd67
Compare
@NTaylorMullen Would like to include this in the next release. We'll consider it experimental (defaulted off) and collect feedback about the performance. Ultimately we may implement an API that is closer to the LSP, but that may wait until Roslyn implements a classification service that returns tokens and modifiers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, when that comes to fruition that'll enable us to add semantic C# colorization to Razor's C# pieces. Is that something that's tracked somewhere? FYI @ryanbrandenburg
What type of switch do you plan to put it behind? Asking because our TagHelper semantic highlighting is cheap and is something we'd have "on" all the time however if we were to semantically highlight our C# pieces of the document we'd want it behind a similar switch to ensure it's only active when yours is active. |
89beb40
to
a00eec9
Compare
a00eec9
to
a2fb93e
Compare
We didn't have an exact issue tracking this, so I made a proposal. dotnet/roslyn#44155
There is a |
Although GitHub checks didn't update, this is the passing Travis CI for this PR - https://travis-ci.org/github/OmniSharp/omnisharp-vscode/builds/685827799 Merging |
for good measure this is the passing master build following this merge https://travis-ci.org/github/OmniSharp/omnisharp-vscode/builds/685844034 |
Implementation of the Semantic Tokens provider (microsoft/vscode#86415) requested in #3565. Built on the Semantic Endpoint added to OmniSharp in OmniSharp/omnisharp-roslyn#1734
Future enhancement:
With Textmate Classification:
With Semantic Classification:
With Semantic Classification and Visual Studio 2019 theme tweaks:
Applied theme tweaks: