Page MenuHomePhabricator

Render indent depth changes more clearly
ClosedPublic

Authored by epriestley on Feb 15 2019, 5:39 PM.
Tags
None
Referenced Files
F14406999: D20181.id48212.diff
Tue, Dec 24, 12:38 AM
Unknown Object (File)
Thu, Dec 19, 6:36 PM
Unknown Object (File)
Tue, Dec 17, 11:03 AM
Unknown Object (File)
Sun, Dec 15, 6:02 PM
Unknown Object (File)
Sat, Dec 14, 6:49 PM
Unknown Object (File)
Tue, Dec 10, 11:47 PM
Unknown Object (File)
Sun, Dec 8, 9:09 AM
Unknown Object (File)
Fri, Dec 6, 12:48 PM
Subscribers
None

Details

Summary

Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like diff -bw, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use diff -bw.

However, this approach is very coarse and has a lot of problems, like detecting "ab" -> "a b" as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.

We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to diff -bw it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.

I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.

Move toward detecting and rendering indent depth changes. Followup work:

  • These should get colorblind colors and the design can probably use a little more tweaking.
  • The OneUp mode is okay, but could be improved.
  • Whitespace mode can now be removed completely.
  • I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.
Test Plan

Screen Shot 2019-02-15 at 9.21.23 AM.png (640×1 px, 165 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 15 2019, 5:41 PM
Harbormaster failed remote builds in B22021: Diff 48176!

Unit test failure is because this needs a small support change in arcanist/, see next diff.

amckinley added inline comments.
src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php
200–205

👍

This revision is now accepted and ready to land.Feb 19 2019, 7:52 PM

The "whitespace added" side of your screenshot looks a little weird. It would be nice to give the chevron the same amount of green background padding on both sides.

  • Fix a bug with the copied code column; could cause concerns with ">>" alignment.
This revision was automatically updated to reflect the committed changes.