Skip to content
This repository has been archived by the owner on Oct 31, 2021. It is now read-only.

Peek definition #1286

Merged
merged 8 commits into from
Dec 8, 2015
Merged

Conversation

vasily-kirichenko
Copy link
Contributor

1

@vasily-kirichenko
Copy link
Contributor Author

  • Add a setting
  • Should it show metadata for external symbols?
  • Should it be fully supported by all the features (colorized, Highlight refs, etc.)? (C# editor does support all features in it)
  • Bug: after first launch all features stop working for the buffer, have to reopen the doc to fix it

@dungpa
Copy link
Contributor

dungpa commented Dec 6, 2015

My answer is yes for all questions. How do you solve the 3rd issue?

@dungpa dungpa added this to the v2.3.0 milestone Dec 6, 2015
@dungpa dungpa added the feature label Dec 6, 2015
@vasily-kirichenko
Copy link
Contributor Author

I' not sure the third one should be implemented. The nice thing about the current impl is that it opens instantly, compared to 2+ seconds for ordinar file opening. I'm afraid that if we make it a full fledged buffer, it will become equally slow.

@dungpa
Copy link
Contributor

dungpa commented Dec 7, 2015

Ok, let's finish the first two items. The last one should only be done if file opening is fast enough.

@vasily-kirichenko
Copy link
Contributor Author

I'm not sure I'm ready to port Go to metadata code, it's tightly coupled with UI in GoToDefinition.fs. I'd rather leave it to implement in the future.

@dungpa
Copy link
Contributor

dungpa commented Dec 7, 2015

Fair enough.

The new Peek API isn't available on VS 2013. I've got this error when invoking Alt + F12 on VS 2013.

System.ComponentModel.Composition.CompositionException: The composition produced a single composition error, with 2 root causes. The root causes are provided below. Review the CompositionException.Errors property for more detailed information. 1) The export 'Microsoft.VisualStudio.Editor.Implementation.PeekResultFactory (ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekResultFactory")' is not assignable to type 'Microsoft.VisualStudio.Language.Intellisense.IPeekResultFactory'. Resulting in: Cannot set import 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider..ctor (Parameter="peekResultFactory", ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekResultFactory")' on part 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider'. Element: FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider..ctor (Parameter="peekResultFactory", ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekResultFactory") --> FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider Resulting in: Cannot get export 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider (ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekableItemSourceProvider")' from part 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider'. Element: FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider (ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekableItemSourceProvider") --> FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider 2) The export 'Microsoft.VisualStudio.Text.Implementation.TextDocumentFactoryService (ContractName="Microsoft.VisualStudio.Text.ITextDocumentFactoryService")' is not assignable to type 'Microsoft.VisualStudio.Text.ITextDocumentFactoryService'. Resulting in: Cannot set import 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider..ctor (Parameter="textDocumentFactoryService", ContractName="Microsoft.VisualStudio.Text.ITextDocumentFactoryService")' on part 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider'. Element: FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider..ctor (Parameter="textDocumentFactoryService", ContractName="Microsoft.VisualStudio.Text.ITextDocumentFactoryService") --> FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider Resulting in: Cannot get export 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider (ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekableItemSourceProvider")' from part 'FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider'. Element: FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider (ContractName="Microsoft.VisualStudio.Language.Intellisense.IPeekableItemSourceProvider") --> FSharpVSPowerTools.Logic.VS2015.PeekableItemSourceProvider at System.ComponentModel.Composition.Hosting.CompositionServices.GetExportedValueFromComposedPart(ImportEngine engine, ComposablePart part, ExportDefinition definition) at System.ComponentModel.Composition.Hosting.CatalogExportProvider.GetExportedValue(CatalogPart part, ExportDefinition export, Boolean isSharedPart) at System.ComponentModel.Composition.Hosting.CatalogExportProvider.CatalogExport.GetExportedValueCore() at System.ComponentModel.Composition.Primitives.Export.get_Value() at System.ComponentModel.Composition.ExportServices.GetCastedExportedValue[T](Export export) at System.ComponentModel.Composition.ExportServices.<>c__DisplayClass42.&lt;CreateStronglyTypedLazyOfTM&gt;b__1()&#x000D;&#x000A; at System.Lazy1.CreateValue() at System.Lazy1.LazyInitValue()&#x000D;&#x000A; at System.Lazy1.get_Value() at Microsoft.VisualStudio.Text.Utilities.GuardedOperations.InvokeMatchingFactories[TExtensionInstance,TExtensionFactory,TMetadataView](IEnumerable1 lazyFactories, Func2 getter, IContentType dataContentType, Object errorSource)

Could we only enable the feature for VS2015+? It's even better to show it clearly in the setting e.g. Peek Definition (VS2015+).

@vasily-kirichenko
Copy link
Contributor Author

I know, that is why I put it into Logic.VS2015 project. Maybe just hide or make "disabled" (gray and non interactive) the setting in VS2013?

@dungpa
Copy link
Contributor

dungpa commented Dec 7, 2015

We should make sure that Alt+F12 on VS2013 should do nothing. A clear label is OK. Of course, graying out the setting on VS 2013 is better :-).

@vasily-kirichenko
Copy link
Contributor Author

Huh. I try to fit this trick https://github.com/fsprojects/VisualFSharpPowerTools/blob/master/src/FSharpVSPowerTools.Logic/NavigateToItem.fs#L277-L295, but the case is not the same because there's no IPeekableItemSourceProvider in VS2013 at all, so we cannot add a fake implementation in Logic, then select right one at runtime. Any ideas?

@vasily-kirichenko
Copy link
Contributor Author

Done both disabling the setting and the trick with returning null (your last comment).
In VS 2015 everything works.

@vasily-kirichenko
Copy link
Contributor Author

About the bug, it seems only Highlight refs stops working after single Peek Definitions invoked:

1

@vasily-kirichenko
Copy link
Contributor Author

I think I understand why the bug: peek definition seems to reuse already open buffers and, when I close peek def window, it's Dispose is called, which disposes all our timers => most features stop working.

So, it seems we should use ITextView.Properties.GetOrCreateSingletonProperty instead of ITextBuffer.GetOrCreateSingletonProperty. Will check shortly.

@vasily-kirichenko
Copy link
Contributor Author

No, it has not helped :(

@vasily-kirichenko
Copy link
Contributor Author

It will be a long story.

@dungpa
Copy link
Contributor

dungpa commented Dec 8, 2015

The link looks scary :(

It still throws errors on VS2013; we need another way.

…k Definition view for same doc open then closed
@vasily-kirichenko
Copy link
Contributor Author

Have fixed the bug with stopping all the features working after opening-closing a Peek Def view.

@vasily-kirichenko
Copy link
Contributor Author

I have no idea how to get rid of the exception in VS 2013. I suggest:

  • stop supporting it
  • create different VSIXs for 2013 and 2015+

if (!_optionsPage.PeekDefinitionAvailable)
{
chbPeekDefinition.Enabled = false;
chbPeekDefinition.Text = chbPeekDefinition.Text + " (VS2015+ only)";
Copy link
Contributor

Choose a reason for hiding this comment

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

This will keep adding VS2015+ suffix to the label each time we open General options dialog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I suspected it.

@dungpa
Copy link
Contributor

dungpa commented Dec 8, 2015

Let's merge this after addressing the last two comments. I'll try to disable the feature for VS2013 after merging the PR.

@vasily-kirichenko
Copy link
Contributor Author

I think we should improve it significantly:

@dungpa
Copy link
Contributor

dungpa commented Dec 8, 2015

Yes, we should eventually get rid of ActiveDocument thing. It has never been working well.

dungpa added a commit that referenced this pull request Dec 8, 2015
@dungpa dungpa merged commit 58b8e40 into fsprojects-archive:master Dec 8, 2015
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants