Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

RFC for multi-file diff #1753

Merged
merged 20 commits into from
Nov 20, 2018
Merged

RFC for multi-file diff #1753

Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fd6be23
RFCs are hard
vanessayuenn Oct 22, 2018
d6267cc
YOLO
vanessayuenn Oct 22, 2018
cf7b2bb
*sobbing intensifies*
vanessayuenn Oct 22, 2018
3d35e6a
wordsoup
vanessayuenn Oct 22, 2018
310b417
testing
Oct 22, 2018
2dc7620
Add drawbacks to only having single-file diffs
kuychaco Oct 23, 2018
896a95b
Add collapsable file diffs to out-of-scope section
kuychaco Oct 23, 2018
21dff76
Add unresolved question of how to construct multi-file diff
kuychaco Oct 23, 2018
75ee9c7
Add some mockups
simurai Oct 25, 2018
7b377d0
Update partially collapsed files mockup
simurai Oct 25, 2018
157c22d
update RFC & emojify
vanessayuenn Oct 25, 2018
8def255
fix link
vanessayuenn Oct 25, 2018
4fd8f17
Update filter section
simurai Oct 26, 2018
3b1759d
Add mockups for staged & unstaged panes
simurai Oct 26, 2018
38c602f
Update unstaged changes mockup
simurai Oct 26, 2018
c34242f
Update more mockups
simurai Oct 26, 2018
ac3dc04
Move "Commit preview" mockup
simurai Oct 26, 2018
fa541a4
Fix link
simurai Oct 26, 2018
b9d9419
Merge remote-tracking branch 'origin/master' into multi-diff-rfc
kuychaco Oct 29, 2018
f0869d3
Update 004-multi-file-diff.md
vanessayuenn Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions docs/rfcs/004-multi-file-diff.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# Commit Preview & Multi-file Diffs

## :tipping_hand_woman: Status

Proposed

## :memo: Summary

Give users an option to, before they make a commit, see diffs of all staged changes in one view, akin to the [`Files changed` tab in pull requests on github.com](https://github.com/atom/github/pull/1753/files).

## :checkered_flag: Motivation

So that users can view a full set of changes with more context before committing them.

Note that the multi-diff view is the MVP of this RFC, and we have identified `Commit Preview` to be the least frictional way to introduce this feature without making too many UX changes. Other planned features that will also make use of multi-diff view are:

- [commit pane item](https://github.com/atom/github/issues/1655) where it shows all changes in a single commit
- [new PR review flow](https://github.com/atom/github/blob/master/docs/rfcs/003-pull-request-review.md) that shows all changed files proposed in a PR
- (TBD) multi-select files from [unstaged](https://user-images.githubusercontent.com/378023/47553710-b60a5700-d942-11e8-8663-731b26d513c4.png) & [staged](https://user-images.githubusercontent.com/378023/47555145-0636e880-d946-11e8-85a7-f825278cc168.png) panes to view diffs

## 🤯 Explanation

#### Commit preview button
A new button added 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.

![commit preview button](https://user-images.githubusercontent.com/378023/47554979-afc9aa00-d945-11e8-9953-45925e3278b9.png)

#### Multi-file diff view

![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).
Copy link
Contributor

@simurai simurai Oct 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kuychaco kuychaco Oct 29, 2018

Choose a reason for hiding this comment

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

I think discarding is only possible for unstaged changes.

This is true 👍

- **[[out of scope]](https://github.com/atom/github/blob/multi-diff-rfc/docs/rfcs/004-multi-file-diff.md#warning-out-of-scope)** It should be easy to jump quickly to a specific file you care about, or back to the file list to get to another file. Dotcom does so by creating a `jump to` drop down.
- **[[out of scope]](https://github.com/atom/github/blob/multi-diff-rfc/docs/rfcs/004-multi-file-diff.md#warning-out-of-scope)** As user scrolls through a long list of diffs, there should be a sticky heading which remains visible showing the filename of the diff being viewed.
- **[[out of scope]](https://github.com/atom/github/blob/multi-diff-rfc/docs/rfcs/004-multi-file-diff.md#warning-out-of-scope)** Each file diff can be collapsed.

#### Workflow
This would be a nice addition to the top-to-bottom flow that currently exists in our panel:
1. View unstaged changes
2. Stage changes to be committed
3. :new: Click "Commit Preview" :new:
4. Write commit message that summarizes all changes
5. Hit commit button
6. See commit appear in recent commits list
7. Profit :tada:


## :anchor: Drawbacks

- There might be performance concerns having to render many diffs at once.
Copy link

@annthurium annthurium Oct 24, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smashwilson smashwilson Oct 25, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@simurai simurai Oct 26, 2018

Choose a reason for hiding this comment

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

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

image


## :thinking: Rationale and alternatives

An alternative would be to _not_ implement multi-file diff, as other editors like VS Code also only has per-file diff at the time of writing. However, not implementing this would imply that [the proposed new PR review flow](https://github.com/atom/github/blob/master/docs/rfcs/003-pull-request-review.md) will have to find another solution to display all changes in a PR. Additionally users would have to do a lot more clicking to view all of their changes. Imagine there was a variable rename and only 10 lines are changed, but they are each in a different file. It'd be a bit of a pain to click through to view each one. Also, if we didn't implement multi-file diffs then we couldn't show commit contents since they often include changes across multiple files.

## :question: Unresolved questions

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.
Copy link

@annthurium annthurium Oct 24, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smashwilson smashwilson Oct 25, 2018

Choose a reason for hiding this comment

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

[..] 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.



## :warning: Out of Scope

The following items are considered out of scope for this RFC, but can be addressed in the future independently of this RFC.

#### Collapsable Diff
It would be cool if each diff was collapsable. Especially for when we start using the multi-file diff for code review and the reviewers may want to hide the contents of a file once they're done addressing the changes in it. "Collapse/Expand All" capabilities would be nice as well.

All files collapsed | Some files collapsed
--- | ---
![all collapsed](https://user-images.githubusercontent.com/378023/47497741-0a0b3200-d896-11e8-90b5-4153009f80b4.png) | ![some collapsed](https://user-images.githubusercontent.com/378023/47498408-27410000-d898-11e8-8e4b-c02dafe7e35a.png)

#### File filter for diff view
"Find" input field for filtering diffs based on search term (which could be a file name, an author, a variable name, etc). When filtering, files that have no match get collapsed. This allows you to uncollapse files (and seeing their diff) without having to clear the filter. Matches get highlighted with a yellow overlay as well as a stripe on the side, similar to git-diff in the editor.

Unfiltered | Filtered
--- | ---
![without filter](https://user-images.githubusercontent.com/378023/47497740-0a0b3200-d896-11e8-85af-7c644af9ca37.png) | ![with filter](https://user-images.githubusercontent.com/378023/47540019-116e2200-d90e-11e8-8d22-d305328d55c4.png)

**Alternative**: It might be possible to re-use the find+replace UI to filter the multi-file diff. And maybe even have "replace" working.

#### Sticky navigation header
As user scrolls through a long list of diffs, there should be a sticky heading which remains visible showing the filename of the diff being viewed.

#### Other out of scope UX considerations
- whether `cmd+click` to select multiple files is discoverable
Copy link
Contributor

@kuychaco kuychaco Oct 29, 2018

Choose a reason for hiding this comment

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

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


## :construction: Implementation phases
See [checklist on PR](https://github.com/atom/github/pull/1767)