Quick info glyph by vasily-kirichenko · Pull Request #2028 · dotnet/fsharp

vasily-kirichenko · 2016-12-16T13:33:58Z

WIP

Show proper glyph for each language construct

…lost during one of the merges

vasily-kirichenko · 2016-12-16T17:22:39Z

It's ready

forki · 2016-12-16T17:24:58Z

Amazing. You even fix the details. Hero.

KevinRansom · 2016-12-16T17:26:24Z

@vasily-kirichenko
The CI sees a Build error:
this should repro it:
build vs

 D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\QuickInfoProviderTests.fs(103,52): error FS0001: Type mismatch. Expecting a� 'FSharpToolTipText * TextSpan * Glyph' �but given a� 'FSharpToolTipText * TextSpan' �The tuples have differing lengths of 3 and 2 [D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\VisualFSharp.Unittests.fsproj]

vasily-kirichenko · 2016-12-16T17:51:26Z

@KevinRansom fixed.

KevinRansom · 2016-12-16T17:52:21Z

@vasily-kirichenko
ty

vasily-kirichenko · 2016-12-16T18:40:58Z

Let me know if I should merge with master (when master is green again).

KevinRansom · 2016-12-16T18:50:03Z

@vasily-kirichenko should these use :

SymbolSearchKind.DoesNotIncludeRightColumn
or
SymbolSearchKind.IncludesRightColumn

https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs#L187

And here also
https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/Navigation/FindReferencesService.fs#L70

vasily-kirichenko · 2016-12-16T19:28:37Z

Both should use IncludesRightColumn

KevinRansom · 2016-12-16T19:29:39Z

Yay ... I guessed right :-)

vasily-kirichenko · 2016-12-16T19:33:33Z

:) All features use IncludesRightColumn except Quick Info, because the tooltip appear too early when you move cursor towards a symbol from right side, like this: symbol |, should work like this: symbol|

# Conflicts: # vsintegration/src/FSharp.Editor/CommonHelpers.fs

vasily-kirichenko · 2016-12-16T19:43:55Z

I've merged with master.

KevinRansom · 2016-12-16T21:26:54Z

@dotnet-bot test this please

cartermp · 2016-12-16T22:27:19Z

Love it!

Next step is hooking up to a presenter to get type colors in there. I plan on looking at that over Christmas time. Well, assuming you don't already have it implemented by then :p

Pilchie · 2016-12-16T23:08:14Z

You don't need a custom presenter - you should look at how Roslyn does it here with TaggedText

KevinRansom

other than packages this looks good.

KevinRansom · 2016-12-16T23:25:45Z

 <package id="Microsoft.VisualStudio.Shell.Design" version="14.3.25407" targetFramework="net46" />
 <package id="Microsoft.VisualStudio.Settings.15.0" version="15.0.25824-RC" targetFramework="net46" />
 <package id="Microsoft.VisualStudio.Utilities" version="14.3.25407" targetFramework="net46" />
+ <package id="Microsoft.VisualStudio.Imaging" version="14.3.25407" targetFramework="net46" />


Should you use:
https://www.nuget.org/packages/Microsoft.VisualStudio.Imaging/15.0.25901-RC
and
https://www.nuget.org/packages/Microsoft.VisualStudio.ImageCatalog/15.0.25824-RC

Or perhaps we should get 2.0.0-rc2 versions published

KevinRansom · 2016-12-16T23:27:23Z

 <HintPath>$(FSharpSourcesRoot)\..\packages\Microsoft.VisualStudio.CoreUtility.$(RoslynVSPackagesVersion)\lib\net45\Microsoft.VisualStudio.CoreUtility.dll</HintPath>
 </Reference>
+ <Reference Include="Microsoft.VisualStudio.Imaging, Version=$(RoslynVSBinariesVersion).0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a">
+ <HintPath>$(FSharpSourcesRoot)\..\packages\Microsoft.VisualStudio.Imaging.$(RoslynVSPackagesVersion)\lib\net45\Microsoft.VisualStudio.Imaging.dll</HintPath>


The package path might not be right.

vasily-kirichenko · 2016-12-17T07:19:13Z

@KevinRansom I'm not sure I understand how VS package versioning set up here. Why these references so complicated? https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj#L92-L133

It looks like all VisualStudio.xxx packages are assumed to have exactly same RoslynVSBinariesVersion version:

 <ReferenceInclude="Microsoft.VisualStudio.Editor, Version=$(RoslynVSBinariesVersion).0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a">
<HintPath>$(FSharpSourcesRoot)\..\packages\Microsoft.VisualStudio.Editor.$(RoslynVSPackagesVersion)\lib\net45\Microsoft.VisualStudio.Editor.dll</HintPath>
</Reference>

That's not the case we have currently:

<package id="Microsoft.VisualStudio.LanguageServices" version="2.0.0-rc2" targetFramework="net46" /><package id="Microsoft.Composition" version="1.0.27" targetFramework="net46" /><package id="Microsoft.VisualStudio.Package.LanguageService.14.0" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Editor" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Text.UI" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Text.UI.Wpf" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Text.Data" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Text.Logic" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Threading" version="14.1.131" targetFramework="net46" /><package id="Microsoft.VisualStudio.CoreUtility" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Shell.Design" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Settings.15.0" version="15.0.25824-RC" targetFramework="net46" /><package id="Microsoft.VisualStudio.Utilities" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Language.StandardClassification" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Language.Intellisense" version="14.3.25407" targetFramework="net46" /><package id="Microsoft.VisualStudio.Designer.Interfaces" version="1.1.4322" />

You suggest to update Imaging to RC, so how it should be done? Some packages are available in version 15.0.25901-RC, some are not.

I found this approach hard to maintain. Why just not use VS UI to manage the packages automagically?
(However, I've not managed to get any results trying to gather updated packages with it)

vladima · 2016-12-17T07:33:44Z

getting there...

vasily-kirichenko · 2016-12-17T07:53:01Z

@vladima Oh, cool!

vasily-kirichenko · 2016-12-17T08:54:29Z

OK, I've sorted out the packages versions, now VisualFSharp.sln builds successfully on clean checkout.

dungpa · 2016-12-17T10:11:32Z

@Pilchie @vladima I tried to colorize signature help by changing https://github.com/Microsoft/visualfsharp/blob/ca2ed9bfbec4710802f3782ce8840ec060618347/vsintegration/src/FSharp.Editor/Completion/SignatureHelp.fs#L182-L206 to use TextTags.Class, TextTags.Keyword, etc. However, F# signature helps still have no color.

Any idea on creating a signature help presenter for F#?

vasily-kirichenko · 2016-12-17T10:27:21Z

@dungpa this looks relevant http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices.Next/FindReferences/StreamingFindUsagesPresenter.DocumentLocationEntry.cs,105

dungpa · 2016-12-17T10:39:39Z

I think it's relevant but should not be necessary. The signature help is colorized at http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/Intellisense/SignatureHelp/Presentation/SignatureHelpClassifier.cs,50

The signature help classifier is restricted to C#/VB at http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/Intellisense/SignatureHelp/Presentation/SignatureHelpClassifierProvider.cs,13. What is the easiest way to enable this for F# signature help? /cc @Pilchie @vladima

dungpa · 2016-12-17T11:00:13Z

Ok, I found a way in #2034

vasily-kirichenko · 2016-12-17T16:08:42Z

Tests fail because VisualFSharp.unittests.dll cannot be loaded:

Any ideas how to spot the wrong/missing assembly?

I think that the way packages are managed in this repository is unhealthy. What about switching to Paket?

/cc @forki

vasily-kirichenko · 2016-12-17T18:14:41Z

OK, I reverted the big changes (VS packages ver. 14 -> 15) and it's now simply adds a single Microsoft.VisualStudio.Imaging ver. 14. Same error on loading the test dll :(

vasily-kirichenko · 2016-12-17T21:00:24Z

I'm stuck with this :(

KevinRansom · 2016-12-17T22:26:33Z

@vasily-kirichenko
I can take a look mate.

vasily-kirichenko · 2016-12-18T07:07:13Z

@KevinRansom thanks!

vasily-kirichenko · 2016-12-19T07:18:38Z

@KevinRansomMicrosoft.VisualStudio.Imaging is here for a single reason - we use CryspImage type from it. I've ported this code http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/Intellisense/QuickInfo/DeferredContent/SymbolGlyphDeferredContent.cs,26 as is, if it helps.

BTW, I had to write this unsafe casting:

letmoniker= GlyphExtensions.GetImageMoniker(glyph)letimage=new CrispImage()
image.Moniker <-(box moniker):?>_

because ImageMoniker returned by the extension method is different from CryspImage.Moniker expects (both are identical simple structures, so it works). Maybe it's a hint of why this type loading error occurs.

KevinRansom · 2016-12-19T08:00:04Z

@vasily-kirichenko @Pilchie @brettfo

This happens because the Roslyn dll's do not have InternalsVisible enabled for VisualFSharpTests.dll

https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/QuickInfo/QuickInfoProvider.fs#L73

This guy's async returns a value:

Microsoft.FSharp.Core.FSharpFunc`2[System.Tuple`3[
Microsoft.FSharp.Compiler.SourceCodeServices.FSharpToolTipText,
Microsoft.CodeAnalysis.Text.TextSpan,Microsoft.CodeAnalysis.Glyph],System.String]

Microsoft.CodeAnalysis.Glyph is internal and thus can not be seen from the test dll.

Which means it can't be used in test code unless the test code has Internals Visible enabled for it:

Either:
1.Rewrite to completely isolate Microsoft.CodeAnalysis.Glyph we actually are mostly there already
or ...
2. Modify Roslyn to Make Glyph public
or ...
3. set internals visible to our test assembly.

vasily-kirichenko · 2016-12-19T08:06:01Z

@KevinRansom Thanks! Will try #1.

vasily-kirichenko · 2016-12-19T08:42:08Z

@KevinRansom fixed. I can load and run the tests locally. Phew, that was a tricky nasty stuff. Thanks!

vasily-kirichenko · 2016-12-19T14:13:42Z

@dotnet-bot test this please

KevinRansom · 2016-12-19T17:31:05Z

Hmmm that failure is absolutely not expected.
@dotnet-bot test this please

vasily-kirichenko · 2016-12-19T18:01:22Z

@vladima has almost finished his work on colorful quick info tooltips here master...vladima:vladima/classification which operates on a slightly higher level of abstraction than this PR and it's incompatible with it anyway. So I'm closing this in favor of vladima#1 (or whatever approach @vladima will choose for showing the glyph). Currently it looks like this:

KevinRansom · 2016-12-19T18:02:26Z

Okay ... thanks.

vasily-kirichenko · 2016-12-19T18:04:44Z

@KevinRansom sorry for wasting your time :(

KevinRansom · 2016-12-19T18:06:55Z

@vasily-kirichenko
It's never a waste of my time mate ... and besides, even if it were (which it wasn't) .... you paid me back many thousands of times with all of your awesome IDE PR's. I reckon I owe you tons.

Kevin

Vasily Kirichenko added 3 commits December 16, 2016 12:23

include BlockStructureService.fs into FSharp.Editor project which is …
a77a095
…lost during one of the merges

try to add glyph to quick info
270bab4

glyph appears
4c5c5e8

msftclas added the cla-already-signed label Dec 16, 2016

most glyphs are done
0ccf7d7

fix tests compilation
612bb8e

KevinRansom approved these changes Dec 16, 2016
View reviewed changes

vasily-kirichenko added 2 commits December 16, 2016 22:36

Merge remote-tracking branch 'origin/master' into quick-info-glyph
107b531
# Conflicts: # vsintegration/src/FSharp.Editor/CommonHelpers.fs

fix project file
1e1f0ec

KevinRansom approved these changes Dec 16, 2016
View reviewed changes

Merge remote-tracking branch 'origin/master' into quick-info-glyph
2ef7050

add Microsoft.VisualStudio.Imaging reference to all VS related projects
cf09f94

vasily-kirichenko force-pushed the quick-info-glyph branch from a737570 to cf09f94Compare December 17, 2016 18:11

Merge branch 'master' into quick-info-glyph
a93a705

Merge remote-tracking branch 'origin/master' into quick-info-glyph
ca57b93

Vasily Kirichenko added 2 commits December 19, 2016 11:12

Merge remote-tracking branch 'origin/master' into quick-info-glyph
ee46642

do not use Roslyn internal type in public API
f4e94ef

Merge remote-tracking branch 'origin/master' into quick-info-glyph
fd16ef6

vasily-kirichenko closed this Dec 19, 2016

Uh oh!

Conversation

vasily-kirichenko commented Dec 16, 2016

Uh oh!

vasily-kirichenko commented Dec 16, 2016

Uh oh!

forki commented Dec 16, 2016

Uh oh!

KevinRansom commented Dec 16, 2016

Uh oh!

vasily-kirichenko commented Dec 16, 2016

Uh oh!

KevinRansom commented Dec 16, 2016

Uh oh!

vasily-kirichenko commented Dec 16, 2016

Uh oh!

KevinRansom commented Dec 16, 2016

Uh oh!

vasily-kirichenko commented Dec 16, 2016

Uh oh!

KevinRansom commented Dec 16, 2016

Uh oh!

vasily-kirichenko commented Dec 16, 2016

Uh oh!

vasily-kirichenko commented Dec 16, 2016

Uh oh!

KevinRansom commented Dec 16, 2016

Uh oh!

cartermp commented Dec 16, 2016

Uh oh!

Pilchie commented Dec 16, 2016

Uh oh!

KevinRansom left a comment

Choose a reason for hiding this comment

Uh oh!

KevinRansomDec 16, 2016

Choose a reason for hiding this comment

Uh oh!

KevinRansomDec 16, 2016

Choose a reason for hiding this comment

Uh oh!

KevinRansomDec 16, 2016

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Dec 17, 2016

Uh oh!

vladima commented Dec 17, 2016

Uh oh!

vasily-kirichenko commented Dec 17, 2016

Uh oh!

vasily-kirichenko commented Dec 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dungpa commented Dec 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasily-kirichenko commented Dec 17, 2016

Uh oh!

dungpa commented Dec 17, 2016

Uh oh!

dungpa commented Dec 17, 2016

Uh oh!

vasily-kirichenko commented Dec 17, 2016

Uh oh!

vasily-kirichenko commented Dec 17, 2016

Uh oh!

vasily-kirichenko commented Dec 17, 2016

Uh oh!

KevinRansom commented Dec 17, 2016

Uh oh!

vasily-kirichenko commented Dec 18, 2016

Uh oh!

vasily-kirichenko commented Dec 19, 2016

Uh oh!

KevinRansom commented Dec 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasily-kirichenko commented Dec 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasily-kirichenko commented Dec 17, 2016•
edited
Loading

dungpa commented Dec 17, 2016•
edited
Loading

KevinRansom commented Dec 19, 2016•
edited
Loading

vasily-kirichenko commented Dec 19, 2016•
edited
Loading

vasily-kirichenko commented Dec 19, 2016•
edited
Loading