VS: Some further QuickInfo improvements

[majocha]

• Exposed FSharpSymbol optionally in ToolTipElementData record, to get accurate icons for quickinfo parts.

before:

after:

• Put descriptions and their xml docs in correct order, with correct icons:

before:

after:

• Correct formatting for keywords info:

before:

after:

• Nicer spacing overall, double line breaks collapsed into smaller gaps:

before:

after:

I removed a lot of code that was putting together signature and implementation docs in quickinfo. I think in light of #14711 and #14920 that is no longer needed and any such functionality should belong to the service, not the client anyway.

[majocha]

This needs some cleanup, probably fixing tests / baselines. If this looks acceptable I'll follow up in this PR.

[psfinaki]

@majocha this looks promising!

I few thoughts:

1. You would help reviewers if you put some side-by-side screenshots of what has changed so that it's easy to compare
2. If you are going to remove a lot of code, that's awesome - I suggest doing this (as much as possible) in a separate PR so that this move can be better evaluated
3. I have recently fixed the localization files, please rebase so that they don't appear in this PR.

Thanks for your effort, all the recent quick info changes are amazing, we might want to write a blog post about them. :)

[majocha]

Added some before screenshots 🙂

[majocha]

I'm wondering what to do with the tests. The problem is, as they stand now, they only effectively exercise the GetToolTip method from the compiler service, and not the actual editor part. OTOH there's a lot of them and they are valuable.

[psfinaki]

Well the whole editor testing is quite sad now and we break a lot of testing principles and guidelines there, what you've noticed is an example of this.

But yeah, the tests are still somewhat valuable and catch a bunch of regressions, so that's another reason why I'd keep refactoring and cleanup as a separate effort.

---

On another note, what's special about Mac in regards to the tooltips?

Also, I wouldn't break the line after the parameter name, e.g. see sharp here:

[majocha]

On another note, what's special about Mac in regards to the tooltips?

I don't know the details as I've never seen VS on mac, but at some point there was an effort to make the rendering of VS visuals cross platform. From that time come all the ContainerElements and ClassifiedTextRuns that are in Views.fs. They are high level abstractions later translated into WPF elements on Windows and something else on mac.

[majocha]

Also, I wouldn't break the line after the parameter name, e.g. see sharp here:

That's probably happening in XMLDocumentation.fs. I really don't want to touch that file more than I did because stuff in there is hair-rising 😀.

Aah, that value shouldn't be there at all because it belongs to signature help , I'll look into it.

[psfinaki]

Thanks :) As for MacOS, good to know that we have at least some efforts for some special handling there.

[majocha]

I salvaged the tests.
This is just to test if it's green. I'll reduce the diff later.

[majocha]

OK reduced diff, this is good when green.

[majocha]

Do we want to show parameters?

Previously it was:

With parameters now:

We can follow up to make it configurable, but I'll probably restore the old behavior for now.

[majocha]

Also fixed old ugly hack that was plain wrong code and sometimes interfered with nice text wrapping.

[majocha]

This is green, so I update the branch and it is ready.

[psfinaki]

I'm taking a close look into the PR. As for showing the parameters - there are two ways to go:

1. Either keeping things as they are whatever they are
2. Or making things consistent with something

My personal stance would be not showing them for the following reasons:

1. Good parameters names don't require descriptions and it's nice to encourage this
2. If there are more than a few, it might create a visual cognitive overload
3. Not all the parameters are equal - e.g. optional parameters might want a different visualization

But - this is just my personal view, personal preferences are somewhat secondary here and yeah ideally we should have this configurable.

On the consistency note, C# doesn't show the parameters:

F# currently behaves in the same manner and it's fine I think.

[majocha]

@psfinaki, exactly my thinking. Params are hidden again now.

[psfinaki]

So I left a few comments - mostly some higher-level questions for future reference.

Overall, I am for merging this, now it's a good time to get this in and try this out.

[psfinaki]

Alright, LGTM, merging.

@majocha thanks!

[vzarytovskii]

We have CPP VS perf tests failing by latest insertion. This is the only pr which touched VS, might be related to changes in tooltip styling, forcing VS to load us in non-F# projects.

Something similar happened before:.
#3045

I am reverting the insertion right now, we will investigate further next week.

[vzarytovskii]

FSharp.Core is loaded when C++ is showing a tooltip in

 microsoft.visualstudio.vc.ni! VCCompletion.SetTooltipContentFromJson.

It uses standard Microsoft.VisualStudio.Text.Adornments.ClassifiedTextElement in the content.

These are “view model” classes used to create remotable and crossplat rich tooltip content, and they require IViewElementFactory exported MEF component that can convert them to WPF UIElement for display.

Microsoft.VisualStudio.Text.Adornments.ClassifiedTextElement already has built-in IViewElementFactory in the editor, which uses standard VS theme, but in this pr wr also export our own IViewElementFactory also targeting Microsoft.VisualStudio.Text.Adornments.ClassifiedTextElement, which effectively means we are subscribing to style any ClassifiedTextElement used in any tooltip in any language.

It’s supported by IViewElementFactory, but the expectation is that we should target your own custom “view model”  type instead of standard Microsoft.VisualStudio.Text.Adornments.ClassifiedTextElement in your IViewElementFactory, this way convertor will only be loaded and called when a tooltip to be shown contains that custom type.

[majocha]

Ay, thanks for the detailed write up. This is salvageable, the culprit is fdcbe04.

I'll fix it after the weekend.

[vzarytovskii]

Ay, thanks for the detailed write up. This is salvageable, the culprit is fdcbe04.

I'll fix it after the weekend.

No rush, we can look into it next week too :)
VS is challenging sometimes. Took me some time and talking to people to get this particular issue.

[majocha]

@vzarytovskii
I did some modification and hopefully #15098 fixes this.

VS is challenging sometimes. Took me some time and talking to people to get this particular issue.

Yep, the documentation can be terse sometimes and it's invaluable to have the right people to talk to :)