VS: reduce some coupling between components #15000
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,7 +109,10 @@ module private ExternalSymbol = | |
| | _ -> [] | ||
|
|
||
| // TODO: Uncomment code when VS has a fix for updating the status bar. | ||
|
Contributor
There was a problem hiding this comment. BTW do you think this is still relevant? :)
Contributor
Author
There was a problem hiding this comment. 😀 I bet VS statusbar works fine. Just not in our code. We should really stand of the shoulders of experts and either copy what VSIX Community Toolkit does or even just install their nuget. |
||
| type StatusBar() = | ||
| let statusBar = | ||
| ServiceProvider.GlobalProvider.GetService<SVsStatusbar, IVsStatusbar>() | ||
|
|
||
| let mutable _searchIcon = | ||
| int16 Microsoft.VisualStudio.Shell.Interop.Constants.SBAI_Find :> obj | ||
|
|
||
|
|
@@ -394,7 +397,6 @@ type internal GoToDefinition(metadataAsSource: FSharpMetadataAsSourceService) = | |
| ( | ||
| document: Document, | ||
| textSpan: Microsoft.CodeAnalysis.Text.TextSpan, | ||
| cancellationToken: CancellationToken | ||
| ) = | ||
| let navigableItem = FSharpGoToDefinitionNavigableItem(document, textSpan) | ||
|
|
@@ -407,9 +409,10 @@ type internal GoToDefinition(metadataAsSource: FSharpMetadataAsSourceService) = | |
| navigationService.TryNavigateToSpan(workspace, navigableItem.Document.Id, navigableItem.SourceSpan, cancellationToken) | ||
|
|
||
| if not navigationSucceeded then | ||
| StatusBar().TempMessage(SR.CannotNavigateUnknown()) | ||
|
|
||
| member _.NavigateToItem(navigableItem: FSharpNavigableItem, cancellationToken: CancellationToken) = | ||
| let statusBar = StatusBar() | ||
| use __ = statusBar.Animate() | ||
|
|
||
| statusBar.Message(SR.NavigatingTo()) | ||
|
|
@@ -434,12 +437,11 @@ type internal GoToDefinition(metadataAsSource: FSharpMetadataAsSourceService) = | |
| targetDocument: Document, | ||
| targetSourceText: SourceText, | ||
| symbolRange: range, | ||
| cancellationToken: CancellationToken | ||
| ) = | ||
| asyncMaybe { | ||
| let! item = this.FindDeclarationOfSymbolAtRange(targetDocument, symbolRange, targetSourceText) | ||
| return this.NavigateToItem(item, cancellationToken) | ||
| } | ||
|
|
||
| /// Find the definition location (implementation file/.fs) of the target symbol | ||
|
|
@@ -448,12 +450,11 @@ type internal GoToDefinition(metadataAsSource: FSharpMetadataAsSourceService) = | |
| targetDocument: Document, | ||
| targetSourceText: SourceText, | ||
| symbolRange: range, | ||
| cancellationToken: CancellationToken | ||
| ) = | ||
| asyncMaybe { | ||
| let! item = this.FindDefinitionOfSymbolAtRange(targetDocument, symbolRange, targetSourceText) | ||
| return this.NavigateToItem(item, cancellationToken) | ||
| } | ||
|
|
||
| member this.NavigateToExternalDeclaration | ||
|
|
@@ -546,7 +547,7 @@ type internal GoToDefinition(metadataAsSource: FSharpMetadataAsSourceService) = | |
| | _ -> TextSpan() | ||
|
|
||
| let navItem = FSharpGoToDefinitionNavigableItem(tmpShownDoc, span) | ||
| this.NavigateToItem(navItem, cancellationToken) | ||
| true | ||
| | _ -> false | ||
| | _ -> false | ||
|
|
@@ -744,13 +745,7 @@ module internal FSharpQuickInfo = | |
| return result |> Option.defaultValue (symbolUse.Range, None, Some targetQuickInfo) | ||
| } | ||
|
|
||
| type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, initialDoc: Document, thisSymbolUseRange: range) = | ||
|
|
||
| let workspace = initialDoc.Project.Solution.Workspace | ||
| let solution = workspace.CurrentSolution | ||
|
|
@@ -791,15 +786,13 @@ type internal FSharpNavigation | |
|
|
||
| match initialDoc.FilePath, targetPath with | ||
| | Signature, Signature | ||
| | Implementation, Implementation -> return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) | ||
|
|
||
| // Adjust the target from signature to implementation. | ||
| | Implementation, Signature -> return! gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, range, cancellationToken) | ||
|
|
||
| // Adjust the target from implmentation to signature. | ||
| | Signature, Implementation -> return! gtd.NavigateToSymbolDeclarationAsync(targetDoc, targetSource, range, cancellationToken) | ||
| } | ||
| |> Async.Ignore | ||
| |> Async.StartImmediate | ||
|
|
@@ -818,6 +811,7 @@ type internal FSharpNavigation | |
|
|
||
| member _.TryGoToDefinition(position, cancellationToken) = | ||
| let gtd = GoToDefinition(metadataAsSource) | ||
| let statusBar = StatusBar() | ||
|
Contributor
There was a problem hiding this comment. So, here (and elsewhere) - are you sure creating a status bar from scratch is equivalent to passing around the thing that flows in?
Contributor
Author
There was a problem hiding this comment. Looking again at this, IMV we should just yeet all the statusbar code. It does nothing anyway.
Contributor
Author
There was a problem hiding this comment. Looks like it was just a clunky wrapper around SVsStatusbar service, when it worked, pretty much stateless on its own, so creating it ad hoc is ok. Removing this completely looks like serious work, this file is 1000+ freaking lines🙃 |
||
| let gtdTask = gtd.FindDefinitionTask(initialDoc, position, cancellationToken) | ||
|
|
||
| // Wrap this in a try/with as if the user clicks "Cancel" on the thread dialog, we'll be cancelled. | ||
|
|
@@ -829,7 +823,7 @@ type internal FSharpNavigation | |
| if gtdTask.Status = TaskStatus.RanToCompletion && gtdTask.Result.IsSome then | ||
| match gtdTask.Result.Value with | ||
| | FSharpGoToDefinitionResult.NavigableItem (navItem), _ -> | ||
| gtd.NavigateToItem(navItem, cancellationToken) | ||
| // 'true' means do it, like Sheev Palpatine would want us to. | ||
| true | ||
| | FSharpGoToDefinitionResult.ExternalAssembly (targetSymbolUse, metadataReferences), _ -> | ||
|
|
@@ -876,7 +870,7 @@ type internal DocCommentId = | |
| | Type of EntityPath: string list | ||
| | None | ||
|
|
||
| type FSharpNavigableLocation(metadataAsSource: FSharpMetadataAsSourceService, symbolRange: range, project: Project) = | ||
| interface IFSharpNavigableLocation with | ||
| member _.NavigateToAsync(_options: FSharpNavigationOptions2, cancellationToken: CancellationToken) : Task<bool> = | ||
| asyncMaybe { | ||
|
|
@@ -892,10 +886,8 @@ type FSharpNavigableLocation(statusBar: StatusBar, metadataAsSource: FSharpMetad | |
| Implementation | ||
|
|
||
| match targetPath with | ||
| | Signature -> return! gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, symbolRange, cancellationToken) | ||
| | Implementation -> return! gtd.NavigateToSymbolDeclarationAsync(targetDoc, targetSource, symbolRange, cancellationToken) | ||
| } | ||
| |> Async.map (fun a -> a.IsSome) | ||
| |> RoslynHelpers.StartAsyncAsTask cancellationToken | ||
|
|
@@ -908,9 +900,6 @@ type FSharpCrossLanguageSymbolNavigationService() = | |
|
|
||
| let workspace = componentModel.GetService<VisualStudioWorkspace>() | ||
|
|
||
| let metadataAsSource = | ||
| componentModel | ||
| .DefaultExportProvider | ||
|
|
@@ -1141,7 +1130,7 @@ type FSharpCrossLanguageSymbolNavigationService() = | |
| // More results can theoretically be returned in case of method overloads, or when we have both signature and implementation files. | ||
| if locations.Count() >= 1 then | ||
| let (location, project) = locations.First() | ||
| return FSharpNavigableLocation(metadataAsSource, location, project) :> IFSharpNavigableLocation | ||
| else | ||
| return Unchecked.defaultof<_> // returning null here, so Roslyn can fallback to default source-as-metadata implementation. | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So once again, what's the need for this change? I'm not against it, just a bit paranoid and trying to better understand all this magic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No immediate need. Although I just found a use for GetServiceAsync in another branch.
This is pretty much copied from Roslyn repo.
The problem is when coding VSIX stuff in C# you'll get warnings from analyzers when doing unsafe things like calling GetService from background thread, we don't have that.
This is not immediately a problem because we have all the related bugs Ironed out over the years, it's more so we don't run into problems in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave the old overload as is, and name the new one GetServiceOnMainThread? What do you think @psfinaki ?