Ensure we do not need to go back to compilations/symbols when a user selects a navbar item.#53887
Conversation
| { | ||
| await NavigateToSymbolItemAsync(document, (RoslynNavigationBarItem.SymbolItem)item.UnderlyingItem, cancellationToken).ConfigureAwait(false); | ||
| await NavigateToSymbolItemAsync(document, item, (RoslynNavigationBarItem.SymbolItem)item.UnderlyingItem, textSnapshot, cancellationToken).ConfigureAwait(false); | ||
| return true; |
There was a problem hiding this comment.
c# no longer needs to do any sort of complex processing logic when navigating. VB still does, but it's much more minor (And can hopefully be removed ina followup).
| Contract.ThrowIfNull(symbolItem.Location.OtherDocumentInfo); | ||
| var otherLocation = symbolItem.Location.OtherDocumentInfo.Value; | ||
| return Task.FromResult((otherLocation.documentId, otherLocation.navigationSpan.Start, 0)); | ||
| } |
There was a problem hiding this comment.
by default, all symbol navigation can be driven entirely based on the information stored in the item.
| OfType(Of RoslynNavigationBarItem.SymbolItem). | ||
| ToList() | ||
|
|
||
| Assert.True(navigableItems.Count() = navigableItems.Distinct(New NavigationBarItemNavigationSymbolComparer(isCaseSensitive)).Count(), "The items were not unique by SymbolID and index.") |
There was a problem hiding this comment.
will see about restoring some sort of check here. but this part was all based on symbolkeys which just has no meaning now.
| Dim text = Await destinationDocument.GetTextAsync(cancellationToken).ConfigureAwait(False) | ||
| Dim navPoint = NavigationPointHelpers.GetNavigationPoint(text, indentSize:=4, methodBlock) | ||
| Return (navigationLocation.documentId, navPoint.Position, navPoint.VirtualSpaces) | ||
| End If |
There was a problem hiding this comment.
this part is still unfortunate. VB has some really funky legacy behavior here around jumping to a blank line (including into virtual space). I just didn't want to clutter the underlying api even more with this, so i kept this part at the vb editor level. Note though that at least now we still avoid semantics. All the work done here just uses syntax/text, so it should still be better than before.
| { | ||
| _nextIds.TryGetValue(id, out var nextId); | ||
| _nextIds[id] = nextId + 1; | ||
| return nextId; |
There was a problem hiding this comment.
i literally cannot even understand what this logic could have ever meant.
| RoslynNavigationBarItem item, Document document, SourceText text, string? containerName = null) | ||
| { | ||
| if (item is not RoslynNavigationBarItem.SymbolItem symbolItem || symbolItem.SelectionSpan == null) | ||
| if (item is not RoslynNavigationBarItem.SymbolItem symbolItem || symbolItem.Location.InDocumentInfo == null) |
There was a problem hiding this comment.
@dibarbet for confirmation. FOr LSP i'm assuming it only supports document symbols navigating within the document.
There was a problem hiding this comment.
Interestingly it seems to depend on which type we return. SymbolInformation defines its span as a Location which contains both the document and location inside the document. But DocumentSymbol (the newer type) defines the spans as Range which does not not have a document piece. So it seems like SymbolInformation does support navigation between documents, but DocumentSymbol does not.
Additionally, neither DocumentSymbol nor SymbolInformation allow multiple ranges, which we would definitely need for partial declarations. I'm going to see if there are existing issues on these, otherwise I'll file them
There was a problem hiding this comment.
Feature request for allowing location in DocumentSymbol
microsoft/language-server-protocol#1255
There was a problem hiding this comment.
lol. i love the incosistencies.
There was a problem hiding this comment.
Feature request for multiple locations
microsoft/language-server-protocol#1285
| { | ||
| if (item.NavigationSymbolIndex < symbols.CandidateSymbols.Length) | ||
| { | ||
| symbol = symbols.CandidateSymbols[item.NavigationSymbolIndex]; |
There was a problem hiding this comment.
i legit do not believe this was ever safe or sensible. esp in an oop world.
src/EditorFeatures/Core/Extensibility/NavigationBar/AbstractEditorNavigationBarItemService.cs
Show resolved
Hide resolved
|
|
||
| // See if there are any references in the starting file. We always prefer those for any symbol we find. | ||
| var referencesInCurrentFile = allReferences.WhereAsArray(r => r.SyntaxTree == tree); | ||
| if (referencesInCurrentFile.Length > 0) |
There was a problem hiding this comment.
is there a possible case where there are caret locations in the current document, but the navigation action should take you to a different document?
It seems like the answer is no for partial types (navigation just takes you to the declaration in the file). And for partial methods it looks like currently 2 different items in the navbar, one for the declaration and one for the implementation. So I'm guessing this case doesn't matter but wanted to confirm
There was a problem hiding this comment.
is there a possible case where there are caret locations in the current document, but the navigation action should take you to a different document?
I can technically think of a case. You could technically have a symbol that had a part in the current file that was totally hidden. In that case we should instead navigate to the location in another file that was not hidden...
However, we didn't really support that before, so i'm ok with this limitation staying.
Fixes #53646
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1334030
Followup to #53878.
This is part of a larger refactoring for navbars. The final form will be that a navbar item can store:
This PR does '2' and '3'.