Show whether diff sides lack trailing newlines
Show a small warning at the end of a diff if either side does not have a
trailing newline. When a diff loads, find the last chunk for both sides
and examine the last line of each chunk. If the newline is missing from
either or both, show a corresponding message.
Keep track of whether the diff component is loading using a _loading
property so that the message can be hidden when loading a new diff.
Feature: Issue 9459
Change-Id: I59f59de5676af2f0352a42aa6d7dabaad5fc65b4
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 718fa17..0fb15c4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -245,6 +245,13 @@
background-position: var(--line-limit) 0;
background-repeat: repeat-y;
}
+ .newlineWarning {
+ color: var(--deemphasized-text-color);
+ text-align: center;
+ }
+ .newlineWarning.hidden {
+ display: none;
+ }
</style>
<style include="gr-syntax-theme"></style>
<div id="diffHeader" hidden$="[[_computeDiffHeaderHidden(_diffHeaderItems)]]">
@@ -284,6 +291,9 @@
</gr-diff-highlight>
</gr-diff-selection>
</div>
+ <div class$="[[_computeNewlineWarningClass(_newlineWarning, _loading)]]">
+ [[_newlineWarning]]
+ </div>
<div id="sizeWarning" class$="[[_computeWarningClass(_showWarning)]]">
<p>
Prevented render because "Whole file" is enabled and this diff is very
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 4f9c8d7..407cbf9 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -27,6 +27,9 @@
const EVENT_ZERO_REBASE = 'rebase-percent-zero';
const EVENT_NONZERO_REBASE = 'rebase-percent-nonzero';
+ const NO_NEWLINE_BASE = 'No newline at end of base file.';
+ const NO_NEWLINE_REVISION = 'No newline at end of revision file.';
+
const DiffViewMode = {
SIDE_BY_SIDE: 'SIDE_BY_SIDE',
UNIFIED: 'UNIFIED_DIFF',
@@ -120,6 +123,11 @@
*/
lineOfInterest: Object,
+ _loading: {
+ type: Boolean,
+ value: false,
+ },
+
_loggedIn: {
type: Boolean,
value: false,
@@ -169,6 +177,11 @@
type: Number,
computed: '_computeParentIndex(patchRange.*)',
},
+
+ _newlineWarning: {
+ type: String,
+ computed: '_computeNewlineWarning(_diff)',
+ },
},
behaviors: [
@@ -196,6 +209,7 @@
/** @return {!Promise} */
reload() {
+ this._loading = true;
this.cancel();
this.clearBlame();
this._safetyBypass = null;
@@ -214,6 +228,8 @@
return this._renderDiffTable();
}
return Promise.resolve();
+ }).then(() => {
+ this._loading = false;
});
},
@@ -857,5 +873,89 @@
expandAllContext() {
this._handleFullBypass();
},
+
+ /**
+ * Find the last chunk for the given side.
+ * @param {!Object} diff
+ * @param {boolean} leftSide true if checking the base of the diff,
+ * false if testing the revision.
+ * @return {Object|null} returns the chunk object or null if there was
+ * no chunk for that side.
+ */
+ _lastChunkForSide(diff, leftSide) {
+ if (!diff.content.length) { return null; }
+
+ let chunkIndex = diff.content.length;
+ let chunk;
+
+ // Walk backwards until we find a chunk for the given side.
+ do {
+ chunkIndex--;
+ chunk = diff.content[chunkIndex];
+ } while (
+ // We haven't reached the beginning.
+ chunkIndex >= 0 &&
+
+ // The chunk doesn't have both sides.
+ !chunk.ab &&
+
+ // The chunk doesn't have the given side.
+ ((leftSide && !chunk.a) || (!leftSide && !chunk.b)));
+
+ // If we reached the beginning of the diff and failed to find a chunk
+ // with the given side, return null.
+ if (chunkIndex === -1) { return null; }
+
+ return chunk;
+ },
+
+ /**
+ * Check whether the specified side of the diff has a trailing newline.
+ * @param {!Object} diff
+ * @param {boolean} leftSide true if checking the base of the diff,
+ * false if testing the revision.
+ * @return {boolean|null} Return true if the side has a trailing newline.
+ * Return false if it doesn't. Return null if not applicable (for
+ * example, if the diff has no content on the specified side).
+ */
+ _hasTrailingNewlines(diff, leftSide) {
+ const chunk = this._lastChunkForSide(diff, leftSide);
+ if (!chunk) { return null; }
+ let lines;
+ if (chunk.ab) {
+ lines = chunk.ab;
+ } else {
+ lines = leftSide ? chunk.a : chunk.b;
+ }
+ return lines[lines.length - 1] === '';
+ },
+
+ /**
+ * @param {!Object} diff
+ * @return {string|null}
+ */
+ _computeNewlineWarning(diff) {
+ const hasLeft = this._hasTrailingNewlines(diff, true);
+ const hasRight = this._hasTrailingNewlines(diff, false);
+ const messages = [];
+ if (hasLeft === false) {
+ messages.push(NO_NEWLINE_BASE);
+ }
+ if (hasRight === false) {
+ messages.push(NO_NEWLINE_REVISION);
+ }
+ if (!messages.length) { return null; }
+ return messages.join(' — ');
+ },
+
+ /**
+ * @param {string} warning
+ * @param {boolean} loading
+ * @return {string}
+ */
+ _computeNewlineWarningClass(warning, loading) {
+ if (loading || !warning) { return 'newlineWarning hidden'; }
+ return 'newlineWarning';
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 33abab4..a3acf1f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -1266,6 +1266,131 @@
assert.isUndefined(reportStub.lastCall.args[1]);
});
});
+
+ suite('trailing newlines', () => {
+ setup(() => {
+ element = fixture('basic');
+ });
+
+ suite('_lastChunkForSide', () => {
+ test('deltas', () => {
+ const diff = {content: [
+ {a: ['foo', 'bar'], b: ['baz']},
+ {ab: ['foo', 'bar', 'baz']},
+ {b: ['foo']},
+ ]};
+ assert.equal(element._lastChunkForSide(diff, false), diff.content[2]);
+ assert.equal(element._lastChunkForSide(diff, true), diff.content[1]);
+
+ diff.content.push({a: ['foo'], b: ['bar']});
+ assert.equal(element._lastChunkForSide(diff, false), diff.content[3]);
+ assert.equal(element._lastChunkForSide(diff, true), diff.content[3]);
+ });
+
+ test('addition', () => {
+ const diff = {content: [
+ {b: ['foo', 'bar', 'baz']},
+ ]};
+ assert.equal(element._lastChunkForSide(diff, false), diff.content[0]);
+ assert.isNull(element._lastChunkForSide(diff, true));
+ });
+
+ test('deletion', () => {
+ const diff = {content: [
+ {a: ['foo', 'bar', 'baz']},
+ ]};
+ assert.isNull(element._lastChunkForSide(diff, false));
+ assert.equal(element._lastChunkForSide(diff, true), diff.content[0]);
+ });
+
+ test('empty', () => {
+ const diff = {content: []};
+ assert.isNull(element._lastChunkForSide(diff, false));
+ assert.isNull(element._lastChunkForSide(diff, true));
+ });
+ });
+
+ suite('_hasTrailingNewlines', () => {
+ test('shared no trailing', () => {
+ const diff = undefined;
+ sandbox.stub(element, '_lastChunkForSide')
+ .returns({ab: ['foo', 'bar']});
+ assert.isFalse(element._hasTrailingNewlines(diff, false));
+ assert.isFalse(element._hasTrailingNewlines(diff, true));
+ });
+
+ test('delta trailing in right', () => {
+ const diff = undefined;
+ sandbox.stub(element, '_lastChunkForSide')
+ .returns({a: ['foo', 'bar'], b: ['baz', '']});
+ assert.isTrue(element._hasTrailingNewlines(diff, false));
+ assert.isFalse(element._hasTrailingNewlines(diff, true));
+ });
+
+ test('addition', () => {
+ const diff = undefined;
+ sandbox.stub(element, '_lastChunkForSide', (diff, leftSide) => {
+ if (leftSide) { return null; }
+ return {b: ['foo', '']};
+ });
+ assert.isTrue(element._hasTrailingNewlines(diff, false));
+ assert.isNull(element._hasTrailingNewlines(diff, true));
+ });
+
+ test('deletion', () => {
+ const diff = undefined;
+ sandbox.stub(element, '_lastChunkForSide', (diff, leftSide) => {
+ if (!leftSide) { return null; }
+ return {a: ['foo']};
+ });
+ assert.isNull(element._hasTrailingNewlines(diff, false));
+ assert.isFalse(element._hasTrailingNewlines(diff, true));
+ });
+ });
+
+ test('_computeNewlineWarning', () => {
+ const NO_NEWLINE_BASE = 'No newline at end of base file.';
+ const NO_NEWLINE_REVISION = 'No newline at end of revision file.';
+
+ let hasLeft;
+ let hasRight;
+ sandbox.stub(element, '_hasTrailingNewlines', (diff, left) => {
+ if (left) { return hasLeft; }
+ return hasRight;
+ });
+ const diff = undefined;
+
+ // The revision has a trailing newline, but the base doesn't.
+ hasLeft = true;
+ hasRight = false;
+ assert.equal(element._computeNewlineWarning(diff), NO_NEWLINE_REVISION);
+
+ // Trailing newlines were undetermined in the revision.
+ hasLeft = true;
+ hasRight = null;
+ assert.isNull(element._computeNewlineWarning(diff));
+
+ // Missing trailing newlines in the base.
+ hasLeft = false;
+ hasRight = null;
+ assert.equal(element._computeNewlineWarning(diff), NO_NEWLINE_BASE);
+
+ // Missing trailing newlines in both.
+ hasLeft = false;
+ hasRight = false;
+ assert.equal(element._computeNewlineWarning(diff),
+ NO_NEWLINE_BASE + ' — ' + NO_NEWLINE_REVISION);
+ });
+
+ test('_computeNewlineWarningClass', () => {
+ const hidden = 'newlineWarning hidden';
+ const shown = 'newlineWarning';
+ assert.equal(element._computeNewlineWarningClass(null, true), hidden);
+ assert.equal(element._computeNewlineWarningClass('foo', true), hidden);
+ assert.equal(element._computeNewlineWarningClass(null, false), hidden);
+ assert.equal(element._computeNewlineWarningClass('foo', false), shown);
+ });
+ });
});
a11ySuite('basic');
