Page MenuHomePhabricator

Render indent depth changes more clearly
ClosedPublic

Authored by epriestley on Feb 15 2019, 5:39 PM.

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

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 15 2019, 5:39 PM
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!
epriestley requested review of this revision.Feb 15 2019, 5:53 PM

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

epriestley edited the summary of this revision. (Show Details)Feb 15 2019, 5:57 PM
amckinley accepted this revision.Feb 19 2019, 7:52 PM
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.

epriestley updated this revision to Diff 48211.Feb 19 2019, 8:36 PM
  • Fix a bug with the copied code column; could cause concerns with ">>" alignment.
This revision was automatically updated to reflect the committed changes.