Switch to a callback form for navigate to#52160
Conversation
| Assert.Equal(string.Format(FeaturesResources.in_0_project_1, "DogBed", "Test"), itemDisplay.AdditionalInformation); | ||
|
|
||
| item = items.ElementAt(1); | ||
| item = items.ElementAt(0); |
There was a problem hiding this comment.
these tests were actually bsted and were dependent on the non-deterministic ordering of ConcurrentBag. i have switched to a colleciton that is actually ordered and represents the real order that hte notifications come in.
| var declaredSymbolInfoKindsSet = new DeclaredSymbolInfoKindSet(kinds); | ||
|
|
||
| var searchResults = await ComputeSearchResultsAsync( | ||
| await ComputeSearchResultsAsync( |
There was a problem hiding this comment.
should this just return the task? then the async keyword could be removed from this method as well
There was a problem hiding this comment.
no. that would break semantics here. we would early-dispose both nameMatcher and containerMatcherOpt.
| ValueTask<ImmutableArray<SerializableNavigateToSearchResult>> SearchDocumentAsync( | ||
| PinnedSolutionInfo solutionInfo, DocumentId documentId, string searchPattern, ImmutableArray<string> kinds, CancellationToken cancellationToken); | ||
| ValueTask SearchDocumentAsync( | ||
| PinnedSolutionInfo solutionInfo, DocumentId documentId, string searchPattern, ImmutableArray<string> kinds, RemoteServiceCallbackId callbackId, CancellationToken cancellationToken); |
|
|
||
| Task<ImmutableArray<INavigateToSearchResult>> SearchProjectAsync(Project project, ImmutableArray<Document> priorityDocuments, string searchPattern, IImmutableSet<string> kinds, CancellationToken cancellationToken); | ||
| Task<ImmutableArray<INavigateToSearchResult>> SearchDocumentAsync(Document document, string searchPattern, IImmutableSet<string> kinds, CancellationToken cancellationToken); | ||
| Task SearchProjectAsync(Project project, ImmutableArray<Document> priorityDocuments, string searchPattern, IImmutableSet<string> kinds, Func<INavigateToSearchResult, Task> onResultFound, CancellationToken cancellationToken); |
| => GetCallback(callbackId).OnResultFoundAsync(result); | ||
| } | ||
|
|
||
| internal sealed class NavigateToSearchServiceCallback |
There was a problem hiding this comment.
callbacks for oop seem a little complicated. It's probably been considered, but why can't the logic for retrieving the actual callback method be abstracted and 'pre-processed' before any request hits the IRemoteXXXService? That they don't need to do hop from callbackid -> callback in each implementation. Is it because of the rehydrating stuff that is going on here that requires the solution (that other requests don't need?)
There was a problem hiding this comment.
not being familiar with the infrastructure, it was fairly difficult to follow the chain
There was a problem hiding this comment.
i don't have the answer to that. and, fwiw, i agree with you and think that would be reasonable. @tmat for his thoughts here. I was just following an existin gpattern that we already have in FindRefs. It does seem like we could potentially simplify things here in some way.
|
|
||
| var results = await searchTask.ConfigureAwait(false); | ||
| return results.Length; | ||
| var results = new List<INavigateToSearchResult>(); |
There was a problem hiding this comment.
curious what the difference was (if any) for these benchmarks
There was a problem hiding this comment.
effectively identical.
Co-authored-by: David Barbet <dibarbet@gmail.com>
Navigate-To currently searching each project concurrently, but results are only reported in one batch per project. This means a large project won't return any results until the entire project is searched.
This PR moves us to work more like the FindReferences model. we now pass in a callback that hte server can call back into as it finds matches. This means a result can be reported the moment it is found.
This does not impact F# or TS as we use EA for them, and this PR proxies this new pattern to the existing bulk-api we have for those languages.