Simplify consumption side of the new async navigation helpers. #59838

CyrusNajmabadi · 2022-03-01T17:21:35Z

Followup to #59802.

Continues work on async navigation, removing old helpers.

THe core idea here is that we'd like the INavigableLocation to potentially be synchronous to navigate, indicating that all expensive BG work has already been done, and the only remaining work is a quite opening of the UI to take the user to the actual location. As such, all consumers of it must get the location asynchronously, then switch to the UI as the very last step, then do the actual navigation.

Note; tehre is only one final portion of the code preventing the actual switch rom async to sync here. That's the reentrancy in MAS where we use async to first generate the MAS file, then we try to open it, reentering to call back into navigation to find the doc post open. Once we clean that up, the final swithc can happen.

…igation5

CyrusNajmabadi · 2022-03-06T05:15:57Z

src/EditorFeatures/Core.Wpf/NavigableSymbols/NavigableSymbolService.NavigableSymbol.cs

-                        _definitions,
-                        cancellationToken).ConfigureAwait(false);
-                    if (location != null)
-                        await location.NavigateToAsync(new NavigationOptions(PreferProvisionalTab: true, ActivateTab: true), cancellationToken).ConfigureAwait(false);


note: i don't pass these options down as all callers use this nav value for the TryPresentLocationsAsync calls. so it's just specified inside that instead.

CyrusNajmabadi · 2022-03-06T05:26:09Z

@jasonmalinowski this is ready for review.

…igation5

CyrusNajmabadi · 2022-03-07T20:45:00Z

src/EditorFeatures/Core/CodeActions/CodeActionEditHandlerService.cs

@@ -301,7 +300,7 @@ internal class CodeActionEditHandlerService : ForegroundThreadAffinitizedObject,
            {
                var navigationService = workspace.Services.GetRequiredService<IDocumentNavigationService>();
                await navigationService.TryNavigateToPositionAsync(
-                    workspace, navigationOperation.DocumentId, navigationOperation.Position, cancellationToken).ConfigureAwait(false);
+                    this.ThreadingContext, workspace, navigationOperation.DocumentId, navigationOperation.Position, cancellationToken).ConfigureAwait(false);


teh general concept here is that all the helpers which actually do perform a navigation take a ThreadingCOntext. That way they could, if necessary, switch to a UI thread. This also means taht the helpers are only available to consumer who ahve a threading context and thus live in an 'editor' world where the concepts of navigating actually make sense.

Ah, interesting way to split that up. I think I like it!

CyrusNajmabadi · 2022-03-07T20:47:11Z

...ore/Portable/ExternalAccess/VSTypeScript/Api/VSTypeScriptDocumentNavigationServiceWrapper.cs

+                var location = await obj.GetLocationForPositionAsync(
+                    workspace, documentId, position, virtualSpace, cancellationToken).ConfigureAwait(false);
+                return location != null &&
+                    await location.NavigateToAsync(NavigationOptions.Default, cancellationToken).ConfigureAwait(false);


this is a genuine weird case. the TS code lives at the feature layer, but has hte concept of navigation. if we changes NavigateTo to be sync and require the UI thread, we'd have to introduce some sort of hack (likely through the IWorkspaceThreadingService) to somehow allow it ot move over. I don't have a good idea of what we can do here that isn't hack. Thoughts @jasonmalinowski ?

Is there a reason the TypeScript stuff isn't at the editor features layer?

src/EditorFeatures/Core/Host/IStreamingFindReferencesPresenter.cs

src/EditorFeatures/Core/Navigation/IDocumentNavigationServiceExtensions.cs

jasonmalinowski · 2022-03-07T23:27:59Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemDisplay.cs

-                NavigationOptions.Default,
-                allowInvalidSpan: _searchResult.NavigableItem.IsStale,
-                CancellationToken.None));
+            _threadingContext.JoinableTaskFactory.Run(async () =>


Hmm, why the await here?

godo catch. no reason will fix.

jasonmalinowski · 2022-03-07T23:29:23Z

src/EditorFeatures/Core/CodeActions/CodeActionEditHandlerService.cs

@@ -301,7 +300,7 @@ internal class CodeActionEditHandlerService : ForegroundThreadAffinitizedObject,
            {
                var navigationService = workspace.Services.GetRequiredService<IDocumentNavigationService>();
                await navigationService.TryNavigateToPositionAsync(
-                    workspace, navigationOperation.DocumentId, navigationOperation.Position, cancellationToken).ConfigureAwait(false);
+                    this.ThreadingContext, workspace, navigationOperation.DocumentId, navigationOperation.Position, cancellationToken).ConfigureAwait(false);


Ah, interesting way to split that up. I think I like it!

jasonmalinowski · 2022-03-07T23:29:58Z

src/EditorFeatures/Core/CodeActions/CodeActionEditHandlerService.cs

-                            if (location != null &&
-                                await location.NavigateToAsync(NavigationOptions.Default, cancellationToken).ConfigureAwait(false))
+                            if (await navigationService.TryNavigateToSpanAsync(
+                                    this.ThreadingContext, editorWorkspace, documentId, resolvedRenameToken.Span, cancellationToken).ConfigureAwait(false))


Indenting intentional this way?

yup. i dont' like my arguments aligning with the parent, they are better indented to visually tell tehy're children, not siblings :)

jasonmalinowski · 2022-03-07T23:33:56Z

...ore/Portable/ExternalAccess/VSTypeScript/Api/VSTypeScriptDocumentNavigationServiceWrapper.cs

+                var location = await obj.GetLocationForPositionAsync(
+                    workspace, documentId, position, virtualSpace, cancellationToken).ConfigureAwait(false);
+                return location != null &&
+                    await location.NavigateToAsync(NavigationOptions.Default, cancellationToken).ConfigureAwait(false);


Is there a reason the TypeScript stuff isn't at the editor features layer?

src/VisualStudio/Core/Impl/SolutionExplorer/SourceGeneratedFileItems/SourceGeneratedFileItem.cs

CyrusNajmabadi · 2022-03-07T23:41:49Z

Going to merge this in. But address all the feedback in a quick followup as i think it's all relevant.

CyrusNajmabadi · 2022-03-07T23:43:45Z

Is there a reason the TypeScript stuff isn't at the editor features layer?

:'(

CyrusNajmabadi added 5 commits February 28, 2022 17:33

Delete extension

af1e748

Remove more helpers

3153cc8

Remove more helpers

bfb7bfa

Remove more helpers

194c522

Remove more helpers

09f5bd2

dotnet-issue-labeler bot added the Area-IDE label Mar 1, 2022

CyrusNajmabadi added 24 commits March 1, 2022 09:38

Add back heleprs

83e5c04

Add back heleprs

c9492fc

Add back heleprs

d78a7a3

Continued simplification

f01fc20

Continued simplification

a36619e

Continued simplification

b448f4e

Continued simplification

1040205

Merge branch 'asyncNavigation4' into asyncNavigation5

d9f8c52

Merge branch 'asyncNavigation5' into asyncNavigation6

7d63cd0

Use helpers

8bc07e1

Use helpers

739f224

Use helpers

0d299cb

Use helpers

8bce926

Use helpers

d17b2e3

Use helpers

b6b0559

Use helpers

571c77b

Switch to helper

9203328

switch to helper

ec87597

Merge branch 'asyncNavigation4' into asyncNavigation5

a2e76e3

Merge branch 'asyncNavigation5' into asyncNavigation6

46ea49f

Merge branch 'asyncNavigation4' into asyncNavigation5

c3d108d

Merge branch 'asyncNavigation5' into asyncNavigation6

9eed312

Merge branch 'asyncNavigation4' into asyncNavigation5

13cd975

Merge branch 'asyncNavigation5' into asyncNavigation6

fe1ccce

runfoapp bot mentioned this pull request Mar 2, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

Merge remote-tracking branch 'upstream/release/dev17.3' into asyncNav…

f67d2e5

…igation5

CyrusNajmabadi changed the base branch from main to release/dev17.3 March 6, 2022 04:53

CyrusNajmabadi force-pushed the asyncNavigation5 branch from f67d2e5 to 87370b5 Compare March 6, 2022 04:54

CyrusNajmabadi changed the base branch from release/dev17.3 to main March 6, 2022 04:55

CyrusNajmabadi force-pushed the asyncNavigation5 branch from 87370b5 to f67d2e5 Compare March 6, 2022 04:55

CyrusNajmabadi changed the base branch from main to release/dev17.3 March 6, 2022 04:56

Merge branch 'asyncNavigation5' into asyncNavigation6

d19b75b

CyrusNajmabadi commented Mar 6, 2022

View reviewed changes

CyrusNajmabadi added 5 commits March 5, 2022 21:16

Use proper defaults

53ea709

simplify

674beea

Doc

ac7c231

simplify

5c487f5

Use normal defaults

bbaa00f

CyrusNajmabadi marked this pull request as ready for review March 6, 2022 05:23

CyrusNajmabadi requested review from a team as code owners March 6, 2022 05:23

CyrusNajmabadi requested a review from jasonmalinowski March 6, 2022 05:26

CyrusNajmabadi changed the title ~~Async navigation5~~ Simplify consumption side of the new async navigation helpers. Mar 6, 2022

Merge remote-tracking branch 'upstream/release/dev17.3' into asyncNav…

cbbd1da

…igation5

CyrusNajmabadi commented Mar 7, 2022

View reviewed changes

jasonmalinowski approved these changes Mar 7, 2022

View reviewed changes

CyrusNajmabadi merged commit 4f046ca into dotnet:release/dev17.3 Mar 8, 2022

CyrusNajmabadi deleted the asyncNavigation5 branch March 8, 2022 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify consumption side of the new async navigation helpers. #59838

Simplify consumption side of the new async navigation helpers. #59838

CyrusNajmabadi commented Mar 1, 2022 •

edited

Loading

CyrusNajmabadi Mar 6, 2022

CyrusNajmabadi commented Mar 6, 2022

CyrusNajmabadi Mar 7, 2022

jasonmalinowski Mar 7, 2022

CyrusNajmabadi Mar 7, 2022

jasonmalinowski Mar 7, 2022

jasonmalinowski Mar 7, 2022

CyrusNajmabadi Mar 7, 2022

jasonmalinowski Mar 7, 2022

jasonmalinowski Mar 7, 2022

CyrusNajmabadi Mar 7, 2022

jasonmalinowski Mar 7, 2022

CyrusNajmabadi commented Mar 7, 2022

CyrusNajmabadi commented Mar 7, 2022

Simplify consumption side of the new async navigation helpers. #59838

Simplify consumption side of the new async navigation helpers. #59838

Conversation

CyrusNajmabadi commented Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 6, 2022

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 commented Mar 7, 2022

CyrusNajmabadi commented Mar 7, 2022

CyrusNajmabadi commented Mar 1, 2022 •

edited

Loading