Improve the Find-References UI for multi-targetting projects#52618
Improve the Find-References UI for multi-targetting projects#52618CyrusNajmabadi merged 36 commits intodotnet:mainfrom
Conversation
| /// </summary> | ||
| private readonly Dictionary<ISymbol, DefinitionItem> _definitionToItem = | ||
| new(MetadataUnifyingEquivalenceComparer.Instance); | ||
| private readonly Dictionary<SymbolGroup, DefinitionItem> _definitionToItem = new(); |
There was a problem hiding this comment.
it is intentional that we don't have the equivalence comparer here. That merging logic moves into SymbolGroup, so it doesn't have to be specified here.
There was a problem hiding this comment.
The new concept in this pr is a 'SymbolGroup'. Instead of treating all the symbols we find from checking linked-files as independent symbols, we now treat them as one 'symbol group'. We will show this group as just a single entry in teh FAR UI, though with info to indicate which projects it is valid in.
|
|
||
| protected abstract ValueTask OnDefinitionFoundWorkerAsync(DefinitionItem definition); | ||
|
|
||
| protected async Task<(Guid, string projectName, SourceText)> GetGuidAndProjectNameAndSourceTextAsync(Document document) |
There was a problem hiding this comment.
this was moved to another file as we need it in a couple of places now.
| } | ||
|
|
||
| private string ComputeCombinedProjectName() | ||
| { |
There was a problem hiding this comment.
helper logic moved to a different location so it coudl be shared.
|
@CyrusNajmabadi I haven't looked much at the code yet (I'll review a bit later today), but wanted to check if this also applied to the LSP version? I could imagine if we're updating the definition node with new text it has a chance of not working properly. |
| searchSymbol, _solution, _cancellationToken).ConfigureAwait(false); | ||
|
|
||
| return new SymbolGroup(searchSymbol, linkedSymbols); | ||
| } |
There was a problem hiding this comment.
this is the core change in the find-refs low level code. Instead of having a LinkedSymbolFinder, we just have the core engine do the linking. We then represent this as a first class concept (the 'symbol group').
…s/ReferenceFinders.cs
…s/ReferenceFinders.cs
Definitely a good point. I'm not wedded to it. If we think it's not necessary, it's trivial to remove :) |
|
I think I agree with Sam, the group having the frameworks listed confused me a bit at first, because not every result is multi-targeted. Not sure its adding any value. |
Ok! i will remove :) |
|
This is awesome! I'll note that test explorer has the same problem, I don't want to run all tests twice or more in each local run. Even if you manage to remove unwanted tests, (right click + run) would include all target frameworks. So some filtering/preference would be nice in that area. |
|
Tag @shyamnamboodiripad for the Test Explorer feedback above. |
|
I agree this is awesome! 👏🏾 @alrz Thanks for the the Test Explorer feedback. The Test Explorer window currently splits tests into different projects by TFM (similar to how projects appear in the dropdown menu in the code editor window). This should allow for running tests from individual TFMs / viewing results from all TFMs side by side.
If you wish to scope the view to just a single TFM, there are a couple of options. You probably know about this already but I'll mention them here in case it proves helpful for others. A. You could group your tests by Target Framework as the top level. In the following screenshot, I have grouped by Target Framework, followed by Project and then Class. After this you can limit interactions to the TFM that you care about most and leave the remaining TFMs unexpanded.
You can also use this custom grouping to remove the Project level entirely and place the Target Framework level directly under the Class / Namespace level to view results in other interesting ways. B. The second option would be to create a 'playlist' that only includes tests from the TFM that you care about. The playlist can be made 'dynamic' e.g. include all tests from a given project including ones that are added in the future. Here's how you can achieve that today -
The above experiences can certainly be improved / streamlined further. We have issues on our backlog to -
If you have feedback / suggestions about ways to improve the experience it would be great to log it via the 'Send Feedback' tool inside Visual Studio as this would also allow the broader community to vote on your suggestions which helps us in our prioritization :) Thanks again! |
PR should be reviewed with whitespace changes off.
Currently our Find-Refs UI can be pretty unpleasant when dealing with multi-targetting projects. As roslyn internally represents tehse as separate projects, with their own separate references to linked files, you end up with situations like the following:
There's a lot of duplication here that makes it hard to navigate and get to the locations you care about. First, the symbol itself is duplicated since it is defined in a file linked into two separate project contexts. We view each of these symbols as unique (as each may have distinct references). We then have the references per symbol. Again, there is a lot of duplication here as several (though not all) of the references appear for both views of the symbol.
We recently improved a similar problem for navigate-to here: #50128
In that PR we changed it so that we merge effectively equivalent results, just adding more information to the display showing that the merged item comes from multiple locations. Like so:
This PR brings a similar concept to FindRefs, merging both definitions that are effectively equivalent, as well as references. Looking at the scenario above, this reduces the UI down to:
This is already a lot better. We can see that there's effectively one definition (albeit for several project flavors), and tehre are only 5 references (though three of them come from a multi-flavored location).