Page MenuHomePhabricator

Plans: Improve Differential diff display behaviors
Open, NormalPublic

Assigned To
None
Authored By
epriestley
Jun 22 2018, 3:11 PM
Referenced Files
F6410155: otterlygreat.gif
Apr 29 2019, 8:47 PM
F6216550: Screen Shot 2019-02-16 at 6.37.40 AM.png
Feb 16 2019, 2:43 PM
F6216531: Screen Shot 2019-02-16 at 6.26.37 AM.png
Feb 16 2019, 2:43 PM
F6216528: Screen Shot 2019-02-16 at 6.27.00 AM.png
Feb 16 2019, 2:43 PM
F6216498: Screen Shot 2019-02-16 at 6.17.09 AM.png
Feb 16 2019, 2:18 PM
F5688425: mozreview-diff.png
Jun 22 2018, 3:11 PM
F5688432: Screen_Shot_2018-06-08_at_8.09.11_AM.png
Jun 22 2018, 3:11 PM
Tokens
"Party Time" token, awarded by cspeckmim.

Description

See T2495. Although we aren't quite ready to write tabstop rules, much of this groundwork can be put in place.

See T784. This should be updated with modern plans.

See PHI756. This requests a warning when interdiffing diffs with different base commits. A related warning is when the diffs have different repositories. I'd also like to add both events (repository change, base commit change) to the diff table itself so there's a visual warning before you start interdiffing. See also PHI757 and T10500.

See PHI757 and PHI879 and PHI1169. Leaving inline comments on the right hand side of files not touched by diff B in an interdiff between A and B raises an error.

See PHI985. When we link to an inline comment, we currently scroll the browser to just above the comment. It would be better to scroll slightly above the comment, especially if the comment spans multiple lines (so you don't need to scroll up to see the context) and probably to highlight the context block.

See D20462:

  • When a blank line is surrounded above and below by lines with an indentation level change, we could reasonably show an indentation level change on the blank line to keep the left margin smooth.
  • When a significant number of consecutive lines (say, 11+) are affected only by the same indentation level change, we could show the first few, fold the middle section, then show the last few.

Via email. Inline suggestions use the same highlighting scheme that normal code changes do, attempting to strike a balance between readability and obviousness-of-diffs. This can make typo corrections (which are likely a common type of inline suggestion) difficult to spot at a glance. Instead, the red/green colors here could reasonably be higher-contrast to make the adjustments more obvious -- general readability is almost certainly less important for suggested edits.

Move / Copy

See T7878. This appears to be a bug in "moved/copied code" detection.

See PHI379. This is another bug in "moved/copied code" detection.

See PHI985. It would be nice to link the "copy/move" markers to the original block of text. This is tricky in the general case, but maybe support in the common cases is reasonable.


Resolved

See T11142. This should probably just be wontfixed since I'm not sure there's a pathway forward.

See T6791. This should probably just be wontfixed since it's obsoleted by prose diffs and not actionable?

See PHI701. This behavior is not ideal:

Screen_Shot_2018-06-08_at_8.09.11_AM.png (605×1 px, 103 KB)

A better behavior would be for line 71 should be fully bright on both sides: there is no meaningful intraline diff there.

See PHI723. Review Board handles indentation level changes like this:

mozreview-diff.png (660×1 px, 85 KB)

This is basically superior to our approach, and I'd like to implement something very similar and remove differential.whitespace-matters.

Adjacently, changes like the one in PHI212 should be visually marked somehow and cause the change to unfold, even in "Ignore All" mode (if that mode even makes sense to retain). It's possible we can simply remove "Show All" and "Ignore All" if indentation level changes are shown in "Ignore Most".

If we do retain whitespace modes, they should likely move to "View Options" per-file (see some rationale in PHI723).

Optionally, default whitespace modes could become a changeset attribute (see T784).

See PHI504; this is T5032 (now T12822): use a bunch of JS magic to make both sides of the diff independently selectable/copyable.

See PHI878, which reports a related copy/select issue in inline comments in the unified diff view.

See PHI985. When a changeset in unified mode renders a yellow "copied/moved" gutter on the right, lines shaded with green get extra green on the left in place of the gutter. This can make it look like whitespace was added to the beginning of the file. Just blanking the gutter would probably look bizarre; perhaps the "copied/moved" gutter can be shifted left inside the "line number" column instead.

Event Timeline

epriestley triaged this task as Normal priority.Jun 22 2018, 3:11 PM
epriestley created this task.

See PHI701. This behavior is not ideal:

Something (?) has fixed this (on D19482):

Screen Shot 2019-02-16 at 6.17.09 AM.png (955×1 px, 228 KB)

I know not what.

Note that T6791 has an example of a change where removing the whitespace options is going to cause a behavioral regression. Here's an example:

$ diff -u old.txt new.txt 
--- old.txt	2019-02-16 06:25:35.000000000 -0800
+++ new.txt	2019-02-16 06:25:52.000000000 -0800
@@ -1 +1,4 @@
-print "hi"
+if (print) {
+  print "hi"
+}
+

Here's the display behavior in master, today, assuming "Whitespace Mode" is "Ignore Most" or "Ignore All". This is good, and nearly what we want -- we'd just like an additional indicator that the indentation depth on line 2 (old line 1) has increased:

Screen Shot 2019-02-16 at 6.27.00 AM.png (179×969 px, 13 KB)

Here's the display behavior after the proposed changes here. This is the default output of diff -u, git diff, etc:

Screen Shot 2019-02-16 at 6.26.37 AM.png (185×971 px, 14 KB)

This is also the behavior you get in master with the "Show All" whitespace mode.

Without the context changes, the individual line renders better now:

Screen Shot 2019-02-16 at 6.37.40 AM.png (137×887 px, 11 KB)

Fixing this is messy but I think it needs to happen to throw away all the whitespace options and configuration. "Fortunately", we already have support for a lot of the pieces, although I may have removed some of them in D20185 and need to put them back.

To fix the alignment of the diff (which left-side lines correspond to which right-side lines), we:

  • Rebuild the old and new files.
  • Normalize all the lines (e.g., strip leading and trailing whitespace, and apply any other transformations which result in better alignment, e.g. collapse whitespace runs to 1 space, lowercase the line, whatever).
  • diff the normalized files.
  • Pull the alignment out of the new diff.
  • Attach the alignment information to the actual diff text.

This is already (approximately) what we did in the "Ignore All / Ignore Most" mode in order to show indentation changes in the text visually even though the alignment came from diff -bw, so it should work fine, it just means the cumulative effect of these changes won't be throwing away as much code as I'd otherwise like us to.

  • The "<<" and ">>" indicators are rendering in places where indentation has changed but the entire line is also different. This is not intended.
  • The ScopeEngine can raise an exception on diffs with partial context. We should probably just skip this feature if we're missing context.
  • The indent depth indicators are probably generally too bright / high-vis.
  • There's no color blind scheme for the indicators.
  • The indentation indicators in one-up mode aren't great (?)

Wow thank you for this. I came across a diff that mixed a lot of whitespace with minimal code change it's much easier to differentiate what happened.

otterlygreat.gif (177×315 px, 1 MB)