Page MenuHomePhabricator

Render indent depth changes more clearly
ClosedPublic

Authored by epriestley on Feb 15 2019, 5:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 12, 5:43 AM
Unknown Object (File)
Sat, Apr 6, 6:28 PM
Unknown Object (File)
Feb 12 2024, 7:15 AM
Unknown Object (File)
Feb 10 2024, 11:27 PM
Unknown Object (File)
Feb 3 2024, 9:23 PM
Unknown Object (File)
Jan 27 2024, 5:36 AM
Unknown Object (File)
Jan 26 2024, 4:49 AM
Unknown Object (File)
Jan 10 2024, 7:11 AM
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 22021
Build 30084: Run Core Tests
Build 30083: arc lint + arc unit

Unit TestsFailed

TimeTest
136 msDifferentialParseRenderTestCase::Unknown Unit Message ("")
Assertion failed, expected values to be equal (at DifferentialParseRenderTestCase.php:62): whitespace.diff.one.whitespace Expected vs Actual Output Diff --- Old Value
1 msAlmanacNamesTestCase::Unknown Unit Message ("")
30 assertions passed.
0 msAlmanacServiceTypeTestCase::Unknown Unit Message ("")
1 assertion passed.
1 msAphrontHTTPSinkTestCase::Unknown Unit Message ("")
1 assertion passed.
0 msAphrontHTTPSinkTestCase::Unknown Unit Message ("")
6 assertions passed.
View Full Test Results (1 Failed · 385 Passed)

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.