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

RFC for multi-file diff #1753

Merged
merged 20 commits into from Nov 20, 2018

Conversation

7 participants
@vanessayuenn
Contributor

vanessayuenn commented Oct 22, 2018

This is a WIP RFC summarizing the dicussion in #1689 (btw should we close that issue in favor of keeping the discussion here?). A few of the sections are left as TBD for now and will be filled in as the conversation continues and things become clearer in terms of direction.

📜Rendered Version

Cc-ing @kuychaco @smashwilson @annthurium @simurai for input & mock-up help. 🙇‍♀️

vanessayuenn and others added some commits Oct 22, 2018

@coveralls

This comment has been minimized.

coveralls commented Oct 22, 2018

Coverage Status

Coverage decreased (-0.01%) to 84.861% when pulling f0869d3 on multi-diff-rfc into 0e906e2 on master.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 23, 2018

Thanks @vanessayuenn for getting this off to a great start! I appreciated the thoughtfulness of your "Drawbacks" section. It inspired me to think of an alternative approach that might be easier and more straight forward to design and implement.

cmd-click to select multiple files might not be as universally known as we assume, so that might affect discoverability of this feature.

I agree with you that it might be tricky to get the discoverability and UX of this just right. Instead we can opt to lessen scope and simply offer a way for users to view the collective set of staged changes that they are about to commit.

This could look like a button above the commit message box that, when clicked, opens a multi-file diff pane item called something like "Commit Preview" and shows a summary of what will go into the user's next commit based on what is currently staged.

Benefits to this approach:

Minimal scope:
It's the simplest use case for a multi-file diff that I can think of, allowing us to focus primarily on building out the multi-file diff item and not having to worry about other complex or nuanced UX interactions

Pareto Principle (aka 80/20 rule)

80% of the results will come from just 20% of the actions
This proposal covers the most common use case for viewing changes for multiple files prior to committing. When I imagine a user's workflow, I actually can't think of any other use cases. I'm sure they exist, but it's probably not worth the added complexity to support them.

Educate and delight users:
By using helpful language like "View changes to be committed" and "Commit Preview" we can help make Git more transparent and understandable to users. For example, when teaching Git I often encounter users who are confused by Git's two-stage commit process and don't immediately see the value for it. Thus they never partially stage any changes.

We can also encourage good practices like reviewing all the changes you are about to commit. This would be a nice addition to the flow that currently exists in our panel: (from top to bottom) view unstaged changes -> stage changes to be committed -> view all staged changes to be committed -> write commit message that summarizes all changes -> hit commit button -> see commit appear in recent commits list

Here's my best shot at creating a visual for ya:

git_ ___src_test-repo_and_how-we-work_md_ ___github_github

@simurai what does your UX spidey sense say about this?

@kuychaco

Left some suggestions and one request for elaboration

Show resolved Hide resolved docs/rfcs/004-multi-file-diff.md Outdated
#### Unstaged Changes pane
- User can `cmd+click` and select multiple files from the list of unstaged changes, and the pane on the left (see multi-file diff section below) will show diffs of the selected files. That pane will continue to reflect any further selecting/unselecting on the Unstaged Changes pane.
- Once there is at least one file selected, `Stage All` button should be worded as `Stage Selected`.

This comment has been minimized.

@kuychaco

kuychaco Oct 23, 2018

Member

Once there is at least one file selected, Stage All button should be worded as Stage Selected.

Personally I think I prefer the current functionality where the Stage All button remains, regardless of how many changed files are selected. It's kind of like in the diff view where there's a Stage File button at the top. It's a quick way to stage everything regardless of what's selected. I could possibly be convinced otherwise though, especially if there's UXR that shows that users would expect the button to change.

Users can currently stage selected files by hitting enter or right-clicking and selecting Stage. Admittedly these are less discoverable than a button that says Stage Selected

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

I'm 👍 for leaving the "Stage All" behavior as it is, I think. At the very least we can revisit independently from this RFC to keep our scope under control 😉

_(note: The following is a summary of what we would like the UX to achieve, but I don't have a clear visuals of what that looks like yet.)_
- Shows diffs of multiple files as a stack.
- Each diff should show up as its own block, and the current functionality should remain independent of each block.

This comment has been minimized.

@kuychaco

kuychaco Oct 23, 2018

Member

Each diff should show up as its own block, and the current functionality should remain independent of each block.

Not quite sure what this means 🤔. Could you please elaborate?

This comment has been minimized.

@vanessayuenn

vanessayuenn Oct 24, 2018

Contributor

I realize now that it was quite badly worded. I meant the current functionality (e.g. the open file, stage file, etc from the header) of the diff view for the file it's showing should remain to be file-specific, even though the view would show more than one file. Does that clarify things? If not I can attempt to illustrate with mock-ups. Words are hardddd...

This comment has been minimized.

@annthurium

annthurium Oct 24, 2018

Contributor

I'm also not sure what this means. In a visual sense, do you mean each file in the diff is separated visually by a border and some margin?

This comment has been minimized.

@kuychaco

kuychaco Oct 24, 2018

Member

Yup I think that makes sense! Thanks for the clarification. Maybe "Each diff retains the controls it currently has in its header (e.g. the open file, stage file, undo last discard, etc)"

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

Oh interesting. So, effectively, we move the FilePatchItem's header controls as-is to the per-file banner within the multi file patch view? I like that - it'd be a lot easier than trying to figure out which file the file-specific buttons map to 😆

Show resolved Hide resolved docs/rfcs/004-multi-file-diff.md Outdated
Show resolved Hide resolved docs/rfcs/004-multi-file-diff.md
## Drawbacks
- `cmd-click` to select multiple files might not be as universally known as we assume, so that might affect discoverability of this feature.

This comment has been minimized.

@kuychaco

kuychaco Oct 23, 2018

Member

Really good point 👍

This comment has been minimized.

@annthurium

annthurium Oct 24, 2018

Contributor

I knew about cmd-click, but didn't realize Atom supported it in the staged files area. I wonder if there's a way we can educate users about this, either in product education / tutorials, or with marketing and educational materials (videos, etc.)

Show resolved Hide resolved docs/rfcs/004-multi-file-diff.md Outdated
#### Unstaged Changes pane
- User can `cmd+click` and select multiple files from the list of unstaged changes, and the pane on the left (see multi-file diff section below) will show diffs of the selected files. That pane will continue to reflect any further selecting/unselecting on the Unstaged Changes pane.

This comment has been minimized.

@simurai

simurai Oct 24, 2018

Member

Another way is to click + drag up/down on multiple files. Kinda hard to discover (I just rediscovered it 😄), but quite nice once you know.

@simurai

This comment has been minimized.

Member

simurai commented Oct 24, 2018

@kuychaco This could look like a button above the commit message box that, when clicked, opens a multi-file diff pane item called something like "Commit Preview" and shows a summary of what will go into the user's next commit based on what is currently staged.

I really like the idea of a "Commit Preview". 👍 Here 3 alternatives to the button above the commit message box.

1. Use the headers to select all files

Clicking on "Unstaged Changes" or "Staged Changes" would select all files (for each section) and thus show the multi-file diff on the left.

screen shot 2018-10-24 at 5 31 52 pm

Downside: Harder to discover. Might might can add a hover style.

2. Add a "Preview" button next to the commit button

The flow would be more like 1. Stage 2. Write commit message 3. Preview (optional) 4. Commit

screen shot 2018-10-24 at 5 41 43 pm

Downside: There is no preview button for all unstaged changes. Long branch names get cut off.

3. Show a preview automatically when the commit message has focus

No buttons to click, always get a preview when typing the commit message.

screen shot 2018-10-24 at 5 46 27 pm

Downside: This could feel jarring and unexpected.


Anyways, I'll try to add some mockups tomorrow.

@vanessayuenn

This comment has been minimized.

Contributor

vanessayuenn commented Oct 24, 2018

@kuychaco I am very into your idea of commit preview! I originally typed a long response but I realized I wasn't really adding anything new but rather just repeating what you said. 😅 Anyway I concur with your rationale of the new approach, and I think we should make the multi-diff view the focus of the work, and introduce this feature to be user-facing in the most efficient and frictionless way possible. Commit preview checks both of these boxes, and I would be happy to update the RFC to reflect this change of direction.

@annthurium

thanks for writing this up, @vanessayuenn!

I love the direction we're going in. The RFC helped me visualize how this could work, and makes me excited to collaborate with you.

## Drawbacks
- `cmd-click` to select multiple files might not be as universally known as we assume, so that might affect discoverability of this feature.

This comment has been minimized.

@annthurium

annthurium Oct 24, 2018

Contributor

I knew about cmd-click, but didn't realize Atom supported it in the staged files area. I wonder if there's a way we can educate users about this, either in product education / tutorials, or with marketing and educational materials (videos, etc.)

## Drawbacks
- `cmd-click` to select multiple files might not be as universally known as we assume, so that might affect discoverability of this feature.
- There might be performance concerns having to render many diffs at once.

This comment has been minimized.

@annthurium

annthurium Oct 24, 2018

Contributor

Hopefully @smashwilson's work on making diffs use a TextEditor will make performance a non issue. Regardless, we should find a way of measuring perf in general and for diff rendering specifically.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

It should help, depending on the way we implement!

It'd likely be worth it to track down some Giant-Ass Diffs to test this with to suss out edge cases. I'm thinking a Go project where they just took out vendored dependencies, say.

This comment has been minimized.

@simurai

simurai Oct 26, 2018

Member

Certain diffs could also be loaded only on demand. Like when the diff is very large. Or when the whole file got deleted:

image

_(note: The following is a summary of what we would like the UX to achieve, but I don't have a clear visuals of what that looks like yet.)_
- Shows diffs of multiple files as a stack.
- Each diff should show up as its own block, and the current functionality should remain independent of each block.

This comment has been minimized.

@annthurium

annthurium Oct 24, 2018

Contributor

I'm also not sure what this means. In a visual sense, do you mean each file in the diff is separated visually by a border and some margin?

- What unresolved questions do you expect to resolve through the implementation of this feature before it is released in a new version of the package?
How exactly do we construct the multi-file diffs? Do we have one TextEditor component that has different sections for each file. Or do we create a new type of pane item that contains multiple TextEditor components stacked on top of one another, one for each file diff... If we do the former we could probably get something shipped sooner (we could just get the diff of the staged changes from Git, add a special decoration for file headers, and present all the changes in one editor). But to pave the way for a more complex code review UX I think taking extra time to do the latter will serve us well. For example, I can imagine reviewers wanting to collapse some files, or mark them as "Done", in which case it would be easier if we treated each diff as its own component.

This comment has been minimized.

@annthurium

annthurium Oct 24, 2018

Contributor

I agree - it seems more robust to have each file be its own TextEditor. It also seems like editable diffs would be easier if we go this route. If we have one large TextEditor that has data from multiple files, when you save a buffer, we have to map that to which file that data lives in, which seems annoying.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

[..] If we do the former we could probably get something shipped sooner [..]

Actually, I think it'd be easier to implement the multiple-TextEditor approach:

<div className="github-MultiFilePatchView">
  <AtomTextEditor /* file one */ />
  <AtomTextEditor /* file two */ />
  <AtomTextEditor /* file three */ />
</div>

Using one <AtomTextEditor> to render all patches, with the file headers as block decorations, would involve some more complicated fiddling in the model layer to keep everything straight - even more-so for editability, as @annthurium noted above.

The tradeoff I see is that the single TextEditor approach is likely to scale better to patches with large numbers of files - by pushing complexity into the decorations within the TextEditor, we gain the performance boost associated with the editor's tiling and scrolling optimizations.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 24, 2018

@simurai @vanessayuenn glad you're on board with the commit preview direction :)

@simurai thanks for proposing UX ideas! I like the "add a 'Preview' button next to the commit button" one the best. One thing to consider is that the "Abort Merge" button shows up in the same spot when there's a merge conflict. As long as things don't get too squishy I think this could be a great place for the "Preview" button.

I do think that "Use the headers to select all files" would suffer discoverability issues and agree that "Show a preview automatically when the commit message has focus" would be jarring and unexpected.

- Same behavior as Unstaged Changes pane.
- Once there is at least one file selected, `Unstage All` button should be worded as `Unstage Selected`.
![staged changes](https://user-images.githubusercontent.com/378023/47497740-0a0b3200-d896-11e8-85af-7c644af9ca37.png)

This comment has been minimized.

@simurai

simurai Oct 25, 2018

Member

Added some mockups, but I'm not so sure about this part:

screen shot 2018-10-25 at 8 48 48 pm

The buttons to stage file and hunks look kinda weird when they are close together. Maybe we could only show them on a file when the file is collapsed.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

How does it look if the buttons were the same width? Or is that just more confusing?

This comment has been minimized.

@simurai

simurai Oct 26, 2018

Member

That would look better, but there is also Stage Selected Lines which is much wider.

image

Hmm.. maybe icons only (with tooltip + keybinding)?

image

Then when some lines get selected, show an additional Stage Selected Lines button.

image

@smashwilson

Nicely done 👍

#### Unstaged Changes pane
- User can `cmd+click` and select multiple files from the list of unstaged changes, and the pane on the left (see multi-file diff section below) will show diffs of the selected files. That pane will continue to reflect any further selecting/unselecting on the Unstaged Changes pane.
- Once there is at least one file selected, `Stage All` button should be worded as `Stage Selected`.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

I'm 👍 for leaving the "Stage All" behavior as it is, I think. At the very least we can revisit independently from this RFC to keep our scope under control 😉

- Same behavior as Unstaged Changes pane.
- Once there is at least one file selected, `Unstage All` button should be worded as `Unstage Selected`.
![staged changes](https://user-images.githubusercontent.com/378023/47497740-0a0b3200-d896-11e8-85af-7c644af9ca37.png)

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

How does it look if the buttons were the same width? Or is that just more confusing?

_(note: The following is a summary of what we would like the UX to achieve, but I don't have a clear visuals of what that looks like yet.)_
- Shows diffs of multiple files as a stack.
- Each diff should show up as its own block, and the current functionality should remain independent of each block.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

Oh interesting. So, effectively, we move the FilePatchItem's header controls as-is to the per-file banner within the multi file patch view? I like that - it'd be a lot easier than trying to figure out which file the file-specific buttons map to 😆

## Drawbacks
- `cmd-click` to select multiple files might not be as universally known as we assume, so that might affect discoverability of this feature.
- There might be performance concerns having to render many diffs at once.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

It should help, depending on the way we implement!

It'd likely be worth it to track down some Giant-Ass Diffs to test this with to suss out edge cases. I'm thinking a Go project where they just took out vendored dependencies, say.

- What unresolved questions do you expect to resolve through the implementation of this feature before it is released in a new version of the package?
How exactly do we construct the multi-file diffs? Do we have one TextEditor component that has different sections for each file. Or do we create a new type of pane item that contains multiple TextEditor components stacked on top of one another, one for each file diff... If we do the former we could probably get something shipped sooner (we could just get the diff of the staged changes from Git, add a special decoration for file headers, and present all the changes in one editor). But to pave the way for a more complex code review UX I think taking extra time to do the latter will serve us well. For example, I can imagine reviewers wanting to collapse some files, or mark them as "Done", in which case it would be easier if we treated each diff as its own component.

This comment has been minimized.

@smashwilson

smashwilson Oct 25, 2018

Member

[..] If we do the former we could probably get something shipped sooner [..]

Actually, I think it'd be easier to implement the multiple-TextEditor approach:

<div className="github-MultiFilePatchView">
  <AtomTextEditor /* file one */ />
  <AtomTextEditor /* file two */ />
  <AtomTextEditor /* file three */ />
</div>

Using one <AtomTextEditor> to render all patches, with the file headers as block decorations, would involve some more complicated fiddling in the model layer to keep everything straight - even more-so for editability, as @annthurium noted above.

The tradeoff I see is that the single TextEditor approach is likely to scale better to patches with large numbers of files - by pushing complexity into the decorations within the TextEditor, we gain the performance boost associated with the editor's tiling and scrolling optimizations.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 25, 2018

This could look like a button above the commit message box that, when clicked, opens a multi-file diff pane item called something like "Commit Preview" and shows a summary of what will go into the user's next commit based on what is currently staged.

👍 I'm also on board with this, by the way.

vanessayuenn added some commits Oct 25, 2018

@vanessayuenn

This comment has been minimized.

Contributor

vanessayuenn commented Oct 25, 2018

I made some updates to the RFC to reflect some of the discussions we have had so far, and the resulting change in direction. Tl;dr of the updates:

  • Instead of making any changes to the exisitng UX of the panels, we are adding a "Commit Preview" button to show diffs for all staged changes.
  • Collapsible diff and filterable diff are now considered out of scope.
  • Emojis.

Please let me know if I missed anything. :)

@gauravchl

This comment has been minimized.

Contributor

gauravchl commented Oct 26, 2018

Wow this is so cool!
While working on commit template, i had a similar idea of "Commit Preview" with multi diffs but for existing commits.
So i published an atom package for it: https://github.com/gauravchl/commit-book

Couldn't resist to share it here :)

screen shot 2018-10-26 at 10 59 53 am
screen shot 2018-10-26 at 11 00 04 am

simurai added some commits Oct 26, 2018

## Drawbacks
- `cmd-click` to select multiple files might not be as universally known as we assume, so that might affect discoverability of this feature.
- There might be performance concerns having to render many diffs at once.

This comment has been minimized.

@simurai

simurai Oct 26, 2018

Member

Certain diffs could also be loaded only on demand. Like when the diff is very large. Or when the whole file got deleted:

image

![commit preview](https://user-images.githubusercontent.com/378023/47555097-e6072980-d945-11e8-9c29-05624825d9f8.png)
- Shows diffs of multiple files as a stack.
- Each diff retains the file-specific controls it currently has in its header (e.g. the open file, stage file, undo last discard, etc).

This comment has been minimized.

@simurai

simurai Oct 26, 2018

Member

In the mockups I changed it a bit to:

Description Mockup
Open a file by clicking on the left of a hunk header (<> icon). It open the file, but also scrolls to the appropriate line. open file
Unstage the entire file or individual hunks by clicking on the up arrow icons. image
When selecting lines, an additional "Unstage Selected Lines" button is shown. image

For the Commit Preview, there is no discard button, since I think discarding is only possible for unstaged changes.

The above changes could easily be moved to "Out of scope" and considered at a later point. Then we can just re-use what we currently have for each file:

image

This comment has been minimized.

@kuychaco

kuychaco Oct 29, 2018

Member

I think discarding is only possible for unstaged changes.

This is true 👍

@simurai

This comment has been minimized.

Member

simurai commented Oct 26, 2018

Here a prototype to click around. Single-file and multi-file diffs have the same layout to make adding/removing files less jarring. "File blocks" just get added or removed, but otherwise the layout stays the same.

And here an alternative where the "Preview" button is next to the commit button. It also shows the avatar and commit message above the diff:

4-commit-preview-2 copy

It would be like a commit pane, just without the SHA and the committed 1 day ago. Once you click commit, they would get added. Anyways, another "out of scope" thing. 😇

@simurai

This comment has been minimized.

Member

simurai commented Oct 26, 2018

@gauravchl So i published an atom package for it: https://github.com/gauravchl/commit-book

Thanks for sharing. ❤️ The filtering is very inspirational. 👏

@simurai

Ok, added some alternatives and "out of scopes", but I like the direction 👍 and we can adapt things once we try them out.

@kuychaco

I'm loving where we've taken this RFC. Well done team! Can't wait to get started with you all

**Alternative**: It might be possible to re-use the find+replace UI to filter the multi-file diff. And maybe even have "replace" working.
#### Other out of scope UX considerations
- whether `cmd+click` to select multiple files is discoverable

This comment has been minimized.

@kuychaco

kuychaco Oct 29, 2018

Member

Is this still relevant given that we're just going with the "Commit preview" button?

![commit preview](https://user-images.githubusercontent.com/378023/47555097-e6072980-d945-11e8-9c29-05624825d9f8.png)
- Shows diffs of multiple files as a stack.
- Each diff retains the file-specific controls it currently has in its header (e.g. the open file, stage file, undo last discard, etc).

This comment has been minimized.

@kuychaco

kuychaco Oct 29, 2018

Member

I think discarding is only possible for unstaged changes.

This is true 👍

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 29, 2018

🚧 🚧 Some random thoughts about the implementation we can pursue. Names and architecture are purely illustrative and up to whomever is ultimately writing the code 😄

  • Let's have a MultiFilePatch model that includes an ordered list of FilePatch objects.
  • We'll need to extend the parser to understand multi-file git diff output. There may be some tricky work in here in relation to the deleted-symlink-and-created-file and deleted-file-and-created-symlink edge cases.
  • Add a method to our git repository plumbing to get the multi-diff from all staged changes, or, preferably, find a way to get this information using existing git repository methods.
  • Let's have a CommitPreview component hierarchy for the React side of things: CommitPreviewItem for the workspace item stuff, CommitPreviewContainer for the loading and error logic.
  • For the rest of the components, we have a decision to make. We can either: (a) generalize the existing FilePatch classes (controller and view) to be usable in both situations, or (b) create new components that are specialized for the multi-file case. I'm leaning a little toward (a) but that might change when we get down into the code.
  • Finally, we'll need to do the addition of the "preview" button within the CommitView and the corresponding wiring for the preview action in CommitController. We'll want to be sure everything looks 👌 even while the "abort merge" button is visible.

@smashwilson smashwilson changed the title from [WIP] RFC for multi-file diff to RFC for multi-file diff Oct 30, 2018

@smashwilson smashwilson referenced this pull request Oct 30, 2018

Merged

Commit Preview #1767

61 of 61 tasks complete
Update 004-multi-file-diff.md
Move sticky nav header to "out of scope"

@annthurium annthurium merged commit cc13510 into master Nov 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 84.861%
Details

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from In Progress 🔧 to Merged ☑️ Nov 20, 2018

@annthurium annthurium deleted the multi-diff-rfc branch Nov 20, 2018

@vanessayuenn vanessayuenn referenced this pull request Nov 21, 2018

Closed

v0.23.0-0 QA Review #1806

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment