Side by side diffs

[rafeca]

Description

This is an initial implementation of side by side diffs on GitHub Desktop.

UX considerations

I've been heavily inspired on the dotcom design/UX to implement this feature, trying to mimic it behaviour as much as possible. This includes a few considerations:

• We only highlight the differences on modified lines when the number of added and deleted lines in a hunk is the same. As an example, in the next screenshot the first hunk has diff highlighting while the second one does not have it:
  
• I've tried to follow as closely as possible the colors/background/borders of dotcom for the different elements (line number boxes, empty spaces, hunk headers, etc) on light mode. Since there's no dark mode in GitHub, I had to go a bit on my own there 🙃 .

In some cases, in order to reuse already existing logic in Desktop, the behaviour is a bit different than in GitHub.com. The most visible instance of this is differences highlighting, where the logic that we use in Desktop differs a bit from the dotcom one (I think that the one in GitHub.com is slightly better, but I don't consider it a big issue to worry about for now):

Desktop:

GitHub.com:

Partial committing

In order to have a nicer interface for partial committing, I've decided to move the line numbers of the left pane to the right, this way we have both columns of line numbers together.

This is quite unorthodox, but it solves the problem of selecting multiple lines by drag/drop plus it allows us to use the spacing between the two columns as a gutter that allows selecting the whole hunk (in a kind of similar way as it's done for unified diffs).

I think I like this approach, but I would love to have a more conventional way to solve it if it exists (it's also strange to have the line numbers in the middle in one view and on the left on another). Any suggestion/idea that I haven't considered is welcome here :)

Technical considerations

The meaty part of the implementation of this feature is in the getDiffRowsFromHunk() method: this function converts a DiffHunk object (that contains the different lines of the diff for that hunk) into an array of DiffRow objects (which is a new object that represents a line that gets displayed on screen).

This conversion is done by merging groups of consecutive deleted/added lines together in the same DiffRow so they can get displayed side by side. Other types of lines are directly converted from a DiffLine object to a DiffRow object.

Ideally, I'd like to not have DiffRow objects, but I found that by creating them with a much more specific TypeScript types than DiffLine, the rendering of rows (done in <SideBySideDiffRow/> gets quite simpler. A follow-up could be to make the DiffLine objects more strict in terms of typing (no nullable fields) which would allow to remove the conversion from DiffRow to DiffLine objects.

Not using CodeMirror allows us to have more control on the rendering, which makes it much easier to handle click interactions on the lines (e.g we can pass a hunk identifier to each row so we can easily know which hunk to select when clicking on the hunk handler, instead of having to calculate that based on the line clicked).

Also, I could appreciate a performance improvement when quickly scrolling through long diffs: it's quite possible that the overhead that CodeMirror adds is superior to the overhead of React (this is more appreciable when building in prod mode, since in dev mode side by side diffs are penalized by React debugging logic):

Unified diffs (48ms spent in scroll handler):

Side By Side diffs (14ms spent in scroll handler):

Next steps

There are a few things that I haven't implemented yet:

1. Auto-scroll the screen when moving the cursor up/down while selecting changes to allow selecting lines that are not shown in the view.

A future next step that I think can be done once/if this PR is merged is to implement unified diffs re-using this same logic. I've taken a quick spike on it and the implementation is relatively straightforward (I only had to change one line in <SideBySideDiff /> to prevent creating modified rows, but <SideBySideDiffRow /> and the css needed a few more changes to adjust the layout accordingly).

With this, we could use split diffs to stress test this diff viewer that does not have CodeMirror, and once is stable enough we could enable it for unified diffs behind a feature flag to end up replacing completely the current diff viewer.

Screenshots

Release notes

Notes: [New] Side by side diffs

[niik]

• Improved the gutters to select lines/hunk for partial committing (now they're all 4px wide):

I've had to revert this change (i.e 3c32a6a). While d38716d fixed the initial misalignment there were still problems with misaligned columns in other zoom levels. I figured it's not worth holding up this entire PR for and we can take a look at it in a separate PR.

[niik]

This is sufficiently isolated and tested that I'm comfortable bringing it to development. Once #10756 has merged we'll see about releasing this to beta.

Absolutely stellar work here @rafeca.

[LeonardLaszlo]

When are we going to see this in the official release?

[niik]

When are we going to see this in the official release?

We're hoping to ship this in the next production release in early to mid November

[victor871129]

Two features that Sublime Merge has and Github Desktop does not was side-by-side diff and viewing all the files at once, modified files and staged files, there you can see all files at once so you just scroll and you don't click too much.

Awesome that you implemented side-by-side diff. Can you implement a setting to view all the modified-staged files at once?

[MarcelloTheArcane]

This is fantastic! If it's a new file, it would make sense to show the old single diff, do you think?

[JosephTLyons]

This is fantastic! If it's a new file, it would make sense to show the old single diff, do you think?

I think that makes sense. I suppose it would imply that if it were a single file delete, it would show the old single file view as well?

[MarcelloTheArcane]

@JosephTLyons Yep, I forgot deleting files too 😄

[ariooo100]

delete

[mohamaddartomi]

holle

[killmen136]

Ed