From 80df1f3f0f08fc785d2969802fb213d309fa71b0 Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Tue, 16 Apr 2019 15:55:08 +0200 Subject: [PATCH] Show but don't highlight ignored whitespace When a line had only whitespace changes that the user had configured to be ignored, the previous behavior was to not show those at all, and if the line showed up as context, render the right-side version of that line both left and right. That is very confusing. The new behavior is to not use colors to highlight those changes, but still render the left and right as the files actually are. Bug: Issue 1062 Change-Id: Ic301cc5445d33ee6e0a780178a189386669f8a7a --- .../gr-diff-builder-side-by-side.js | 3 + .../gr-diff-builder-unified.js | 3 + .../diff/gr-diff-host/gr-diff-host.js | 46 ---------- .../diff/gr-diff-host/gr-diff-host_test.html | 84 ------------------- .../elements/diff/gr-diff/gr-diff-group.js | 6 +- .../app/elements/diff/gr-diff/gr-diff.html | 13 +++ 6 files changed, 24 insertions(+), 131 deletions(-) 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 1ef278f99d..bb590ba5f4 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 @@ -35,6 +35,9 @@ if (group.dueToRebase) { sectionEl.classList.add('dueToRebase'); } + if (group.ignoredWhitespaceOnly) { + sectionEl.classList.add('ignoredWhitespaceOnly'); + } const pairs = group.getSideBySidePairs(); for (let 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 a9b1660d1f..611f8860bc 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 @@ -35,6 +35,9 @@ if (group.dueToRebase) { sectionEl.classList.add('dueToRebase'); } + if (group.ignoredWhitespaceOnly) { + sectionEl.classList.add('ignoredWhitespaceOnly'); + } for (let 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-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js index d47a61d170..507a2b9464 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js @@ -271,9 +271,6 @@ .then(diff => { this._loadedWhitespaceLevel = whitespaceLevel; this._reportDiff(diff); - if (this._getIgnoreWhitespace() !== WHITESPACE_IGNORE_NONE) { - return this._translateChunksToIgnore(diff); - } return diff; }) .catch(e => { @@ -734,49 +731,6 @@ matchers.some(matcher => matcher(threadEl))); }, - /** - * Take a diff that was loaded with a ignore-whitespace other than - * IGNORE_NONE, and convert delta chunks labeled as common into shared - * chunks. - * @param {!Object} diff - * @returns {!Object} - */ - _translateChunksToIgnore(diff) { - const newDiff = Object.assign({}, diff); - const mergedContent = []; - - // Was the last chunk visited a shared chunk? - let lastWasShared = false; - - for (const chunk of diff.content) { - if (lastWasShared && chunk.common && chunk.b) { - // The last chunk was shared and this chunk should be ignored, so - // add its revision content to the previous chunk. - mergedContent[mergedContent.length - 1].ab.push(...chunk.b); - } else if (chunk.common && !chunk.b) { - // If the chunk should be ignored, but it doesn't have revision - // content, then drop it and continue without updating lastWasShared. - continue; - } else if (lastWasShared && chunk.ab) { - // Both the last chunk and the current chunk are shared. Merge this - // chunk's shared content into the previous shared content. - mergedContent[mergedContent.length - 1].ab.push(...chunk.ab); - } else if (!lastWasShared && chunk.common && chunk.b) { - // If the previous chunk was not shared, but this one should be - // ignored, then add it as a shared chunk. - mergedContent.push({ab: chunk.b}); - } else { - // Otherwise add the chunk as is. - mergedContent.push(chunk); - } - - lastWasShared = !!mergedContent[mergedContent.length - 1].ab; - } - - newDiff.content = mergedContent; - return newDiff; - }, - _getIgnoreWhitespace() { if (!this.prefs || !this.prefs.ignore_whitespace) { return WHITESPACE_IGNORE_NONE; 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 6d4e408444..65d81bfc92 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 @@ -1257,89 +1257,5 @@ limitations under the License. assert.deepEqual(element._filterThreadElsForLocation(threadEls, line, Gerrit.DiffSide.RIGHT), [r]); }); - - suite('_translateChunksToIgnore', () => { - let content; - - setup(() => { - content = [ - {ab: ['one', 'two']}, - {a: ['three'], b: ['different three']}, - {b: ['four']}, - {ab: ['five', 'six']}, - {a: ['seven']}, - {ab: ['eight', 'nine']}, - ]; - }); - - test('does nothing to unmarked diff', () => { - assert.deepEqual(element._translateChunksToIgnore({content}), - {content}); - }); - - test('merges marked delta chunk', () => { - content[1].common = true; - assert.deepEqual(element._translateChunksToIgnore({content}), { - content: [ - {ab: ['one', 'two', 'different three']}, - {b: ['four']}, - {ab: ['five', 'six']}, - {a: ['seven']}, - {ab: ['eight', 'nine']}, - ], - }); - }); - - test('merges marked addition chunk', () => { - content[2].common = true; - assert.deepEqual(element._translateChunksToIgnore({content}), { - content: [ - {ab: ['one', 'two']}, - {a: ['three'], b: ['different three']}, - {ab: ['four', 'five', 'six']}, - {a: ['seven']}, - {ab: ['eight', 'nine']}, - ], - }); - }); - - test('merges multiple marked delta', () => { - content[1].common = true; - content[2].common = true; - assert.deepEqual(element._translateChunksToIgnore({content}), { - content: [ - {ab: ['one', 'two', 'different three', 'four', 'five', 'six']}, - {a: ['seven']}, - {ab: ['eight', 'nine']}, - ], - }); - }); - - test('marked deletion chunks are omitted', () => { - content[4].common = true; - assert.deepEqual(element._translateChunksToIgnore({content}), { - content: [ - {ab: ['one', 'two']}, - {a: ['three'], b: ['different three']}, - {b: ['four']}, - {ab: ['five', 'six', 'eight', 'nine']}, - ], - }); - }); - - test('marked deltas can start shared chunks', () => { - content[0] = {a: ['one'], b: ['two'], common: true}; - assert.deepEqual(element._translateChunksToIgnore({content}), { - content: [ - {ab: ['two']}, - {a: ['three'], b: ['different three']}, - {b: ['four']}, - {ab: ['five', 'six']}, - {a: ['seven']}, - {ab: ['eight', 'nine']}, - ], - }); - }); - }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js index 311eac6076..d7e23b3762 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js @@ -33,7 +33,11 @@ /** @type {boolean} */ this.dueToRebase = false; - /** @type {boolean} */ + /** + * True means all changes in this line are whitespace changes that should + * not be highlighted as changed as per the user settings. + * @type{boolean} + */ this.ignoredWhitespaceOnly = false; /** @type {?HTMLElement} */ 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 f78f7fd020..a6e703b99f 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html @@ -136,6 +136,8 @@ limitations under the License. .content.remove { background-color: var(--light-remove-highlight-color); } + + /* dueToRebase */ .dueToRebase .content.add .intraline, .delta.total.dueToRebase .content.add { background-color: var(--dark-rebased-add-highlight-color); @@ -150,6 +152,17 @@ limitations under the License. .dueToRebase .content.remove { background-color: var(--light-remove-add-highlight-color); } + + /* ignoredWhitespaceOnly */ + .ignoredWhitespaceOnly .content.add .intraline, + .delta.total.ignoredWhitespaceOnly .content.add, + .ignoredWhitespaceOnly .content.add, + .ignoredWhitespaceOnly .content.remove .intraline, + .delta.total.ignoredWhitespaceOnly .content.remove, + .ignoredWhitespaceOnly .content.remove { + background: none; + } + .content .contentText:empty:after { /* Newline, to ensure empty lines are one line-height tall. */ content: '\A';