Quick info api2 #13623

mattwar · 2016-09-06T18:11:30Z

@DustinCampbell @jasonmalinowski @Pilchie @dpoeschl @CyrusNajmabadi please review

Here is my proposal for the new public QuickInfo API... (note: the API as presented is not yet public.)

This is the "simple" form of the API, not the "almost as complicated as XML" form that I describe a few weeks ago. It has one API to get the QuickInfoItem for a position in document, and that item simply lays out the data/text that we typically display for C# and VB. It also includes information we use in the syntactic c# provider (that shows ranges of the original document where the matching open braces are). The presenter then choses what to do with it. There is no customization of the presenter on the editor side.

If other languages need different information and different presentation, the plan is to add those extra data times to the one QuickInfoItem class and change the single presenter to account for it. Worst case scenario if we decide we want something drastically different in the future, then we invent QuickInfo2.

CyrusNajmabadi · 2016-09-06T18:13:14Z

src/Dependencies/PooledObjects/Microsoft.CodeAnalysis.PooledObjects.shproj

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup Label="Globals">
-    <ProjectGuid>c1930979-c824-496b-a630-70f5369a636f</ProjectGuid>
+    <ProjectGuid>cd77a8cc-2885-47a7-b99c-fbf13353400b</ProjectGuid>


You likely are far out of date with master. you shoudl merge in master.

Just merged last friday.. Not sure why sln file changed. I didn't add any projects, etc.

CyrusNajmabadi · 2016-09-06T18:30:48Z

src/Features/VisualBasic/Portable/QuickInfo/VisualBasicSemanticQuickInfoProvider.vb

-                       textEditorFactoryService, glyphService, typeMap)
+
+Imports Microsoft.CodeAnalysis.QuickInfo
+Imports System.Collections.Immutable


sort imports.

paulvanbrenk · 2016-09-06T20:04:01Z

At first glance this looks very similar to the other public APIs, so it shouldn't be an issue for TypeScript to implement this. Is there anything we have to do specifically for the 'semantic' vs. 'syntactic' quickinfo, or is that optional?

CyrusNajmabadi · 2016-09-06T20:06:27Z

Is there anything we have to do specifically for the 'semantic' vs. 'syntactic' quickinfo, or is that optional?

That's optional. C# happens to have two different providers that serve different purposes. "Semantic" is the traditional provider which gives you symbolic informaiton when you hover on an identifier.

"Syntactic" is the quick info provider we use when you hover over a close-curly and want to see the beginning of the declaration that close curly belongs to.

You can implement either of these (or none of these) as you wish. We happened to use two separate providers because it kept hte code cleanly separated.

CyrusNajmabadi · 2016-09-14T22:26:05Z

src/EditorFeatures/CSharpTest/QuickInfo/SyntacticQuickInfoSourceTests.cs

 using Microsoft.VisualStudio.Language.Intellisense;
 using Microsoft.VisualStudio.Text.Editor;
 using Microsoft.VisualStudio.Text.Projection;
 using Roslyn.Test.Utilities;
 using Xunit;
+using Microsoft.CodeAnalysis.QuickInfo;


Sort usings.

CyrusNajmabadi · 2016-09-14T22:27:02Z

src/EditorFeatures/Core/Implementation/IntelliSense/QuickInfo/Controller.cs

@@ -15,6 +15,7 @@
 using Microsoft.VisualStudio.Text;
 using Microsoft.VisualStudio.Text.Editor;
 using Roslyn.Utilities;
+using Microsoft.CodeAnalysis.QuickInfo;


CyrusNajmabadi · 2016-09-14T22:28:47Z

...itorFeatures/Core/Implementation/IntelliSense/QuickInfo/Presentation/ElisionBufferContent.cs

+    /// used to create an elision buffer out that will then be displayed in the quick info
+    /// window.
+    /// </summary>
+    internal class ElisionBufferContent


Probably want to make this a ForegroundThreadAffinitizedObject. And add asserts in our methods ot make sure the UI stuff only happens on the UI thread.

CyrusNajmabadi · 2016-09-14T22:29:32Z

...torFeatures/Core/Implementation/IntelliSense/QuickInfo/Presentation/QuickInfoDisplayPanel.cs

+    internal class TextBlockElement
+    {
+        public string Kind { get; }
+        public TextBlock Block { get; }


I'm not certain what direction this is. Is this basically the owner of this "TextBlockElement", or is this something the TextBlockElement contains?

CyrusNajmabadi · 2016-09-14T22:31:39Z

...torFeatures/Core/Implementation/IntelliSense/QuickInfo/Presentation/QuickInfoDisplayPanel.cs

+        internal TextBlock TypeParameterMap => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.TypeParameters)?.Block;
+        internal TextBlock AnonymousTypes => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.AnonymousTypes)?.Block;
+        internal TextBlock UsageText => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.Usage)?.Block;
+        internal TextBlock ExceptionText => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.Exception)?.Block;


So, it's not totally clear to me why these are needed. Do we have specfici code elsewhere that needs to know "is this the type parameter map"? It seems highly desirable for "private ImmutableArray _textBlocks;" to contain enough information in it alone that the presentation can work off of.

If this is just for tests, then that's fine. However, we should then change the names to "_ForTestingPurposesOnly" on them. That way we can ensure that they're not used in the actual product code.

CyrusNajmabadi · 2016-09-14T22:33:18Z

src/Features/Core/Portable/FeaturesResources.resx

@@ -1067,4 +1067,7 @@ This version used in: {2}</value>
  <data name="Install_package_0" xml:space="preserve">
    <value>Install package '{0}'</value>
  </data>
+  <data name="Mutiple_Types" xml:space="preserve">
+    <value>&lt;Multiple Types&gt;</value>


where is this used?

It looks like it's used in VB for cases like this:

Dim x As Integer, y As String

CyrusNajmabadi · 2016-09-14T22:33:24Z

src/Features/Core/Portable/QuickInfo/CommonQuickInfoProvider.cs

+using System.Collections.Immutable;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;


CyrusNajmabadi · 2016-09-14T22:35:05Z

src/Features/Core/Portable/QuickInfo/IndentationHelper.cs

+
+namespace Microsoft.CodeAnalysis.QuickInfo
+{
+    // Reproduces logic in IProjectionBufferFactoryServiceExtensions (editor layer) 


definitely concerning. is there a reason that we can't just have the logic exist here and have IProjectionBufferFactoryServiceExtensions call this instead?

will take a look.

Unfortunately, this is not trivial to do. IProjectionBufferFactoryServiceExtensions operates directly on editor primitives. Converting those to use this is not trivial. However, this is really just there for tests.

CyrusNajmabadi · 2016-09-14T22:35:29Z

src/Features/Core/Portable/QuickInfo/QuickInfoItem.cs

+        /// <summary>
+        /// The span of the document that the item is based on.
+        /// </summary>
+        public TextSpan Span { get; set; }


public settable?

That does feel weird. Was that intentional?

CyrusNajmabadi · 2016-09-14T22:36:30Z

src/Features/Core/Portable/QuickInfo/QuickInfoService.cs

+        /// </summary>
+        public static QuickInfoService GetService(Workspace workspace, string language)
+        {
+            return workspace.Services.GetLanguageServices(language)?.GetService<QuickInfoService>() ?? NoOpService.Instance;


is this a pattern we want to use everywhere?

CyrusNajmabadi · 2016-09-14T22:37:10Z

src/Features/Core/Portable/QuickInfo/QuickInfoTextBlock.cs

+
+        private string _rawText;
+
+        public string RawText


is this needed publicly? or is this just for tests?

We have something similar on CompletionDescription, but it's just called Text. The property called Text on this class is called TaggedParts on CompletionDescription. I'll keep them similar here.

CyrusNajmabadi · 2016-09-14T22:37:50Z

src/Features/VisualBasic/Portable/VBFeaturesResources.resx

@@ -1235,4 +1235,7 @@ Sub(&lt;parameterList&gt;) &lt;statement&gt;</value>
  <data name="Make_Async_Sub" xml:space="preserve">
    <value>Make Async Sub</value>
  </data>
+  <data name="Multiple_Types" xml:space="preserve">


Why is this here as well? What is it needed for? If it is needed, we def should not have the same string in two different projects.

CyrusNajmabadi

Thanks!

CyrusNajmabadi · 2016-09-14T22:41:26Z

src/Features/Core/Portable/QuickInfo/QuickInfoItem.cs

+        /// Descriptive tags from the <see cref="Microsoft.CodeAnalysis.Completion.CompletionTags"/> type.
+        /// These tags may influence how the item is displayed.
+        /// </summary>
+        public ImmutableArray<string> Tags { get; }


Do we have examples of what sort of tags we use here? Is this to give things like the icon displayed in QuickInfo? Or is that information within the TextBlocks?

DustinCampbell

LGTM pending Cyrus's API questions

DustinCampbell · 2016-09-14T22:39:24Z

src/EditorFeatures/Core/Implementation/IntelliSense/QuickInfo/Model.cs

@@ -2,6 +2,7 @@

 using Microsoft.CodeAnalysis.Text;
 using Microsoft.CodeAnalysis.Text.Shared.Extensions;
+using Microsoft.CodeAnalysis.QuickInfo;
 using Microsoft.VisualStudio.Text;


sort usings

DustinCampbell · 2016-09-15T18:27:10Z

src/Features/Core/Portable/QuickInfo/QuickInfoItem.cs

+        /// <summary>
+        /// The span of the document that the item is based on.
+        /// </summary>
+        public TextSpan Span { get; set; }


That does feel weird. Was that intentional?

DustinCampbell · 2017-01-04T23:24:47Z

Looks like post-dev15 is up-to-date. You should be able to rebase this on that

DustinCampbell · 2017-12-05T18:24:45Z

Closing in favor of #20554

mattwar added 5 commits August 11, 2016 15:58

General API variant

66920bb

make tests compile

2eead77

simpler QuickInfo API

f97475e

merge with master and fix conflicts

785efef

removal of old or unused code & declarations

3f9aa4d

dnfclas added the cla-already-signed label Sep 6, 2016

CyrusNajmabadi reviewed Sep 6, 2016
View reviewed changes

Pilchie added the Area-IDE label Sep 6, 2016

mattwar added 7 commits September 12, 2016 12:48

Merge remote-tracking branch 'dotnet/master' into QuickInfoAPI2

3b09c7d

merge with master and fix conflicts

1493c75

fix feedback issues

f447083

Merge remote-tracking branch 'dotnet/master' into QuickInfoAPI2

698362c

use extension manager for providers

c48e203

Change to list of text blocks

90cb667

Use QuickInfoService.GetService

1590e67

CyrusNajmabadi reviewed Sep 14, 2016

View reviewed changes

DustinCampbell approved these changes Sep 15, 2016

View reviewed changes

DustinCampbell mentioned this pull request Nov 11, 2016

use SymbolDisplayFormat in TypeLookup OmniSharp/omnisharp-roslyn#665

Merged

merge with master and fix conflicts

c68bf9f

mattwar changed the base branch from master to post-dev15 January 5, 2017 00:07

DustinCampbell mentioned this pull request Jan 13, 2017

Built-in property and method help should include fully qualified type name dotnet/vscode-csharp#1118

Closed

DustinCampbell mentioned this pull request Jan 24, 2017

OmniSharp Agenda » Meeting Thursday January 26th, 2017 @ 18:00 UTC OmniSharp/OmniSharp#15

Closed

agocke changed the base branch from post-dev15 to master February 14, 2017 23:28

DustinCampbell mentioned this pull request Jun 29, 2017

New Quick Info API #20554

Merged

5 tasks

DustinCampbell closed this Dec 5, 2017

Quick info api2 #13623

Quick info api2 #13623

Conversation

mattwar commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulvanbrenk commented Sep 6, 2016

CyrusNajmabadi commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Jan 4, 2017

DustinCampbell commented Dec 5, 2017