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.

Sign up for GitHub

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

Jump to bottom

New Quick Info API #20554

Merged
merged 55 commits into from
Feb 16, 2018
Merged

New Quick Info API #20554

merged 55 commits into from
Feb 16, 2018

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Jun 29, 2017 •
edited
Loading

This is a PR to create a new public Quick Info API (#11103)

cc @dotnet/roslyn-ide

@DustinCampbell
Copy link
Member Author

@jasonmalinowski: Are VS integration tests known to be broken? Both failed with the following before they even started running tests.

17:06:39 	Uninstalling Roslyn.VisualStudio.InteractiveComponents.vsix
17:06:39 "C:\Users\dotnet-bot\.nuget\packages\roslyntools.microsoft.vsixexpinstaller\0.4.0-beta\tools\VsixExpInstaller.exe" -u /rootSuffix:RoslynDev /vsInstallDir:"C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise" D:\j\workspace\windows_debug---0c191de0\Binaries\Debug\Vsix\VisualStudioInteractiveComponents\Roslyn.VisualStudio.InteractiveComponents.vsix
17:06:39   Running as Admin.
17:06:44   Uninstalling local extension: 'C:\USERS\DOTNET-BOT\APPDATA\LOCAL\MICROSOFT\VISUALSTUDIO\15.0_42810653ROSLYNDEV\EXTENSIONS\LBLEM5VG.V4K\'
17:06:45 Exception: The local extension failed to uninstall.
17:06:45 System.Exception: The local extension failed to uninstall.
17:06:45    at VsixExpInstaller.Program.<>c__DisplayClass9_3.<Main>g__Uninstall8(IInstalledExtension installedExtension)
17:06:45    at VsixExpInstaller.Program.<>c__DisplayClass9_2.<Main>g__RunProgram1()
17:06:45    at VsixExpInstaller.Program.Main(String[] args)
17:06:45 Command failed to execute: C:\Users\dotnet-bot\.nuget\packages\roslyntools.microsoft.vsixexpinstaller\0.4.0-beta\tools\VsixExpInstaller.exe -u /rootSuffix:RoslynDev /vsInstallDir:"C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise" D:\j\workspace\windows_debug---0c191de0\Binaries\Debug\Vsix\VisualStudioInteractiveComponents\Roslyn.VisualStudio.InteractiveComponents.vsix
17:06:45 System.Management.Automation.RuntimeException: Command failed to execute: C:\Users\dotnet-bot\.nuget\packages\roslyntools.microsoft.vsixexpinstaller\0.4.0-beta\tools\VsixExpInstaller.exe -u /rootSuffix:RoslynDev /vsInstallDir:"C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise" D:\j\workspace\windows_debug---0c191de0\Binaries\Debug\Vsix\VisualStudioInteractiveComponents\Roslyn.VisualStudio.InteractiveComponents.vsix

@Pilchie
Copy link
Member

Pilchie commented Jun 30, 2017

Tagging @dotnet/roslyn-infrastructure. I saw the same in #20558

@DustinCampbell
Copy link
Member Author

Looks like integration tests are working again. I re-ran them and both passed.

@CyrusNajmabadi
Copy link
Member

Tagging @Therzok

@CyrusNajmabadi
Copy link
Member

reviewing now. have been discussing offline ideas with therzok about how to make this sort of thing work well with VS4Mac as well.

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 30, 2017
View reviewed changes
src/EditorFeatures/CSharpTest/QuickInfo/SyntacticQuickInfoSourceTests.cs Outdated
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;
Copy link
Member

Choose a reason for hiding this comment

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

Sort!

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@DustinCampbell
Copy link
Member Author

reviewing now. have been discussing offline ideas with therzok about how to make this sort of thing work well with VS4Mac as well.

OK. Do note the TODO list above which indicates that I've yet to go through your comments on Matt's original PR yet.

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 30, 2017
View reviewed changes
src/EditorFeatures/CSharpTest/QuickInfo/SyntacticQuickInfoSourceTests.cs Outdated
workspace.GetService<ITextEditorFactoryService>(),
workspace.GetService<IGlyphService>(),
workspace.GetService<ClassificationTypeMap>());
return new CSharpSyntacticQuickInfoProvider();
Copy link
Member

Choose a reason for hiding this comment

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

Consider making a singleton.

Copy link
Member

Choose a reason for hiding this comment

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

oh, this is tests. n/m

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 30, 2017
View reviewed changes
...torFeatures/Core/Implementation/IntelliSense/QuickInfo/Presentation/QuickInfoDisplayPanel.cs Outdated

internal TextBlock MainDescription => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.Description)?.Block;
internal TextBlock Documentation => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.DocumentationComments)?.Block;
internal TextBlock TypeParameterMap => _textBlocks.FirstOrDefault(tb => tb.Kind == QuickInfoTextKinds.TypeParameters)?.Block;
Copy link
Member

Choose a reason for hiding this comment

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

So is this the display for C#/VB? Will other languages create their own display panel?

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 30, 2017
View reviewed changes
...EditorFeatures/Core/Implementation/IntelliSense/QuickInfo/Presentation/QuickInfoPresenter.cs Outdated
using System.ComponentModel.Composition;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.VisualStudio.Language.Intellisense;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Utilities;
using System.Collections.Generic;
using System;
using Microsoft.VisualStudio.Text.Projection;
Copy link
Member

Choose a reason for hiding this comment

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

sort!

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@CyrusNajmabadi
Copy link
Member

So, one thing we'd like here is to have a similar model as CompletionItem where there is an ImmutableDictionary<string,string> Properties to store data. Roslyn could then add SymbolId to the QuickInfo item and consumes like VS4Mac could then use that SymbolId to get back to the Symbol and do extra stuff they might care about.

@CyrusNajmabadi
Copy link
Member

@DustinCampbell the primary thing i want to look at are the APIs a language implements to produce the quick-info data, and teh API defining the quick-info data itself. Can you directly point out where these APIs are in this PR? I just want to look at it from the perspective of what this would be like for F#/TS/JS. The internal details of how that data gets passed around and eventually shown is less interesting for me :)

@DustinCampbell
Copy link
Member Author

@CyrusNajmabadi: Please look at the TODO list at the top of this PR. I'm not there yet. 😄 The goal of this PR was just to get the old Quick Info API moved forward and building (success!).

@CyrusNajmabadi
Copy link
Member

Ah ok. When you get there, let me know.

@CyrusNajmabadi
Copy link
Member

So, will this PR break F#/TS/JS? Or is that all still fine?

@DustinCampbell
Copy link
Member Author

Yeah, that's definitely fair. Can we use typeforwarders to the rescue?! :)

No idea. They terrify me. Also, I'm not sure if they can forward to a type in the same assembly. The documentation says, "Specifies a destination Type in another assembly."

@terrajobst
Copy link
Member

terrajobst commented Feb 15, 2018 •
edited
Loading

@CyrusNajmabadi @DustinCampbell

Yeah, that's definitely fair. Can we use typeforwarders to the rescue?! :)

You can't type forward between namespaces as the namespace name is part of the type's identity. You can only bridge assembly differences.

No idea. They terrify me.

They should. THEY TOTALLY SHOULD.

@DustinCampbell
Copy link
Member Author

Besides, look at all the trouble @terrajobst has gotten us into with type forwarders. 😃

@DustinCampbell
Apply code review feedback to Quick Info PR
0c10f4b 
This commit has a few significant changes beyond correcting minor nit picks:

1. It unifies the GlyphExtensions classes in the Workspaces and non-WPF EditorFeatures layers.
2. It significantly cleans up the super-ugly mocking code in QuickInfoControlTests.vb
3. Tweaks the QuickInfo API to remove QuickInfoItem.Empty and QuickInfoItem.IsEmpty properties.
@DustinCampbell
Copy link
Member Author

@dotnet-bot retest this please

@DustinCampbell
Copy link
Member Author

@dotnet-bot retest this please

@terrajobst
Copy link
Member

@DustinCampbell

Besides, look at all the trouble @terrajobst has gotten us into with type forwarders. 😃

image

@DustinCampbell
Copy link
Member Author

@dotnet-bot retest this please

@DustinCampbell
Copy link
Member Author

@dotnet-bot retest this please

@DustinCampbell
Copy link
Member Author

excuse the noise from closing/reopening. I'm trying to get CI to run on this branch and on this PR.

@DustinCampbell DustinCampbell merged commit 8985690 into dotnet:features/quick-info-party Feb 16, 2018
@JieCarolHu JieCarolHu mentioned this pull request Apr 6, 2018
Merged
@rchande rchande mentioned this pull request Jun 13, 2018
Closed
@DustinCampbell DustinCampbell mentioned this pull request Sep 11, 2018
Merged
@SirIntruder SirIntruder mentioned this pull request Oct 18, 2018
Closed
@filipw filipw mentioned this pull request Mar 12, 2019
Merged
@DustinCampbell DustinCampbell deleted the new-quick-info-api branch August 24, 2021 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olegtk olegtk olegtk left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

Assignees
No one assigned
Labels
Area-IDE cla-already-signed Concept-API This issue involves adding, removing, clarification, or modification of an API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.