opendev/gerrit
Code Issues Proposed changes

Show but don't highlight ignored whitespace

Browse Source 
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
This commit is contained in:
Ole Rehmsen
2019-04-16 15:55:08 +02:00
parent 60da5472d8
commit 80df1f3f0f
6 changed files with 24 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
View File

@@ -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,

3
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
View File

@@ -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]));

46
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
View File

@@ -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;

84
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
View File

@@ -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']},
],
});
});
});
});
</script>

6
polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js
View File

@@ -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} */

13
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@@ -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';