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
diff --git 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
index 23c5036..e9ca947 100644
--- 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,
diff --git 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
index d2c543d..b75fafc 100644
--- 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]));
diff --git 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
index 497cae4..ffa39dd 100644
--- 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);
diff --git 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
index 80f48ff..6fcbd74 100644
--- 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 @@
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;
diff --git 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
index 2f1ded9..0591e03 100644
--- 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 @@
<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 @@
<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 @@
</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>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 21f8f32..54e9c6e 100644
--- 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 @@
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 {
