Skip to content

Add support for navigation to mapped file paths.#45956

Merged
dibarbet merged 8 commits intodotnet:release/dev16.8-preview1from
dibarbet:razor_def
Jul 21, 2020
Merged

Add support for navigation to mapped file paths.#45956
dibarbet merged 8 commits intodotnet:release/dev16.8-preview1from
dibarbet:razor_def

Conversation

@dibarbet
Copy link
Member

@dibarbet dibarbet commented Jul 14, 2020

This supports go to def / impl / far from a c# file to a razor file.

With #45954 we can remove a bit of the logic around opening the file and reading the RDT.

Resolves #45767 and partially #45063 TODO).

@dibarbet
Add support for navigation to mapped file paths.
35e2042 
This supports go to def / impl / far from a c# file to a razor file.
@dibarbet dibarbet added this to the 16.8.P1 milestone Jul 14, 2020
@dibarbet dibarbet requested a review from a team as a code owner July 14, 2020 02:29
dibarbet
dibarbet commented Jul 14, 2020
View reviewed changes
src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs
public bool TryNavigateToSpan(Workspace workspace, DocumentId documentId, TextSpan textSpan, OptionSet options)
{
// Navigation should not change the context of linked files and Shared Projects.
documentId = workspace.GetDocumentIdInCurrentContext(documentId);
Copy link
Member Author

@dibarbet dibarbet Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

All the logic in this method was duplicated across multiple methods, I just combined them all.
There's basically 3 different parameters - TextSpan, line position + character, and just position. The APIs we call require both a TextSpan (just a start and end location) or a VsTextSpan(line positions + characters)

dibarbet
dibarbet commented Jul 14, 2020
View reviewed changes
src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs

static VsTextSpan GetVsTextSpan(SourceText text, int position, int virtualSpace)
{
var boundedPosition = GetPositionWithinDocumentBounds(position, text.Length);
Copy link
Member Author

@dibarbet dibarbet Jul 14, 2020

Choose a reason for hiding this comment

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

moved from line 225 in original file

dibarbet
dibarbet commented Jul 14, 2020
View reviewed changes
src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs
{
var document = OpenDocument(workspace, documentId);
// Before attempting to open the document, check if the location maps to a different file that should be opened instead.
var document = workspace.CurrentSolution.GetDocument(documentId);
Copy link
Member Author

@dibarbet dibarbet Jul 14, 2020

Choose a reason for hiding this comment

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

this is new logic

dibarbet
dibarbet commented Jul 14, 2020
View reviewed changes
src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs Outdated
}
}

private bool TryNavigateToMappedFile(Workspace workspace, ISpanMappingService spanMappingService, Document generatedDocument, TextSpan textSpan)
Copy link
Member Author

@dibarbet dibarbet Jul 14, 2020

Choose a reason for hiding this comment

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

this is also entirely new logic.

@dibarbet
Copy link
Member Author

dibarbet commented Jul 14, 2020

cc @TanayParikh @ajaybhargavb @NTaylorMullen

dibarbet and others added 2 commits July 14, 2020 11:40
@dibarbet @CyrusNajmabadi
Apply suggestions from code review
f140f21 
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
@dibarbet
Address feedback
aca98ab
jasonmalinowski
jasonmalinowski previously requested changes Jul 14, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

This is potentially adding a deadlock risk because of use of GetService in a MEF constructor; that definitely needs to be fixed up. Other comments are less critical. 😄

@dibarbet dibarbet requested a review from jasonmalinowski July 15, 2020 19:27
@dibarbet dibarbet dismissed jasonmalinowski’s stale review July 20, 2020 17:41

stale, addressed comments

@dibarbet dibarbet requested a review from a team July 20, 2020 22:17
@dibarbet dibarbet changed the base branch from master to release/dev16.8-preview1 July 20, 2020 23:06
JoeRobich
JoeRobich approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@dibarbet
Copy link
Member Author

dibarbet commented Jul 21, 2020

@jinujoseph for m2 approval

@dibarbet dibarbet merged commit 0c48c30 into dotnet:release/dev16.8-preview1 Jul 21, 2020
@dibarbet dibarbet deleted the razor_def branch July 21, 2020 17:45
@dibarbet dibarbet mentioned this pull request Sep 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidwengier davidwengier davidwengier left review comments

@JoeRobich JoeRobich JoeRobich approved these changes

@jasonmalinowski jasonmalinowski Awaiting requested review from jasonmalinowski

+1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Approved to merge Area-IDE

Projects

None yet

Milestone

16.8.P1

Development

Successfully merging this pull request may close these issues.

C# -> Razor FAR of Razor Property Not Working

6 participants

@dibarbet @jasonmalinowski @JoeRobich @davidwengier @CyrusNajmabadi @jinujoseph