Added extra items to file menu in PR details view.

[grokys]

There are now four options:

• View Changes
• View File
• Compare With Solution
• Open File In Solution

[jcansdale]

We should maybe deal with the following as part of this PR: #1004 (review)

VisualStudio/src/GitHub.App/Services/PullRequestService.cs

Line 425 in bf27bc6

var tempFileName = $"{Path.GetFileNameWithoutExtension(fileName)}@{commitSha}{Path.GetExtension(fileName)}";

[jcansdale]

At the moment the Working Directory options are only available when on the PR branch.

We should also make them available when the PR has been merged with the current branch (e.g. when looking at an old PR that has already been merged).

[jcansdale]

It seems to work. I'll merge.

[jcansdale]

Something funny seems to be going on. I'm using Compare File on its own PR.

The margin seems to think there are changes, but not the diff view. 😕

[jcansdale]

There's also a unit test complaining that ExtractDiffFiles no longer exists.

[jcansdale]

There's unit test complaining that ExtractDiffFiles no longer exists.

[jcansdale]

I think, "Compare with Working Directory" might look better.

[grokys]

Visual Studio seems to mainly use title casing even on articles and prepositions, for example "Edit -> Insert File As Text", "Edit -> Bookmark -> Previous Bookmark In Document" (although it is inconsistent - it uses e.g. "Attach to Process"). I agree though, it does look better to me the way you suggested...

[jcansdale]

Again, maybe change this to "Open File in Working Directory".

[jcansdale]

Users could easily miss the fact that the "Working Directory" commands are writable.
Maybe change this label to, "View File (read-only)"?

[jcansdale]

Similarly with "Compare File (read-only)" above.

[jcansdale]

I think my preference would be to simplify the menu slightly to this:

(here's the current layout for comparison)

Having used various iterations of this functionality, I have to wonder if having the View File option is worth it.

1. We risk having a bewildering array of open file options.
2. The read-only file isn't highlighted with warnings/errors which has given me a false sense of security in the past.
3. The read-only file isn't navigable, which can be surprising/frustrating.
4. You can get the same functionality via Compare File and show Right file only.

Simplifying it to 3 options gets it more under control. What do you think?

[grokys]

Hmm, not sure:

1. I think View File could be quite useful and it gives the menu a symmetry: 1 open and 1 compare for both PR and working directory.
2. "View File (read-only)" would work well but "Compare File (read-only)" seems strange to me - I don't expect a diff to be editable, so why are you pointing it out? Also is there anywhere else in VS that has "(read only)" in menus? It doesn't strike me as something a polished UX would have.
3. "Compare File" and show "Right File Only" is rather long-winded and more importantly subsequent diffs will open in that mode, which isn't convenient.

I actually find the screenshot of the menu you posted more cluttered than the current one, with the "(read only)" in there and lack of symmetry, but maybe it's just because I'm not used to it.

As ever though, I'll defer the decision to someone who is better at UX than me - @donokuda?

[jcansdale]

I think View File could be quite useful

I'm curious when you might find it useful? I think the PR file is only really of interest relative to the PR/merge base. Standalone it doesn't let you navigate or highlight info/warnings (e.g. superfluous using statements). It's just extra mental load then choosing which option to go for.

"Compare File (read-only)" seems strange to me

Yup, I get it. The thing is, Compare with Working Directory is writable and this happens to by quite useful (but not necessarily discoverable). Compare with Working Directory (writable) would be getting a bit long winded. 😉 Ideas anyone?

"Compare File" and show "Right File Only" is rather long-winded and more importantly subsequent diffs will open in that mode, which isn't convenient.

Fair point.

I actually find the screenshot of the menu you posted more cluttered than the current one, with the "(read only)" in there and lack of symmetry, but maybe it's just because I'm not used to it.

How about this:

At the moment when we open a Diff, the tab looks like this:

Compared to when we Open File.

Perhaps we could prefix the file with PR File -. That might make it a little more obvious that is isn't an editable file from your current solution?

[jcansdale]

I'm going to be sad when actively working on a PR that the default action isn't to open a live version of that file. 😢 I get what @shana says about the changing behavior being potentially surprising, but I'll have to train myself to not use the default action but one of the ...Working Directory options instead.

A better option might be to make Open File in Working Directory the default when working on the PR branch and Compare File the default when viewing from a different branch. That way there would be an obvious visual difference between the two modes (one's a diff view and the other a file view). These would be the default actions I'd want 95% of the time, depending on the context.

[shana]

I'm going to be sad when actively working on a PR that the default action isn't to open a live version of that file. 😢 I get what @shana says about the changing behavior being potentially surprising, but I'll have to train myself to not use the default action but one of the ...Working Directory options instead.

@jcansdale Contextually, we are in the PR details window, and we are showing a list of files with a header saying "Changes". The most obvious action that can reasonably happen from this context when the user double clicks the file on the changes list is:

• it opens the diff to the changes made to that file on this PR. It's called Changes, it's in a PR detail view, it follows that it will show the changes of the file in that PR

A diff that includes markers for all the changes that occurred in the PR PLUS all the changes that have ocurred locally that haven't yet been pushed is a complex view that is mixing two completely separate pieces of information, with no visual distinction between which ones are coming from the local directory and which ones are coming from previous commits. For a user to understand what is going on in a view like this, they should open it with a clear intention - clicking on an action that clearly states that it is going to open a live diff that mixes all the changes together in one view.

You don't need this intention because you wrote it, you know exactly how it works, but users don't, and they can't guess at what's happening. We need to be focused on what the default actions are, what their context is, and how the users will use them based on the context we establish.

The PR details view is part of a code review workflow. We established the maintainer workflow as part of a pair of maintainer and contributor workflows where a maintainer reviews PRs and sends feedback to the contributor, and where we fill in the gaps for all the missing steps that users need to do manually in order to review a PR. That is the context in which these features are being built - a code review process. In this context, the user focus is to review the changes made by someone else and provide feedback on them. Having the file click action show show a live diff view by default isn't the most valuable action for this workflow.

A better option might be to make Open File in Working Directory the default when working on the PR branch and Compare File the default when viewing from a different branch. That way there would be an obvious visual difference between the two modes (one's a diff view and the other a file view). These would be the default actions I'd want 95% of the time, depending on the context.

Opening the file in the working directory doesn't work towards the goal of providing the user with a quick tool for reviewing changes, which is the purpose of the code review workflow.

As for having different defaults depending on the branch you're in, that's definitely something we should not be doing. Human-computer interface guidelines everywhere will tell you that you never ever change what an action does without changing it's UI completely. We're training users to expect an action to happen when they click on a thing, and changing what it does based on some arbitrary hidden logic that they can't guess at will only confuse and frustrate users.

[grokys]

@jcansdale updated to use (hopefully) final naming if you want to give some 👀 .

[jcansdale]

Here's what is looks like now for reference:

I think the naming is fine now. 👍

Now that comments are no longer appear on the editor window, I have even more reservations about the usefulness of View File. It lets you see the file without any context (what has changed) or comments. The content is exactly the same as the right hand side of the View Changes diff. This is an option that I'd only ever choose by accident.

Given that commands are a lot easier to add then remove, my preference would be to remove this for the moment and revisit it a future release.

I know we're up against it time wise and certainly don't want to block this it others have feelings in the opposite direction! I wanted to at least mention it though...

[jcansdale]

As much as I might dislike the View File option, I'm going to hold my nose and merge. 😉

Here goes...