From 0af80c1d90e90a87df8f75024ef937b62f405d29 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 6 Mar 2015 09:58:26 -0800 Subject: [PATCH] Further improve line grouping in unified views Summary: Ref T2009. This tweaks things a bit more to improve consecuitive groups of added and removed lines. Generally, it gives us "old, old, old, new, new, new" intead of "old, new, old, new, old, new". Feelin' real good about having unit tests for this stuff. Test Plan: Unit tests, looked at diffs in web UI. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2009 Differential Revision: https://secure.phabricator.com/D11994 --- .../data/generated.diff.one.unshielded | 2 +- .../differential/__tests__/data/groups.diff | 16 ++++++++++++++ .../__tests__/data/groups.diff.one.expect | 17 ++++++++++++++ .../__tests__/data/groups.diff.two.expect | 22 +++++++++++++++++++ .../render/DifferentialChangesetRenderer.php | 21 +++++++----------- 5 files changed, 64 insertions(+), 14 deletions(-) create mode 100644 src/applications/differential/__tests__/data/groups.diff create mode 100644 src/applications/differential/__tests__/data/groups.diff.one.expect create mode 100644 src/applications/differential/__tests__/data/groups.diff.two.expect diff --git a/src/applications/differential/__tests__/data/generated.diff.one.unshielded b/src/applications/differential/__tests__/data/generated.diff.one.unshielded index b0b99545e7..ca4b1b167e 100644 --- a/src/applications/differential/__tests__/data/generated.diff.one.unshielded +++ b/src/applications/differential/__tests__/data/generated.diff.one.unshielded @@ -1,6 +1,6 @@ N 1 . @generated\n~ O 2 - \n~ -N 2 + \n~ O 3 - This is a generated file.\n~ +N 2 + \n~ N 3 + This is a generated file{(, full of generated code)}.\n~ N 4 . \n~ diff --git a/src/applications/differential/__tests__/data/groups.diff b/src/applications/differential/__tests__/data/groups.diff new file mode 100644 index 0000000000..624e6f307d --- /dev/null +++ b/src/applications/differential/__tests__/data/groups.diff @@ -0,0 +1,16 @@ +diff --git a/style b/style +index a5fc249..18edce4 100644 +--- a/style ++++ b/style +@@ -57,8 +57,8 @@ + .dashboard-panel div.phabricator-feed-frame { + background: #fff; + margin: 0; +- border-left: 1px solid {$lightblueborder}; +- border-right: 1px solid {$lightblueborder}; +- border-bottom: 1px solid {$blueborder}; ++ border-back: 1px solid {$lightblueborder}; ++ border-front: 1px solid {$lightblueborder}; ++ border-inside: 1px solid {$blueborder}; + max-width: none; + } diff --git a/src/applications/differential/__tests__/data/groups.diff.one.expect b/src/applications/differential/__tests__/data/groups.diff.one.expect new file mode 100644 index 0000000000..b7acdd1672 --- /dev/null +++ b/src/applications/differential/__tests__/data/groups.diff.one.expect @@ -0,0 +1,17 @@ +CTYPE 2 1 (unforced) +style +style +- +X +N 57 . .dashboard-panel div.phabricator-feed-frame {\n~ +N 58 . background: #fff;\n~ +N 59 . margin: 0;\n~ +O 60 - border-{(left)}: 1px solid {$lightblueborder};\n~ +O 61 - border-{(righ)}t: 1px solid {$lightblueborder};\n~ +O 62 - border-{(bottom)}: 1px solid {$blueborder};\n~ +N 60 + border-{(back)}: 1px solid {$lightblueborder};\n~ +N 61 + border-{(fron)}t: 1px solid {$lightblueborder};\n~ +N 62 + border-{(inside)}: 1px solid {$blueborder};\n~ +N 63 . max-width: none;\n~ +N 64 . }\n~ +X diff --git a/src/applications/differential/__tests__/data/groups.diff.two.expect b/src/applications/differential/__tests__/data/groups.diff.two.expect new file mode 100644 index 0000000000..14754c0f7c --- /dev/null +++ b/src/applications/differential/__tests__/data/groups.diff.two.expect @@ -0,0 +1,22 @@ +CTYPE 2 1 (unforced) +style +style +- +X +O 57 . .dashboard-panel div.phabricator-feed-frame {\n~ +N 57 . .dashboard-panel div.phabricator-feed-frame {\n~ +O 58 . background: #fff;\n~ +N 58 . background: #fff;\n~ +O 59 . margin: 0;\n~ +N 59 . margin: 0;\n~ +O 60 - border-{(left)}: 1px solid {$lightblueborder};\n~ +N 60 + border-{(back)}: 1px solid {$lightblueborder};\n~ +O 61 - border-{(righ)}t: 1px solid {$lightblueborder};\n~ +N 61 + border-{(fron)}t: 1px solid {$lightblueborder};\n~ +O 62 - border-{(bottom)}: 1px solid {$blueborder};\n~ +N 62 + border-{(inside)}: 1px solid {$blueborder};\n~ +O 63 . max-width: none;\n~ +N 63 . max-width: none;\n~ +O 64 . }\n~ +N 64 . }\n~ +X diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index e681908aea..c19fa176aa 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -503,10 +503,6 @@ abstract class DifferentialChangesetRenderer { // Ignore it when rendering a one-up diff. continue; } - if ($new_buf) { - $out[] = $new_buf; - $new_buf = array(); - } $old_buf[] = $primitive; } else if ($type == 'new') { if ($primitive['line'] === null) { @@ -514,17 +510,16 @@ abstract class DifferentialChangesetRenderer { // old file. Ignore it when rendering a one-up diff. continue; } - if ($old_buf) { - $out[] = $old_buf; - $old_buf = array(); - } if (!$primitive['htype']) { // If this line is the same in both versions of the file, put it in // the old line buffer. This makes sure inlines on old, unchanged // lines end up in the right place. - // First, we need to flush the new line buffer if there's anything - // in it. + // First, we need to flush the line buffers if they're not empty. + if ($old_buf) { + $out[] = $old_buf; + $old_buf = array(); + } if ($new_buf) { $out[] = $new_buf; $new_buf = array(); @@ -545,15 +540,15 @@ abstract class DifferentialChangesetRenderer { if (!$primitive['right']) { $out[] = $old_buf; $out[] = array($primitive); - $out[] = $new_buf; + $old_buf = array(); } else { $out[] = $old_buf; $out[] = $new_buf; $out[] = array($primitive); + $old_buf = array(); + $new_buf = array(); } - $old_buf = array(); - $new_buf = array(); } else { throw new Exception("Unknown primitive type '{$primitive}'!"); }