Return cached results from nav to while we are loading the solution#52380
Return cached results from nav to while we are loading the solution#52380CyrusNajmabadi merged 21 commits intodotnet:mainfrom
Conversation
| // on faster than the rest of the solution. | ||
| _activeDocument = docTrackingService.GetActiveDocument(_solution); | ||
| _visibleDocuments = docTrackingService.GetVisibleDocuments(_solution) | ||
| .WhereAsArray(d => d != _activeDocument); |
There was a problem hiding this comment.
this just moved up. it is needed in multiple codepaths, and can be computed synchronously. so it can just be done in the searcher constructor itself, and then referenced from later methods. Saves us having to compute inside a method and then pass along as state.
|
@dibarbet Still working on tests. However, teh core logic is there if you want to take a look :) |
| Cache, | ||
|
|
||
| // If the search examined the latest data we have for the requested project or document. | ||
| Latest |
There was a problem hiding this comment.
it could have been a boolean, but i found namign these clearer.
|
@dibarbet this is ready for review. |
| { | ||
| [UseExportProvider] | ||
| [Trait(Traits.Feature, Traits.Features.NavigateTo)] | ||
| public class NavigateToSearcherTests |
There was a problem hiding this comment.
basic tests that validate the order of calls we get, and the relation between returning stale dta and being called in to get fresh data.
| foreach (var result in results) | ||
| await onResultFound(Convert(result)).ConfigureAwait(false); | ||
| return NavigateToSearchLocation.Latest; | ||
| } |
There was a problem hiding this comment.
our bridge methods to F#/TS always return 'Latest' as that's always been the semantics they expose. they can always come and decide to do things differnetly here if they want (for example supporting returning cached results with the same semantics as us if they want.)
| return false; | ||
|
|
||
| var isRemoteHostFullyLoaded = GetRemoteHostHydrateTask().IsCompleted; | ||
| return isRemoteHostFullyLoaded; |
There was a problem hiding this comment.
intentionally not 'await'ing the tasks. we're not waiting for hydration to finish, we're just asking if hydration has finished. so we only care about completion here.
| bool searchCurrentDocument, | ||
| IImmutableSet<string> kinds, | ||
| CancellationToken disposalToken, | ||
| INavigateToSearcherHost? host = null) |
There was a problem hiding this comment.
Any reason to not make this required?
There was a problem hiding this comment.
I could. Though i liked the idea that the core two cases (normal VS, and LSP) just didn't have to think about this.
| } | ||
|
|
||
| await Task.WhenAll(tasks).ConfigureAwait(false); | ||
| result.RemoveDuplicates(); |
There was a problem hiding this comment.
could this be an ordered set type (do we have that type anywhere)?
There was a problem hiding this comment.
we do not have such a type afaik :)
| cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| public ValueTask HydrateAsync(PinnedSolutionInfo solutionInfo, CancellationToken cancellationToken) |
There was a problem hiding this comment.
should this be part of the navigate to service? It seems like a general request to the process
There was a problem hiding this comment.
so the navto service is Project affinitized. I don't really want N project calls calling out to OOP to hydrate (since hydration is at the SOlution granularity for OOP). I started there, but moved to this model to more closely match the underlying semantics we can actually provide.
Continued followup to #52351. Includes cleanup for clarity, as well as new logic so that if we find nothing in teh search of hte cache, we fallback to still hydrating and searching OOP fully. This has the benefit that if you loaded a solution fresh, and you search, that you will get some results, and not nothing when searching. This also means that you shoudl always end up either getting:
You should never end up with no results, and a message saying thigns are incomplete. We definitely want to avoid this case as the user won't really have an idea about when that state will be over and when they can search again. Ideally, the 'some results' they get back are enough to accomplish at least some of the task they want until they end up searching again.
--
Needs some testing here. My initial approach on testing this didn't pan out given that we mock out storage during most of our normal tests. SO this will actually have to create a real storage subsystem to validate the behavior with/without data in it.