[Feature] Enable navigation from diff view to editor

[jcansdale]

Functionality

• This feature will enable easy navigation from a read-only PR diff file, to the same file inside the editor.

MVP

• Hit the Enter key when in PR file diff view (double-click on PR file)
  • Command will navigate to corresponding working file and line
• Hit the Enter key when in PR file view (right-click on PR file and View File)
  • Command will navigate to corresponding working file and line
• Right-click on diff view and Open in Solution File
  • This is a more discoverable but less convenient way to navigate to working file
• Line matching algorithm
  • It will navigate to the nearest matching line to the original line number. When a PR branch is first checked out this will always be correct.
  • The algorithm will prioritize lines that are unique in the destination document (e.g. a method definition would be unique but a { line likely wouldn't be).
  • If the target line isn't unique in the destination document, the lines above the target line will be considered (currently 4 lines will be considered). If one of them is unique, it will be used instead (with the required offset form the target line).
  • As well as reducing the number of false positives, this allows navigation to work even when the target line has been changed (a common scenario when there is an inline comment).
  • If line matching fails completely, the original line number (and column 0) will be used. This should take the user near where they wanted to be.
• Surface Open Solution File at Caret command and keyboard shortcut on inline comment view
  • The user is most likely to want to navigate to solution file when viewing an inline comment. Adding a tool bar button here makes it easily discoverable in this context, but also advertises the keyboard shortcut for use in other contexts (when there are no comments on the diff view).

Screengrabs

The Open in Solution File command is surfaced on the code context menu:

Attempting to open a PR file that isn't checked out will bring up this:

Open Solution file command and keyboard shortcut is discoverable on Inline Comment task bar

To do

• Add metrics for navigate to live file command
• Make sure functionality works for renamed files

Discoverability

Moving this to separate issue / PR:

• Make functionality discoverable from inline comment view

At the moment there is no visual indication that this feature exists. The user must open a PR diff file and know to hit the Enter key.

The Enter key is used for navigation to source files in a number of contexts inside Visual Studio. It can be used to navigate to the selected file from Solution Explorer or to a displayed file, line and column from the Output window (e.g. for build errors). It is also used by the Changes tree view inside Team Explorer, but currently not on our GitHub Changes tree.

To make it more discoverable we might:

• Mention this navigation option on status bar after opening a PR diff file
• Add an explicit Go To Working File command on the code context menu
• Put a tooltip/note on the inline comment view that advertises this shortcut
• Put an explicit edit button on the inline comment view (pencil icon borrowed from .com's edit file), e.g.
  

Related issues

• Added extra items to file menu in PR details view: Added extra items to file menu in PR details view. #1036
• Contextually, we are in the PR details window: Added extra items to file menu in PR details view. #1036 (comment)

[grokys]

I think this feature would be very useful, but (as you mention yourself) I don't think hitting any key is the correct way to expose this:

1. It's not very discoverable
2. Worse, it's "accidentally discoverable" (yes I just invented that term) - I can imagine someone hitting a key and having no clue as to why they've been navigated away from the think they were looking at!

I'm not sure what a better UI would be. I like the "edit button" in your mockup above, though that is only available when there are comments in the diff. Maybe simply a context menu entry?

[jcansdale]

I think this feature would be very useful, but (as you mention yourself) I don't think hitting any key is the correct way to expose this.

Hitting any key was just a way to get something working. In the latest revision I've changed it to use a VS command rather than a key.

I'm currently using the Edit.BreakLine command (i.e. hitting enter). To me this feels very natural. This is used for navigating to the code window in a number of contexts. You can use if from the Output window to navigate to a displayed file:line or from Solution Explorer to navigate to the currently selected item.

Worse, it's "accidentally discoverable" (yes I just invented that term) - I can imagine someone hitting a key and having no clue as to why they've been navigated away from the think they were looking at!

I agree that accidentally hitting a random key and ending up on a different view would be weird. Since there is already precedent for Enter to navigating to the editor, I don't think this would feel wrong. In fact, if the user did hit Enter hoping to edit the document, accidentally discovering this command would be a good thing.

Since this command is only ever active on a read-only view, is there anywhere else it would make sense to navigate to?

I'm not sure what a better UI would be. I like the "edit button" in your mockup above, though that is only available when there are comments in the diff. Maybe simply a context menu entry?

Indeed and the majority of PR probably don't even have inline comments. This is definitely something we'd want to have easily accessible on all read-only PR files. A context menu entry would certainly make sense, but users wouldn't necessarily notice it on an already cluttered context menu.

[donokuda]

I took this out for a test drive!

It looks like we don't checkout the PR branch when we switch from the diff view to the editor view. Can we change it so that we do? Otherwise, I'll end up editing the file in the wrong branch or be confused why some of the file's content has disappeared or is outdated.

I'm not sure what a better UI would be. I like the "edit button" in your mockup above, though that is only available when there are comments in the diff. Maybe simply a context menu entry?

I personally don't have a problem having the feature as a whole only available to comments in the diff. I interpreted this feature originally as a part of the inline review flow, specifically when I'm ready to address feedback.

One thing that I noticed is that it would be nice is a way to quickly reference the feedback left after navigating to the editor view. Otherwise, I'll need to jump back and forth between the two views. This would be something that would need further speccing/designs and I don't think should hold up this pull request. However, I'm more than happy to continue the conversation in a separate issue.

[jcansdale]

@donokuda,

It looks like we don't checkout the PR branch when we switch from the diff view to the editor view. Can we change it so that we do? Otherwise, I'll end up editing the file in the wrong branch or be confused why some of the file's content has disappeared or is outdated.

We certainly don't want users inadvertently editing files on the wrong branch! In order of complexity, we could:

1. Disable this feature when the PR branch isn't checked out (like we do with the Open File in Solution command on the PR Changes tree).
2. Alert the user that the PR branch isn't checked out (highlight status bar or dialog box) when they attempt to switch.
3. Show the user a dialog with either Checkout or Cancel when they attempt to switch.

I personally don't have a problem having the feature as a whole only available to comments in the diff. I interpreted this feature originally as a part of the inline review flow, specifically when I'm ready to address feedback.

This feature certainly dovetails nicely with the inline review flow, but I've found myself using it as a standalone feature on PRs that don't have comments. The PR Changes tree is a handy way to filter files that are likely to be of interest when working on a PR.

If for example there is a change part way into a large file on that is on the PR Changes list. Then the PR file diff opens, it automatically navigates to the first change in that file. The user can simply hit Enter to be taken to the place they most likely want to edit. This flows particularly nicely now you can hit Enter on the Changes tree to bring up the diff view (a change @StanleyGoldman made to make it behave consistently with other parts of VS).

One thing that I noticed is that it would be nice is a way to quickly reference the feedback left after navigating to the editor view. Otherwise, I'll need to jump back and forth between the two views. This would be something that would need further speccing/designs and I don't think should hold up this pull request.

Yes, definitely! I think this would be part of inline reviews in the editor.

The current situation isn't actually too bad, because the user can hit Ctrl + Tab to cycle between the editor view and the diff view (it will go back and forward between them). This is a shortcut that VS users will be familiar with and it will feel pretty intuitive.

[jcansdale]

I've now surfaced this command on the diff view context menu / GitHub sub-menu.

It's pretty awkward to navigate to, not very discoverable and against the Visual Studio UI guidelines to have a single item on a sub-menu. 😉 I'll experiment to find a better location for it.

I've labeled the command Open File in Solution to be consistent with the sub-menu item on the PR changes context menu.

VS has a number of commands that navigate to different parts of the solution, e.g. Go To Definition and Go To Implementation. I'm wondering if Go To Solution File would be more consistent with these and hint that it does more than simply opening the file (it also navigates to the selected line).

I'll experiment and see how it looks/feels.

[jcansdale]

The menu item currently appears at the bottom (the same place as the GitHub sub-menu):

Here's another possibility:

Alas it's a mock-up since menu item positioning is ridiculously tricky in VS. 😭

[jcansdale]

@donokuda,

It looks like we don't checkout the PR branch when we switch from the diff view to the editor view. Can we change it so that we do? Otherwise, I'll end up editing the file in the wrong branch or be confused why some of the file's content has disappeared or is outdated.

Since there are things that can go wrong when checking out a PR branch, I thought it would be best if we point the user the right direction (and let them continue down that workflow) rather than automatically initiating a checkout.

The next commit will make it pop up a simple dialog when it detects the user is on a different branch to the PR diff file:

This is similar to what MS does when user tries to Go To Definition on a symbol that isn't part of the current solution (e.g. when the user is browsing PR files without checking them out):

Does that make sense?

[meaghanlewis]

I think this is looking great @jcansdale 🙌 . I like the most recent change you made with the prompt to checkout a branch first. It's a good reminder for users to be on the branch before making any changes.

Also, it's really easy to switch between the diff view and editor (as long as you know how to do so!) and it reminds me a lot of how easy it is to switch between the diff and edit files on dotcom as well.

The hardest part with this feature is discoverability... The menu item you added on right-click is nice if users know to look there, but I don't think they will.

I really like the earlier idea you suggested of an "explicit edit button on the inline comment view". Although that would seem to tie this feature with inline comments, at least there will be a visible option to switch between the diff and editor.

[donokuda]

I've been playing around with the built-in diff view in VS, and it looks like it supports what the original goal of this PR (being able to edit a file from a diff):

I might need a refresher, but is there a reason why we are using read-only diffs in this situation? Is it to prevent losing the review comments? Can we consider scoping this feature to only diffs with comments in them?

[jcansdale]

@donokuda I've added the following under related issues:

• Contextually, we are in the PR details window: Added extra items to file menu in PR details view. #1036 (comment)
• Added extra items to file menu in PR details view: Added extra items to file menu in PR details view. #1036

I'll flesh out some more when I'm back on Monday...

[jcansdale]

@grokys,

Metrics

Done

Some brief doc comments, e.g. what does TextViewCommandDispatcher do? From first glance I have no idea, would be nice to have a brief doc comment to give me a starting point

Done

These changes overlap with changes on the feature/pr-reviews branch, might be worth pre-emtively merging the changes from that branch?

This PR now targets feature/pr-reviews

I hope feature/pr-reviews isn't 1,000,000 miles away from being ready to ship! 😉

[jcansdale]

@grokys, Okay, I've renamed/moved NavigationService to PullRequestEditorService in the GitHub.Exports.Reactive project. 😉

[grokys]

Just a nit about a doc comment, but other than that LGTM!

[grokys]

This sentence doesn't make sense grammatically. Also might be useful to say why this class is needed in the <remarks>? That is, why do commands need to be filtered (and?) dispatched to a(n) Exec event?