From 234616a8627334686769f1de989d286039f4d6a5 Mon Sep 17 00:00:00 2001 From: Hermann Loose Date: Tue, 11 Feb 2020 16:54:27 +0100 Subject: [PATCH] Let GrDiffHost determine whether to show newline warnings This allows greater control over presentation, e.g. when the GrDiff component is used independently it can be provided with a diff that lacks the empty line at the end to prevent an empty line from being displayed, while the warning can still be shown when necessary based on whether a terminating newline was present in the file. Change-Id: I9fa8b5b6e1ef6b1d33a7a2ba0bcfaec8041cb947 --- .../diff/gr-diff-host/gr-diff-host.html | 4 +- .../diff/gr-diff-host/gr-diff-host.js | 67 +++++++ .../diff/gr-diff-host/gr-diff-host_test.html | 97 ++++++++++ .../app/elements/diff/gr-diff/gr-diff.js | 82 ++------- .../elements/diff/gr-diff/gr-diff_test.html | 171 +++++------------- 5 files changed, 230 insertions(+), 191 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html index 7d60f134bd..2d9369f7f4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html @@ -51,7 +51,9 @@ limitations under the License. coverage-ranges="[[_coverageRanges]]" blame="[[_blame]]" layers="[[_layers]]" - diff="[[diff]]"> + diff="[[diff]]" + show-newline-warning-left="[[_showNewlineWarningLeft(diff)]]" + show-newline-warning-right="[[_showNewlineWarningRight(diff)]]"> = 0 && + + // The chunk doesn't have both sides. + !chunk.ab && + + // The chunk doesn't have the given side. + ((leftSide && (!chunk.a || !chunk.a.length)) || + (!leftSide && (!chunk.b || !chunk.b.length)))); + + // 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] === ''; + } + + _showNewlineWarningLeft(diff) { + return this._hasTrailingNewlines(diff, true) === false; + } + + _showNewlineWarningRight(diff) { + return this._hasTrailingNewlines(diff, false) === false; + } } customElements.define(GrDiffHost.is, GrDiffHost); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html index fd93cb4d28..5a42fc4b7d 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html @@ -1530,5 +1530,102 @@ limitations under the License. }); }); }); + + suite('trailing newlines', () => { + setup(() => { + }); + + 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 with a undefined', () => { + const diff = {content: [ + {b: ['foo', 'bar', 'baz']}, + ]}; + assert.equal(element._lastChunkForSide(diff, false), diff.content[0]); + assert.isNull(element._lastChunkForSide(diff, true)); + }); + + test('addition with a empty', () => { + const diff = {content: [ + {a: [], b: ['foo', 'bar', 'baz']}, + ]}; + assert.equal(element._lastChunkForSide(diff, false), diff.content[0]); + assert.isNull(element._lastChunkForSide(diff, true)); + }); + + test('deletion with b undefined', () => { + const diff = {content: [ + {a: ['foo', 'bar', 'baz']}, + ]}; + assert.isNull(element._lastChunkForSide(diff, false)); + assert.equal(element._lastChunkForSide(diff, true), diff.content[0]); + }); + + test('deletion with b empty', () => { + const diff = {content: [ + {a: ['foo', 'bar', 'baz'], b: []}, + ]}; + 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)); + }); + }); + }); }); 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 ef22188c03..8e96147817 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -249,9 +249,19 @@ parentIndex: Number, + showNewlineWarningLeft: { + type: Boolean, + value: false, + }, + showNewlineWarningRight: { + type: Boolean, + value: false, + }, + _newlineWarning: { type: String, - computed: '_computeNewlineWarning(diff)', + computed: '_computeNewlineWarning(' + + 'showNewlineWarningLeft, showNewlineWarningRight)', }, _diffLength: Number, @@ -916,76 +926,16 @@ } /** - * 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 || !chunk.a.length)) || - (!leftSide && (!chunk.b || !chunk.b.length)))); - - // 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 + * @param {!boolean} warnLeft + * @param {!boolean} warnRight * @return {string|null} */ - _computeNewlineWarning(diff) { - const hasLeft = this._hasTrailingNewlines(diff, true); - const hasRight = this._hasTrailingNewlines(diff, false); + _computeNewlineWarning(warnLeft, warnRight) { const messages = []; - if (hasLeft === false) { + if (warnLeft) { messages.push(NO_NEWLINE_BASE); } - if (hasRight === false) { + if (warnRight) { messages.push(NO_NEWLINE_REVISION); } if (!messages.length) { return null; } 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 40b15275f9..a59e89325c 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 @@ -833,137 +833,60 @@ limitations under the License. }); }); - suite('trailing newlines', () => { + suite('trailing newline warnings', () => { + const NO_NEWLINE_BASE = 'No newline at end of base file.'; + const NO_NEWLINE_REVISION = 'No newline at end of revision file.'; + + const getWarning = element => + element.shadowRoot.querySelector('.newlineWarning').textContent; + setup(() => { element = fixture('basic'); + element.showNewlineWarningLeft = false; + element.showNewlineWarningRight = false; }); - 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 with a undefined', () => { - const diff = {content: [ - {b: ['foo', 'bar', 'baz']}, - ]}; - assert.equal(element._lastChunkForSide(diff, false), diff.content[0]); - assert.isNull(element._lastChunkForSide(diff, true)); - }); - - test('addition with a empty', () => { - const diff = {content: [ - {a: [], b: ['foo', 'bar', 'baz']}, - ]}; - assert.equal(element._lastChunkForSide(diff, false), diff.content[0]); - assert.isNull(element._lastChunkForSide(diff, true)); - }); - - test('deletion with b undefined', () => { - const diff = {content: [ - {a: ['foo', 'bar', 'baz']}, - ]}; - assert.isNull(element._lastChunkForSide(diff, false)); - assert.equal(element._lastChunkForSide(diff, true), diff.content[0]); - }); - - test('deletion with b empty', () => { - const diff = {content: [ - {a: ['foo', 'bar', 'baz'], b: []}, - ]}; - 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), + test('shows combined warning if both sides set to warn', () => { + element.showNewlineWarningLeft = true; + element.showNewlineWarningRight = true; + assert.include(getWarning(element), NO_NEWLINE_BASE + ' — ' + NO_NEWLINE_REVISION); }); + suite('showNewlineWarningLeft', () => { + test('show warning if true', () => { + element.showNewlineWarningLeft = true; + assert.include(getWarning(element), NO_NEWLINE_BASE); + }); + + test('hide warning if false', () => { + element.showNewlineWarningLeft = false; + assert.notInclude(getWarning(element), NO_NEWLINE_BASE); + }); + + test('hide warning if undefined', () => { + element.showNewlineWarningLeft = undefined; + assert.notInclude(getWarning(element), NO_NEWLINE_BASE); + }); + }); + + suite('showNewlineWarningRight', () => { + test('show warning if true', () => { + element.showNewlineWarningRight = true; + assert.include(getWarning(element), NO_NEWLINE_REVISION); + }); + + test('hide warning if false', () => { + element.showNewlineWarningRight = false; + assert.notInclude(getWarning(element), NO_NEWLINE_REVISION); + }); + + test('hide warning if undefined', () => { + element.showNewlineWarningRight = undefined; + assert.notInclude(getWarning(element), NO_NEWLINE_REVISION); + }); + }); + test('_computeNewlineWarningClass', () => { const hidden = 'newlineWarning hidden'; const shown = 'newlineWarning';