Skip to content
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

Introduces a new hover provider, under V2 of the protocol, that uses Roslyn's QuickInfoService #1860

Merged
merged 6 commits into from
Jul 25, 2020

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented Jul 24, 2020

The existing provider uses a custom handler, which this replaces. Among other benefits, this brings nullability display when available, and ensures that any new additions to roslyn's info get propagated to users of this service. Unfortunately, however, TaggedText in VS is significantly more powerful than vscode's hover renderer: that simply uses markdown. Their implementation does not support any extensions to enable C# formatting of code inline, I created a poor-man's substitute: for the description line, we treat the whole line as C#. It does mean there can be a bit of odd formatting with (parameter) or similar, but this exactly mirrors what typescript does so I don't think it's a big deal. For other sections, I picked sections that looked ok when formatted as C# code, and I otherwise did a simple conversion, as best I could, from tagged text to inline markdown.

I've submitted a parallel PR to move vscode to use this provider, and provided some samples of what it looks like there: dotnet/vscode-csharp#3928.

Fixes #1860.

333fred added 2 commits July 23, 2020 19:00
…Roslyn's QuickInfoService

The existing provider uses a custom handler, which this replaces. Among other benefits, this brings nullability display when available, and ensures that any new additions to roslyn's info get propagated to users of this service. Unfortunately, however, TaggedText in VS is significantly more powerful than vscode's hover renderer: that simply uses markdown. Their implementation does not support any extensions to enable C# formatting of code inline, I created a poor-man's substitute: for the description line, we treat the whole line as C#. It does mean there can be a bit of odd formatting with `(parameter)` or similar, but this exactly mirrors what typescript does so I don't think it's a big deal. For other sections, I picked sections that looked ok when formatted as C# code, and I otherwise did a simple conversion, as best I could, from tagged text to inline markdown.
333fred added a commit to 333fred/vscode-csharp that referenced this pull request Jul 24, 2020
@333fred 333fred changed the title quickinfo Introduces a new hover provider, under V2 of the protocol, that uses Roslyn's QuickInfoService Jul 24, 2020
@filipw
Copy link
Member

filipw commented Jul 24, 2020

thanks a lot! I had it on my TODO since it became public in Roslyn 😀

src/OmniSharp.Abstractions/Models/v2/QuickInfoResponse.cs Outdated Show resolved Hide resolved
case QuickInfoSectionKinds.TypeParameters:
return new QuickInfoResponseSection { IsCSharpCode = true, Text = s.Text };

default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we special case any example as well and emit them as full markdown code blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there is an examples section in quickinfo.

src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs Outdated Show resolved Hide resolved
Rewrite quickinfo loop to be fully iterative and not create closures.
Moved the new service to V1.
Simplify the output.
@333fred
Copy link
Contributor Author

333fred commented Jul 24, 2020

@filipw @david-driscoll @mholo65 addressed feedback, and updated the vscode side as well.

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@bjorkstromm
Copy link
Member

Thanks! I've kicked CI since Mac build stalled.

@JoeRobich
Copy link
Member

Thanks @333fred!

@JoeRobich JoeRobich merged commit d3ad9a6 into OmniSharp:master Jul 25, 2020
@333fred 333fred deleted the quickinfo branch July 25, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants