Update go-to-def handler to support entirely async go-to-def operation. #59801
Conversation
|
@jasonmalinowski this is ready for review. note: the actual changes are pretty small. it's just the mechanical test changs that bloated the PR. |
| public bool TryGoToDefinition(Document document, int position, CancellationToken cancellationToken) | ||
| => throw new NotImplementedException("Use FindDefinitionLocationAsync instead."); |
There was a problem hiding this comment.
Why is this method here at all at this point?
There was a problem hiding this comment.
yup. nasty. will fixup.
| return await _streamingPresenter.GetNavigableLocationAsync( | ||
| _threadingContext, solution.Workspace, title, definitions, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Seems a bit odd this is a method on the streaming presenter here?
There was a problem hiding this comment.
Ah: I see it's also displaying the items too; should this still be called PresentAndGetItems? Or what happens if we're presenting and then nothing further is shown?
There was a problem hiding this comment.
yeah... this guy is funky in that it gets back an object that can optionally navigate you to a since location (if there is one) or which actually displayes the presenter if there are many. Importantly though, that presentation/navigation only happens on the stage-2 of the navigation. will think on a better name here.
src/EditorFeatures/Core/GoToDefinition/IGoToDefinitionService.cs
Outdated
Show resolved
Hide resolved
| @@ -88,27 +87,27 @@ internal static class IStreamingFindUsagesPresenterExtensions | |||
| // If we're directly going to a location we need to activate the preview so | |||
| // that focus follows to the new cursor position. This behavior is expected | |||
| // because we are only going to navigate once successfully | |||
| if (await item.TryNavigateToAsync(workspace, new NavigationOptions(PreferProvisionalTab: true, ActivateTab: true), cancellationToken).ConfigureAwait(false)) | |||
| return true; | |||
| return new NavigableLocation(cancellationToken => item.TryNavigateToAsync(workspace, options, cancellationToken)); | |||
There was a problem hiding this comment.
I'm now confused why we introduced INavigableLocation when we also had an INavigableItem here. (I swore we had something earlier but couldn't remember what it was called...now I know. 😄)
There was a problem hiding this comment.
So NavigableItem truly is "i have a single document and a span and i will go to that". INavigableItem encapsulates the idea of just anything generic that can be navigated to. You've seen already taht one example of that with the streaming-presenter 'navigating' by presenting a set of results. Other examples include calling out to 3rd parties to take over the navigation (we call this '3rd party navigation) and take the user somewhere.
src/EditorFeatures/Test2/GoToDefinition/VisualBasicGoToDefinitionTests.vb
Outdated
Show resolved
Hide resolved
Precursor to the BackgroundWorkIndicator work.
Followup to #59798