Page MenuHomePhabricator

Plans: Improve Differential diff display behaviors
Open, NormalPublic

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.

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:

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:

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.
epriestley updated the task description. (Show Details)Jul 13 2018, 6:58 PM
epriestley updated the task description. (Show Details)Jul 13 2018, 7:10 PM
epriestley updated the task description. (Show Details)Aug 27 2018, 5:23 PM
epriestley updated the task description. (Show Details)Sep 14 2018, 3:59 PM
epriestley updated the task description. (Show Details)Sep 14 2018, 7:07 PM
epriestley updated the task description. (Show Details)Nov 24 2018, 3:11 PM
epriestley updated the task description. (Show Details)Nov 24 2018, 3:17 PM
epriestley updated the task description. (Show Details)Nov 26 2018, 1:01 PM

See PHI701. This behavior is not ideal:

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

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:

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

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:

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.

epriestley updated the task description. (Show Details)Feb 16 2019, 4:42 PM
epriestley added a comment.EditedFeb 19 2019, 10:42 PM
  • 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 (?)
epriestley updated the task description. (Show Details)Feb 20 2019, 5:04 AM
epriestley updated the task description. (Show Details)Feb 20 2019, 1:15 PM
epriestley updated the task description. (Show Details)Mar 28 2019, 3:09 PM
epriestley updated the task description. (Show Details)Apr 24 2019, 2:02 AM

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.

glob added a subscriber: glob.Mon, Oct 28, 2:53 PM