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
Branch
xspace1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22067
Build 30145: Run Core Tests
Build 30144: arc lint + arc unit

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
194โ€“199

๐Ÿ‘

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.