Skip to content

Dedupe navigate-to results across project flavors#50128

Merged
12 commits merged intodotnet:masterfrom
CyrusNajmabadi:dedupeNavigateTo
Dec 28, 2020
Merged

Dedupe navigate-to results across project flavors#50128
12 commits merged intodotnet:masterfrom
CyrusNajmabadi:dedupeNavigateTo

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Dec 24, 2020

Before:

image

After:

image

Note the duplicate results across netcoreapp3.1 and netcoreapp2.0 are gone and you now just get one result for all those flavors. There are still multiple results due to partial files, and we need to make that better, but that's out of scope here.

--

We accomplish this by deduping results that map back to the same file path and same location in that file. There's no point showing multiple results of that sort as all those results will just end navigating back to the exact same file and location.

A more ideal approach might be some mechanism to show that there are multiple results across all the linked projects for a file. However, that's not really supported in the streaming model that navigate-to currently presents us.

public readonly Document Document;
public readonly DeclaredSymbolInfo DeclaredSymbolInfo;

private readonly Lazy<string> _lazyAdditionalInfo;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

there's no point in being lazy here. we create these values and then immediately marshall them over the OOP boundary, realizing al lthe state of them.

public string? Summary => null;

public readonly Document Document;
public readonly DeclaredSymbolInfo DeclaredSymbolInfo;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

remove these as there's no point being lazy here.

Comment thread src/Features/Core/Portable/NavigateTo/NavigateToSearcher.cs Outdated
// leading to a better condensed view that doesn't look like it contains duplicate info.
lock (seenItems)
{
if (!seenItems.Add(result))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this mean results from documents linked into multiple multi-targeting projects will only be shown in a single multi-targeting project? (e.g. PooledHashSet in Roslyn)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. that's what it means :)

@CyrusNajmabadi CyrusNajmabadi requested a review from genlu December 28, 2020 21:04
Copy link
Copy Markdown
Member

@genlu genlu left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 7f100fc into dotnet:master Dec 28, 2020
@ghost ghost added this to the Next milestone Dec 28, 2020
@DavidKarlas
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi I wonder if you are aware that this is not working for "Go To Implementation" command?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@DavidKarlas Yup. Navigate-to and go-to-impl are different features and need to be updated independently.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants