Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure the initial symbol a user searches for is top of hte find-refs list #43472

Merged
7 commits merged into from
Apr 22, 2020
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,11 @@ private async Task<DefinitionItem> GetDefinitionItemAsync(ISymbol definition)
if (!_definitionToItem.TryGetValue(definition, out var definitionItem))
{
definitionItem = await definition.ToClassifiedDefinitionItemAsync(
_solution, includeHiddenLocations: false, _options, _context.CancellationToken).ConfigureAwait(false);
_solution,
isPrimary: _definitionToItem.Count == 0,
includeHiddenLocations: false,
_options,
_context.CancellationToken).ConfigureAwait(false);

_definitionToItem[definition] = definitionItem;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,6 @@ private static async Task FindSymbolMonikerReferencesAsync(
if (moniker == null)
return;

// Let the find-refs window know we have outstanding work
await using var _ = await context.ProgressTracker.AddSingleItemAsync().ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentional removal. progress tracking doesn't work well for richnav as we don't really know how many items it will need to search. so trying to evne add progress items for it just ends up interfering with the good progress bar we have for the normal find-refs for user code in their solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so we did all that refactoring work to then not use it? 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used elsewhere :) just here it's not particularly good and def degrades the experience. i.e. if you do have code-index, we just jump to the 50% point immediately because it says "i have one piece of work", and then finishes it. The other part of find refs has has added one piece of owrk that is not done, causing us to be at 1-of-2 done.

This is unlike normal find refs, which can add a bunch of known work it needs to do (like looking at all files with bloom filter matches) which gives far more fine-grained progress.


var displayParts = GetDisplayParts(definition).AddRange(new[]
{
new TaggedText(TextTags.Space, " "),
Expand All @@ -45,8 +42,7 @@ private static async Task FindSymbolMonikerReferencesAsync(
var monikers = ImmutableArray.Create(moniker);

var first = true;
await foreach (var referenceItem in monikerUsagesService.FindReferencesByMoniker(
definitionItem, monikers, context.ProgressTracker, cancellationToken))
await foreach (var referenceItem in monikerUsagesService.FindReferencesByMoniker(definitionItem, monikers, cancellationToken))
{
if (first)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ await context.SetSearchTitleAsync(
foreach (var implementation in implementations)
{
var definitionItem = await implementation.ToClassifiedDefinitionItemAsync(
solution, includeHiddenLocations: false, FindReferencesSearchOptions.Default, cancellationToken).ConfigureAwait(false);
solution, isPrimary: true, includeHiddenLocations: false, FindReferencesSearchOptions.Default, cancellationToken).ConfigureAwait(false);

await context.OnDefinitionFoundAsync(definitionItem).ConfigureAwait(false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,25 +61,28 @@ public static DefinitionItem ToNonClassifiedDefinitionItem(
// to compute the classified spans for the locations of the definition. So it's totally
// fine to pass in CancellationToken.None and block on the result.
return ToDefinitionItemAsync(
definition, solution, includeHiddenLocations, includeClassifiedSpans: false,
definition, solution, isPrimary: false, includeHiddenLocations, includeClassifiedSpans: false,
options: FindReferencesSearchOptions.Default, cancellationToken: CancellationToken.None).WaitAndGetResult_CanCallOnBackground(CancellationToken.None);
}

public static Task<DefinitionItem> ToClassifiedDefinitionItemAsync(
this ISymbol definition,
Solution solution,
bool isPrimary,
bool includeHiddenLocations,
FindReferencesSearchOptions options,
CancellationToken cancellationToken)
{
return ToDefinitionItemAsync(definition, solution,
return ToDefinitionItemAsync(
definition, solution, isPrimary,
includeHiddenLocations, includeClassifiedSpans: true,
options, cancellationToken);
}

private static async Task<DefinitionItem> ToDefinitionItemAsync(
this ISymbol definition,
Solution solution,
bool isPrimary,
bool includeHiddenLocations,
bool includeClassifiedSpans,
FindReferencesSearchOptions options,
Expand Down Expand Up @@ -107,7 +110,7 @@ private static async Task<DefinitionItem> ToDefinitionItemAsync(

using var sourceLocationsDisposer = ArrayBuilder<DocumentSpan>.GetInstance(out var sourceLocations);

var properties = GetProperties(definition);
var properties = GetProperties(definition, isPrimary);

var displayableProperties = AbstractReferenceFinder.GetAdditionalFindUsagesProperties(definition);

Expand Down Expand Up @@ -161,10 +164,15 @@ private static async Task<DefinitionItem> ToDefinitionItemAsync(
nameDisplayParts, properties, displayableProperties, displayIfNoReferences);
}

private static ImmutableDictionary<string, string> GetProperties(ISymbol definition)
private static ImmutableDictionary<string, string> GetProperties(ISymbol definition, bool isPrimary)
{
var properties = ImmutableDictionary<string, string>.Empty;

if (isPrimary)
{
properties = properties.Add(DefinitionItem.Primary, "");
}

var rqName = RQNameInternal.From(definition);
if (rqName != null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
using Microsoft.CodeAnalysis.FindUsages;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Utilities;

namespace Microsoft.CodeAnalysis.Editor.FindUsages
Expand All @@ -23,20 +22,20 @@ namespace Microsoft.CodeAnalysis.Editor.FindUsages
/// </summary>
internal interface IFindSymbolMonikerUsagesService : IWorkspaceService
{
IAsyncEnumerable<ExternalReferenceItem> FindReferencesByMoniker(DefinitionItem definition, ImmutableArray<SymbolMoniker> monikers, IStreamingProgressTracker progress, CancellationToken cancellationToken);
IAsyncEnumerable<DefinitionItem> FindDefinitionsByMoniker(SymbolMoniker moniker, IStreamingProgressTracker progress, CancellationToken cancellationToken);
IAsyncEnumerable<DefinitionItem> FindImplementationsByMoniker(SymbolMoniker moniker, IStreamingProgressTracker progress, CancellationToken cancellationToken);
IAsyncEnumerable<ExternalReferenceItem> FindReferencesByMoniker(DefinitionItem definition, ImmutableArray<SymbolMoniker> monikers, CancellationToken cancellationToken);
IAsyncEnumerable<DefinitionItem> FindDefinitionsByMoniker(SymbolMoniker moniker, CancellationToken cancellationToken);
IAsyncEnumerable<DefinitionItem> FindImplementationsByMoniker(SymbolMoniker moniker, CancellationToken cancellationToken);
}

internal abstract class AbstractFindSymbolMonikerUsagesService : IFindSymbolMonikerUsagesService
{
public virtual IAsyncEnumerable<DefinitionItem> FindDefinitionsByMoniker(SymbolMoniker moniker, IStreamingProgressTracker progress, CancellationToken cancellationToken)
public virtual IAsyncEnumerable<DefinitionItem> FindDefinitionsByMoniker(SymbolMoniker moniker, CancellationToken cancellationToken)
=> EmptyAsyncEnumerable<DefinitionItem>.Instance;

public virtual IAsyncEnumerable<DefinitionItem> FindImplementationsByMoniker(SymbolMoniker moniker, IStreamingProgressTracker progress, CancellationToken cancellationToken)
public virtual IAsyncEnumerable<DefinitionItem> FindImplementationsByMoniker(SymbolMoniker moniker, CancellationToken cancellationToken)
=> EmptyAsyncEnumerable<DefinitionItem>.Instance;

public virtual IAsyncEnumerable<ExternalReferenceItem> FindReferencesByMoniker(DefinitionItem definition, ImmutableArray<SymbolMoniker> monikers, IStreamingProgressTracker progress, CancellationToken cancellationToken)
public virtual IAsyncEnumerable<ExternalReferenceItem> FindReferencesByMoniker(DefinitionItem definition, ImmutableArray<SymbolMoniker> monikers, CancellationToken cancellationToken)
=> EmptyAsyncEnumerable<ExternalReferenceItem>.Instance;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ await context.SetSearchTitleAsync(
if (sourceDefinition != null)
{
var definitionItem = await sourceDefinition.ToClassifiedDefinitionItemAsync(
solution, includeHiddenLocations: false, FindReferencesSearchOptions.Default, cancellationToken: cancellationToken).ConfigureAwait(false);
solution, isPrimary: true, includeHiddenLocations: false, FindReferencesSearchOptions.Default, cancellationToken: cancellationToken).ConfigureAwait(false);

await context.OnDefinitionFoundAsync(definitionItem).ConfigureAwait(false);
found = true;
Expand Down
6 changes: 6 additions & 0 deletions src/Features/Core/Portable/FindUsages/DefinitionItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ namespace Microsoft.CodeAnalysis.FindUsages
/// </summary>
internal abstract partial class DefinitionItem
{
/// <summary>
/// The definition item corresponding to the initial symbol the user was trying to find. This item should get
/// prominent placement in the final UI for the user.
/// </summary>
internal const string Primary = nameof(Primary);

// Existing behavior is to do up to two lookups for 3rd party navigation for FAR. One
// for the symbol itself and one for a 'fallback' symbol. For example, if we're FARing
// on a constructor, then the fallback symbol will be the actual type that the constructor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ protected RoslynDefinitionBucket GetOrCreateDefinitionBucket(DefinitionItem defi
{
if (!_definitionToBucket.TryGetValue(definition, out var bucket))
{
bucket = new RoslynDefinitionBucket(Presenter, this, definition);
bucket = RoslynDefinitionBucket.Create(Presenter, this, definition);
_definitionToBucket.Add(definition, bucket);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,34 @@ private class RoslynDefinitionBucket : DefinitionBucket, ISupportsNavigation
public readonly DefinitionItem DefinitionItem;

public RoslynDefinitionBucket(
string name,
bool expandedByDefault,
StreamingFindUsagesPresenter presenter,
AbstractTableDataSourceFindUsagesContext context,
DefinitionItem definitionItem)
: base(name: definitionItem.DisplayParts.JoinText() + " " + definitionItem.GetHashCode(),
: base(name,
sourceTypeIdentifier: context.SourceTypeIdentifier,
identifier: context.Identifier)
identifier: context.Identifier,
expandedByDefault: expandedByDefault)
{
_presenter = presenter;
DefinitionItem = definitionItem;
}

public static RoslynDefinitionBucket Create(
StreamingFindUsagesPresenter presenter,
AbstractTableDataSourceFindUsagesContext context,
DefinitionItem definitionItem)
{
var isPrimary = definitionItem.Properties.ContainsKey(DefinitionItem.Primary);

// Sort the primary item above everything else.
var name = $"{(isPrimary ? 0 : 1)} {definitionItem.DisplayParts.JoinText()} {definitionItem.GetHashCode()}";

return new RoslynDefinitionBucket(
name, expandedByDefault: true, presenter, context, definitionItem);
}

public bool TryNavigateTo(bool isPreview)
=> DefinitionItem.TryNavigateTo(_presenter._workspace, isPreview);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,43 +43,37 @@ public VisualStudioFindSymbolMonikerUsagesService(
}

public override async IAsyncEnumerable<ExternalReferenceItem> FindReferencesByMoniker(
DefinitionItem definition, ImmutableArray<SymbolMoniker> monikers,
IStreamingProgressTracker progress, [EnumeratorCancellation] CancellationToken cancellationToken)
DefinitionItem definition, ImmutableArray<SymbolMoniker> monikers, [EnumeratorCancellation] CancellationToken cancellationToken)
{
if (_codeIndexProvider == null)
yield break;

// Only grab the first 500 results. This keeps server load lower and is acceptable for //build demo purposes.
const int PageCount = 5;
Comment on lines +51 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this mean to come in this PR or something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. i pulled this in from my main richnav PR (which i'm still trying to get in).


var convertedMonikers = ConvertMonikers(monikers);
var currentPage = 0;
while (true)
for (var currentPage = 0; currentPage < PageCount; currentPage++)
{
var referenceItems = await FindReferencesByMonikerAsync(
_codeIndexProvider, definition, convertedMonikers, progress, currentPage, cancellationToken).ConfigureAwait(false);
_codeIndexProvider, definition, convertedMonikers, currentPage, cancellationToken).ConfigureAwait(false);

// If we got no items, we're done.
if (referenceItems.Length == 0)
break;

foreach (var item in referenceItems)
yield return item;

// Otherwise, we got some items. Return them to our caller and attempt to retrieve
// another page.
currentPage++;
}
}

private async Task<ImmutableArray<ExternalReferenceItem>> FindReferencesByMonikerAsync(
ICodeIndexProvider codeIndexProvider, DefinitionItem definition, ImmutableArray<ISymbolMoniker> monikers,
IStreamingProgressTracker progress, int pageIndex, CancellationToken cancellationToken)
ICodeIndexProvider codeIndexProvider, DefinitionItem definition,
ImmutableArray<ISymbolMoniker> monikers, int pageIndex, CancellationToken cancellationToken)
{
// Let the find-refs window know we have outstanding work
await using var _1 = await progress.AddSingleItemAsync().ConfigureAwait(false);

var results = await FindReferencesByMonikerAsync(
codeIndexProvider, monikers, pageIndex, cancellationToken).ConfigureAwait(false);

using var _2 = ArrayBuilder<ExternalReferenceItem>.GetInstance(out var referenceItems);
using var _ = ArrayBuilder<ExternalReferenceItem>.GetInstance(out var referenceItems);

foreach (var result in results)
referenceItems.Add(ConvertResult(definition, result));
Expand Down