Skip to content

Conversation

@majocha
Copy link
Contributor

@majocha majocha commented Apr 14, 2023

QuickInfo links navigation have a slow path and a fast path.
Navigating to the range provided by the ToolTipText is fast / instant.
We have also logic that tries to navigate to .fs implementation in case the starting point (where the QuickInfo was triggered) is in an implementation file. This can take a type check and a lot of time in cases when navigating to another project.

JoinableTaskFactory.Run overload will display a standard dialog and allow for cancellation when link navigation is taking too long:
image

Also fixed the situation when this slower path silently fails in some cases. Now it falls back to the fast path (i.e. goes to .fsi, which is fast).

@majocha majocha requested a review from a team as a code owner April 14, 2023 06:15
@psfinaki
Copy link
Contributor

Thanks! This is going to be a valuable UX improvement.

I'd like to clarify a few things:

  • The current code seems to ensure navigating from signatures to signatures and from impl files to impl files - is this preserved?
  • Terms "slow" and "fast" would benefit from being clarified in code. In this context, "falling back to fast navigation" also sounds weird a bit - fallbacks usually go towards more basic and less optimized solutions
  • What was the concrete user scenario which was silently failing?
  • What's the motivation for picking 1-second timeout? Maybe it will be also helpful extracting the number to a constant with a comment

@majocha
Copy link
Contributor Author

majocha commented Apr 14, 2023

Thanks! This is going to be a valuable UX improvement.

I'd like to clarify a few things:

  • The current code seems to ensure navigating from signatures to signatures and from impl files to impl files - is this preserved?

Should be.

  • Terms "slow" and "fast" would benefit from being clarified in code. In this context, "falling back to fast navigation" also sounds weird a bit - fallbacks usually go towards more basic and less optimized solutions

Yeah, I was not very precise. Historically this feature started very thin. Niceprint in the compiler augmented the taggedtext with ranges, and given the range you could very quickly navigate to the textspan. This is now TryNavigateToTextSpan.

The slower path is the one that tries to find the implementation based on signature, this involves sometimes waiting for a type check, I suppose.

  • What was the concrete user scenario which was silently failing?

I've seen it fail in VisualFsharp.sln when navigating from Editor to FSharp.Core.

  • What's the motivation for picking 1-second timeout? Maybe it will be also helpful extracting the number to a constant with a comment

Just personal taste :) The default 2 seconds felt a bit too long time to not have any feedback. OTOH it seems aything less than a second gets rounded down to 0.

@majocha
Copy link
Contributor Author

majocha commented Apr 14, 2023

  • What was the concrete user scenario which was silently failing?

I've seen it fail in VisualFsharp.sln when navigating from Editor to FSharp.Core.

Specifically, to trigger this, try navigating to something crazy like bool :) It is underlined suggesting you can do it, but nothing happens, and there's no feedback something failed. With this PR it will navigate.

@psfinaki
Copy link
Contributor

  • What was the concrete user scenario which was silently failing?

I've seen it fail in VisualFsharp.sln when navigating from Editor to FSharp.Core.

Specifically, to trigger this, try navigating to something crazy like bool :) It is underlined suggesting you can do it, but nothing happens, and there's no feedback something failed. With this PR it will navigate.

Ohhh that's where you should have started! That's awesome, didn't think we have such a basic thing broken.

Anyway, it should be good then. I'd still ask to explain or elaborate on the terms "fast" and "slow" in the code/comments, otherwise - great job!

@majocha
Copy link
Contributor Author

majocha commented Apr 17, 2023

I updated the comments to make it less cryptic :)

@psfinaki
Copy link
Contributor

Cool!

@psfinaki psfinaki enabled auto-merge (squash) April 17, 2023 15:48
auto-merge was automatically disabled April 17, 2023 16:31

Head branch was pushed to by a user without write access

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants