Highlight add/remove diff lines more consistently

Diffs in PolyGerrit apply two shades of highlight to changed lines (light and dark) to indicate the granularity of modifications and to distinguish intraline edits. However, the logic for choosing the background shade for diff lines would differ from that of GWT UI diffs subtly. +----------------------------------+----------------------------------+ | GWT UI Shading Logic | PG Shading Logic (incorrect) | +----------------------------------+----------------------------------+ | Diff lines get a dark background | Diff lines get a dark background | | IFF they appear in a delta chunk | IFF they do NOT contain any | | that is empty on the left OR | intraline differences. | | empty on the right. | | +----------------------------------+----------------------------------+ | Diff lines get a light background otherwise. | +---------------------------------------------------------------------+ With this change, the shading logic in PolyGerrit is modified to match that of the GWT UI. Bug: Issue 4219 Bug: Issue 5117 Change-Id: Ice24292df777118c08c3e73f771720f8a186a183
2016-12-21 12:55:21 -08:00
parent e9070d639d
commit 1e3cd47707
6 changed files with 64 additions and 18 deletions
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
@@ -26,6 +26,9 @@
  GrDiffBuilderSideBySide.prototype.buildSectionElement = function(group) {
    var sectionEl = this._createElement('tbody', 'section');
    sectionEl.classList.add(group.type);
+    if (this._isTotal(group)) {
+      sectionEl.classList.add('total');
+    }
    var pairs = group.getSideBySidePairs();
    for (var i = 0; i < pairs.length; i++) {
      sectionEl.appendChild(this._createRow(sectionEl, pairs[i].left,
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
@@ -26,6 +26,9 @@
  GrDiffBuilderUnified.prototype.buildSectionElement = function(group) {
    var sectionEl = this._createElement('tbody', 'section');
    sectionEl.classList.add(group.type);
+    if (this._isTotal(group)) {
+      sectionEl.classList.add('total');
+    }

    for (var i = 0; i < group.lines.length; ++i) {
      sectionEl.appendChild(this._createRow(sectionEl, group.lines[i]));
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -408,9 +408,6 @@
      contentText.innerHTML = html;
    }

-    td.classList.add(line.highlights.length > 0 ?
-        'lightHighlight' : 'darkHighlight');
-
    this.layers.forEach(function(layer) {
      layer.annotate(contentText, line);
    });
@@ -568,5 +565,17 @@
    throw Error('Subclasses must implement _getNextContentOnSide');
  };

+  /**
+   * Determines whether the given group is either totally an addition or totally
+   * a removal.
+   * @param {GrDiffGroup} group
+   * @return {Boolean}
+   */
+  GrDiffBuilder.prototype._isTotal = function(group) {
+    return group.type === GrDiffGroup.Type.DELTA &&
+        (!group.adds.length || !group.removes.length) &&
+        !(!group.adds.length && !group.removes.length);
+  }
+
  window.GrDiffBuilder = GrDiffBuilder;
 })(window, GrDiffGroup, GrDiffLine);
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -280,6 +280,37 @@ limitations under the License.
      checkThreadProps(threadEl, '3', 'REVISION', [l3, r5]);
    });

+    suite('_isTotal', function() {
+      test('is total for add', function() {
+        var group = new GrDiffGroup(GrDiffGroup.Type.DELTA);
+        for (var idx = 0; idx < 10; idx++) {
+          group.addLine(new GrDiffLine(GrDiffLine.Type.ADD));
+        }
+        assert.isTrue(GrDiffBuilder.prototype._isTotal(group));
+      });
+
+      test('is total for remove', function() {
+        var group = new GrDiffGroup(GrDiffGroup.Type.DELTA);
+        for (var idx = 0; idx < 10; idx++) {
+          group.addLine(new GrDiffLine(GrDiffLine.Type.REMOVE));
+        }
+        assert.isTrue(GrDiffBuilder.prototype._isTotal(group));
+      });
+
+      test('not total for empty', function() {
+        var group = new GrDiffGroup(GrDiffGroup.Type.BOTH);
+        assert.isFalse(GrDiffBuilder.prototype._isTotal(group));
+      });
+
+      test('not total for non-delta', function() {
+        var group = new GrDiffGroup(GrDiffGroup.Type.DELTA);
+        for (var idx = 0; idx < 10; idx++) {
+          group.addLine(new GrDiffLine(GrDiffLine.Type.BOTH));
+        }
+        assert.isFalse(GrDiffBuilder.prototype._isTotal(group));
+      });
+    });
+
    suite('intraline differences', function() {
      var el;
      var str;
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
@@ -32,9 +32,9 @@ limitations under the License.
        <tbody class="section both">
          <tr class="diff-row side-by-side" left-type="both" right-type="both">
            <td class="left lineNum" data-value="138">138</td>
-            <td class="content both darkHighlight"><div class="contentText">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
+            <td class="content both"><div class="contentText">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
            <td class="right lineNum" data-value="119">119</td>
-            <td class="content both darkHighlight"><div class="contentText">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
+            <td class="content both"><div class="contentText">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
          </tr>
        </tbody>

@@ -42,21 +42,21 @@ limitations under the License.
          <tr class="diff-row side-by-side" left-type="remove" right-type="add">
            <td class="left lineNum" data-value="140">140</td>
            <!-- Next tag is formatted to eliminate zero-length text nodes. -->
-            <td class="content remove lightHighlight"><div class="contentText">na💢ti <hl class="foo">te, inquit</hl>, sumus <hl class="bar">aliquando</hl> otiosum, <hl>certe</hl> a <hl><span class="tab withIndicator" style="tab-size:8;"></span></hl>udiam, <hl>quid</hl> sit, <span class="tab withIndicator" style="tab-size:8;"></span>quod <hl>Epicurum</hl></div><gr-diff-comment-thread>
+            <td class="content remove"><div class="contentText">na💢ti <hl class="foo">te, inquit</hl>, sumus <hl class="bar">aliquando</hl> otiosum, <hl>certe</hl> a <hl><span class="tab withIndicator" style="tab-size:8;"></span></hl>udiam, <hl>quid</hl> sit, <span class="tab withIndicator" style="tab-size:8;"></span>quod <hl>Epicurum</hl></div><gr-diff-comment-thread>
                [Yet another random diff thread content here]
              </gr-diff-comment-thread></td>
            <td class="right lineNum" data-value="120">120</td>
            <!-- Next tag is formatted to eliminate zero-length text nodes. -->
-            <td class="content add lightHighlight"><div class="contentText">nacti , <hl>,</hl> sumus <hl><span class="tab withIndicator" style="tab-size:8;"></span></hl> otiosum,  <span class="tab withIndicator" style="tab-size:8;"></span> audiam,  sit, quod</div></td>
+            <td class="content add"><div class="contentText">nacti , <hl>,</hl> sumus <hl><span class="tab withIndicator" style="tab-size:8;"></span></hl> otiosum,  <span class="tab withIndicator" style="tab-size:8;"></span> audiam,  sit, quod</div></td>
          </tr>
        </tbody>

        <tbody class="section both">
          <tr class="diff-row side-by-side" left-type="both" right-type="both">
            <td class="left lineNum" data-value="141"></td>
-            <td class="content both darkHighlight"><div class="contentText">nam et<hl><span class="tab withIndicator" style="tab-size:8;">	</span></hl>complectitur<span class="tab withIndicator" style="tab-size:8;">	</span>verbis, quod vult, et dicit plane, quod intellegam;</div></td>
+            <td class="content both"><div class="contentText">nam et<hl><span class="tab withIndicator" style="tab-size:8;">	</span></hl>complectitur<span class="tab withIndicator" style="tab-size:8;">	</span>verbis, quod vult, et dicit plane, quod intellegam;</div></td>
            <td class="right lineNum" data-value="130"></td>
-            <td class="content both darkHighlight"><div class="contentText">nam et complectitur verbis, quod vult, et dicit plane, quod intellegam;</div></td>
+            <td class="content both"><div class="contentText">nam et complectitur verbis, quod vult, et dicit plane, quod intellegam;</div></td>
          </tr>
        </tbody>

@@ -81,21 +81,21 @@ limitations under the License.
          </tr>
        </tbody>

-        <tbody class="section delta">
+        <tbody class="section delta total">
          <tr class="diff-row side-by-side" left-type="blank" right-type="add">
            <td class="left"></td>
-            <td class="blank darkHighlight"></td>
+            <td class="blank"></td>
            <td class="right lineNum" data-value="146"></td>
-            <td class="content add darkHighlight"><div class="contentText">[17] Quid igitur est? inquit; audire enim cupio, quid non probes. Principio, inquam,</div></td>
+            <td class="content add"><div class="contentText">[17] Quid igitur est? inquit; audire enim cupio, quid non probes. Principio, inquam,</div></td>
          </tr>
        </tbody>

        <tbody class="section both">
          <tr class="diff-row side-by-side" left-type="both" right-type="both">
            <td class="left lineNum" data-value="165"></td>
-            <td class="content both darkHighlight"><div class="contentText">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
+            <td class="content both"><div class="contentText">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
            <td class="right lineNum" data-value="147"></td>
-            <td class="content both darkHighlight"><div class="contentText">in physicis, <hl><span class="tab withIndicator" style="tab-size:8;">	</span></hl> quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
+            <td class="content both"><div class="contentText">in physicis, <hl><span class="tab withIndicator" style="tab-size:8;">	</span></hl> quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
          </tr>
        </tbody>

--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -125,17 +125,17 @@ limitations under the License.
        width: var(--content-width, 80ch);
      }
      .content.add .intraline,
-      .content.add.darkHighlight {
+      .delta.total .content.add {
        background-color: var(--dark-add-highlight-color);
      }
-      .content.add.lightHighlight {
+      .content.add {
        background-color: var(--light-add-highlight-color);
      }
      .content.remove .intraline,
-      .content.remove.darkHighlight {
+      .delta.total .content.remove {
        background-color: var(--dark-remove-highlight-color);
      }
-      .content.remove.lightHighlight {
+      .content.remove {
        background-color: var(--light-remove-highlight-color);
      }
      .content .contentText:after {