New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Enable navigation from diff view to editor #1407

Merged
merged 34 commits into from Feb 6, 2018

Conversation

Projects
5 participants
@jcansdale
Contributor

jcansdale commented Jan 3, 2018

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:

image

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

image

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

image

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.
    image

Related issues

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

This comment has been minimized.

Contributor

grokys commented Jan 8, 2018

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 added some commits Jan 4, 2018

Trigger navigation using command dispacher
Currently uses VSStd2KCmdID.RETURN command.
Add simple line matching algorithm
This will fix navigation for the common case where a few lines have been added or removed from the working file.
There is no fuzzy matching, it will simply pick the nearest matching line.
@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Jan 11, 2018

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.

jcansdale added some commits Jan 11, 2018

@donokuda

This comment has been minimized.

Member

donokuda commented Jan 11, 2018

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.

Add `Open File in Solution` context menu item
Added to `GitHub` submenu, but this isn't very discoverable.
@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Jan 12, 2018

@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

This comment has been minimized.

Contributor

jcansdale commented Jan 12, 2018

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

image

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

This comment has been minimized.

Contributor

jcansdale commented Jan 12, 2018

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

image

Here's another possibility:

image

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

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Jan 12, 2018

@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:

image

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):

image

Does that make sense?

Warn if user tries to open solution file that isn't checked out
Stop user from opening solution files that aren't related to the target PR diff file.
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Jan 12, 2018

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

This comment has been minimized.

Member

donokuda commented Jan 12, 2018

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):

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

This comment has been minimized.

Contributor

jcansdale commented Jan 12, 2018

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

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

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

Advertise Open File in Solution on Inline Comment view
Make the `Open File in Solution` command and keyboard shortcut discoverable by advertising on the inline comment view task bar.

@jcansdale jcansdale added this to In progress in 2.4.0 Jan 19, 2018

meaghanlewis and others added some commits Jan 19, 2018

Revert "Advertise Open File in Solution on Inline Comment view"
Was rough and non-functional!
This reverts commit 9857cdc.

@jcansdale jcansdale changed the base branch from master to feature/pr-reviews Feb 1, 2018

@ghost

ghost approved these changes Feb 1, 2018

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 2, 2018

@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! 😉

Rename NavigationService to PullRequestEditorService
Move service class/interface to GitHub.Exports.Reactive project.

@jcansdale jcansdale changed the base branch from feature/pr-reviews to master Feb 2, 2018

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 2, 2018

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

@grokys grokys added this to In progress in 2.4.3 Feb 6, 2018

@grokys

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

namespace GitHub.VisualStudio.Views.GitHubPane
{
/// <summary>
/// Used to filter commands send to <see cref="IVsTextView"/> dispatch them to a <see cref="Exec"/> event.

This comment has been minimized.

@grokys

grokys Feb 6, 2018

Contributor

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?

@grokys

grokys approved these changes Feb 6, 2018

@jcansdale jcansdale merged commit 2dd3ce5 into master Feb 6, 2018

3 checks passed

GitHub CLA @jcansdale has accepted the GitHub Contributor License Agreement.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

2.4.0 automation moved this from Punted to Done Feb 6, 2018

2.4.3 automation moved this from In progress to Done Feb 6, 2018

@jcansdale jcansdale deleted the feature/navigate-diff-to-editor branch Feb 6, 2018

grokys added a commit that referenced this pull request Feb 15, 2018

Port changes from #1407.
Enable navigation from diff view to editor

grokys added a commit that referenced this pull request Mar 16, 2018

Port changes from #1407.
Enable navigation from diff view to editor

grokys added a commit that referenced this pull request Mar 19, 2018

Port changes from #1407.
Enable navigation from diff view to editor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment