opendev/gerrit
Code Issues Proposed changes

Revert "gr-diff-builder: Changing single-directional context expansion to bi-directional ('+10' button)"

Browse Source 
This reverts commit 22b5757ca5.

Reason for revert: We will wait for UX feedback to be sure.

Change-Id: I71d81bc46c47863b932ed22e98ee2392dc4cd54e
This commit is contained in:
Renan Oliveira
2019-09-19 14:38:22 +00:00
parent 22b5757ca5
commit a132faecdc
5 changed files with 77 additions and 166 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -83,16 +83,11 @@
}; };
GrDiffBuilder.ContextButtonType = { GrDiffBuilder.ContextButtonType = {
STEP: 'step', ABOVE: 'above',
BELOW: 'below',
ALL: 'all', ALL: 'all',
}; };
const ContextPlacement = {
LAST: 'last',
FIRST: 'first',
MIDDLE: 'middle',
};
const PARTIAL_CONTEXT_AMOUNT = 10; const PARTIAL_CONTEXT_AMOUNT = 10;
/** /**
@@ -236,53 +231,40 @@
group => { return group.element; }); group => { return group.element; });
}; };
/**
* @param {!Array<!Object>} contextGroups (!Array<!GrDiffGroup>)
*/
GrDiffBuilder.prototype._getContextControlPlacementFor = function(
contextGroups) {
const firstContextGroup = contextGroups[0];
const lastContextGroup = contextGroups[contextGroups.length - 1];
const firstLines = firstContextGroup.lines;
const lastLines = lastContextGroup.lines;
const firstContextInFile = firstLines.length && firstLines[0].firstInFile;
const lastContextInFile = lastLines.length &&
lastLines[lastLines.length - 1].lastInFile;
if (firstContextInFile && !lastContextInFile) {
return ContextPlacement.FIRST;
} else if (lastContextInFile && !firstContextInFile) {
return ContextPlacement.LAST;
}
return ContextPlacement.MIDDLE;
};
GrDiffBuilder.prototype._createContextControl = function(section, line) { GrDiffBuilder.prototype._createContextControl = function(section, line) {
const contextGroups = line.contextGroups; if (!line.contextGroups) return null;
if (!contextGroups) return null;
const numLines = contextGroups[contextGroups.length - 1].lineRange.left.end const numLines =
- contextGroups[0].lineRange.left.start + 1; line.contextGroups[line.contextGroups.length - 1].lineRange.left.end -
line.contextGroups[0].lineRange.left.start + 1;
if (numLines === 0) return null; if (numLines === 0) return null;
const contextPlacement = this._getContextControlPlacementFor(contextGroups);
const isStepBidirectional = (contextPlacement === ContextPlacement.MIDDLE);
const minimalForStepExpansion = isStepBidirectional ?
PARTIAL_CONTEXT_AMOUNT * 2 : PARTIAL_CONTEXT_AMOUNT;
const showStepExpansion = numLines > minimalForStepExpansion;
const td = this._createElement('td'); const td = this._createElement('td');
if (showStepExpansion) { const showPartialLinks = numLines > PARTIAL_CONTEXT_AMOUNT;
if (showPartialLinks) {
td.appendChild(this._createContextButton( td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.STEP, section, line, GrDiffBuilder.ContextButtonType.ABOVE, section, line, numLines));
numLines, contextPlacement)); td.appendChild(document.createTextNode(' - '));
} }
td.appendChild(this._createContextButton( td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.ALL, section, line, numLines)); GrDiffBuilder.ContextButtonType.ALL, section, line, numLines));
if (showPartialLinks) {
td.appendChild(document.createTextNode(' - '));
td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.BELOW, section, line, numLines));
}
return td; return td;
}; };
GrDiffBuilder.prototype._createContextButton = function(type, section, line, GrDiffBuilder.prototype._createContextButton = function(type, section, line,
numLines, contextPlacement) { numLines) {
const context = PARTIAL_CONTEXT_AMOUNT;
const button = this._createElement('gr-button', 'showContext'); const button = this._createElement('gr-button', 'showContext');
button.setAttribute('link', true); button.setAttribute('link', true);
button.setAttribute('no-uppercase', true); button.setAttribute('no-uppercase', true);
@@ -294,14 +276,14 @@
text = 'Show ' + numLines + ' common line'; text = 'Show ' + numLines + ' common line';
if (numLines > 1) { text += 's'; } if (numLines > 1) { text += 's'; }
groups.push(...line.contextGroups); groups.push(...line.contextGroups);
} else if (type === GrDiffBuilder.ContextButtonType.STEP) { } else if (type === GrDiffBuilder.ContextButtonType.ABOVE) {
const linesToShowAbove = contextPlacement === ContextPlacement.FIRST ? text = '+' + context + '↑';
0 : PARTIAL_CONTEXT_AMOUNT; groups = GrDiffGroup.hideInContextControl(line.contextGroups,
const linesToShowBelow = contextPlacement === ContextPlacement.LAST ? context, numLines);
0 : PARTIAL_CONTEXT_AMOUNT; } else if (type === GrDiffBuilder.ContextButtonType.BELOW) {
text = '+' + PARTIAL_CONTEXT_AMOUNT + ' Lines'; text = '+' + context + '↓';
groups = GrDiffGroup.hideInContextControl( groups = GrDiffGroup.hideInContextControl(line.contextGroups,
line.contextGroups, linesToShowAbove, numLines - linesToShowBelow); 0, numLines - context);
} }
Polymer.dom(button).textContent = text; Polymer.dom(button).textContent = text;

92
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
View File

@@ -89,92 +89,44 @@ limitations under the License.
assert.isTrue(node.classList.contains('classes')); assert.isTrue(node.classList.contains('classes'));
}); });
function assertContextControl(buttons, expectedButtons) { test('context control buttons', () => {
const actualButtonLabels = buttons.map( // Create 10 lines.
b => Polymer.dom(b).textContent);
assert.deepEqual(actualButtonLabels, expectedButtons);
}
function createContextControl({numLines,
firstInFile, lastInFile,
expectStepExpansion}) {
const lines = []; const lines = [];
for (let i = 0; i < numLines; i++) { for (let i = 0; i < 10; i++) {
const line = new GrDiffLine(GrDiffLine.Type.BOTH); const line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = i + 1; line.beforeNumber = i + 1;
line.afterNumber = i + 1; line.afterNumber = i + 1;
line.text = 'lorem upsum'; line.text = 'lorem upsum';
lines.push(line); lines.push(line);
} }
lines[0].firstInFile = !!firstInFile;
lines[lines.length - 1].lastInFile = !!lastInFile;
const contextLine = { const contextLine = {
contextGroups: [new GrDiffGroup(GrDiffGroup.Type.BOTH, lines)], contextGroups: [new GrDiffGroup(GrDiffGroup.Type.BOTH, lines)],
}; };
const section = {}; const section = {};
const td = builder._createContextControl(section, contextLine);
return [...td.querySelectorAll('gr-button.showContext')];
}
function getGroupsAfterExpanding(button) {
let newGroups;
button.addEventListener('tap', e => { newGroups = e.detail.groups; });
MockInteractions.tap(button);
return newGroups;
}
test('context control buttons in the middle of the file', () => {
// Does not include +10 buttons when there are fewer than 21 lines.
let buttons = createContextControl({numLines: 20});
assertContextControl(buttons, ['Show 20 common lines']);
// Includes +10 buttons when there are at least 21 lines.
buttons = createContextControl({numLines: 21});
assertContextControl(buttons, ['+10 Lines', 'Show 21 common lines']);
// When clicked with 22 Lines expands in both directions with remaining context in the middle.
buttons = createContextControl({numLines: 22});
assertContextControl(buttons, ['+10 Lines', 'Show 22 common lines']);
const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type);
assert.deepEqual(newGroupTypes,
[GrDiffLine.Type.BOTH, GrDiffLine.Type.CONTEXT_CONTROL,
GrDiffLine.Type.BOTH]);
});
test('context control buttons in the beginning of the file', () => {
// Does not include +10 buttons when there are fewer than 11 lines. // Does not include +10 buttons when there are fewer than 11 lines.
let buttons = createContextControl({numLines: 10, firstInFile: true}); let td = builder._createContextControl(section, contextLine);
assertContextControl(buttons, ['Show 10 common lines']); let buttons = td.querySelectorAll('gr-button.showContext');
// Includes +10 button when there are at least 11 lines. assert.equal(buttons.length, 1);
buttons = createContextControl({numLines: 11, firstInFile: true}); assert.equal(Polymer.dom(buttons[0]).textContent, 'Show 10 common lines');
assertContextControl(buttons, ['+10 Lines', 'Show 11 common lines']);
// When clicked with 12 Lines expands only up and remaining context in the beginning of the file. // Add another line.
buttons = createContextControl({numLines: 12, firstInFile: true}); const line = new GrDiffLine(GrDiffLine.Type.BOTH);
assertContextControl(buttons, ['+10 Lines', 'Show 12 common lines']); line.text = 'lorem upsum';
const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type); line.beforeNumber = 11;
assert.deepEqual(newGroupTypes, line.afterNumber = 11;
[GrDiffLine.Type.CONTEXT_CONTROL, GrDiffLine.Type.BOTH]); contextLine.contextGroups[0].addLine(line);
});
test('context control buttons in the end of the file', () => { // Includes +10 buttons when there are at least 11 lines.
// Does not include +10 buttons when there are fewer than 11 lines. td = builder._createContextControl(section, contextLine);
let buttons = createContextControl({numLines: 10, lastInFile: true}); buttons = td.querySelectorAll('gr-button.showContext');
assertContextControl(buttons, ['Show 10 common lines']);
// Includes +10 button when there are at least 11 lines. assert.equal(buttons.length, 3);
buttons = createContextControl({numLines: 11, lastInFile: true}); assert.equal(Polymer.dom(buttons[0]).textContent, '+10↑');
assertContextControl(buttons, ['+10 Lines', 'Show 11 common lines']); assert.equal(Polymer.dom(buttons[1]).textContent, 'Show 11 common lines');
assert.equal(Polymer.dom(buttons[2]).textContent, '+10↓');
// When clicked with 12 Lines expands only up and remaining context in the beginning of the file.
buttons = createContextControl({numLines: 12, lastInFile: true});
assertContextControl(buttons, ['+10 Lines', 'Show 12 common lines']);
// When clicked with 12 Lines expands only down and remaining context in the end of the file.
const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type);
assert.deepEqual(newGroupTypes, [GrDiffLine.Type.BOTH,
GrDiffLine.Type.CONTEXT_CONTROL]);
}); });
test('newlines 1', () => { test('newlines 1', () => {

39
polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
View File

@@ -66,6 +66,7 @@
ADDED: 'edit_b', ADDED: 'edit_b',
REMOVED: 'edit_a', REMOVED: 'edit_a',
}; };
/** /**
* The maximum size for an addition or removal chunk before it is broken down * The maximum size for an addition or removal chunk before it is broken down
* into a series of chunks that are this size at most. * into a series of chunks that are this size at most.
@@ -268,8 +269,7 @@
right: this._linesRight(chunk).length, right: this._linesRight(chunk).length,
}, },
groups: [this._chunkToGroup( groups: [this._chunkToGroup(
chunk, state.lineNums.left + 1, state.lineNums.right + 1, chunk, state.lineNums.left + 1, state.lineNums.right + 1)],
/* firstInFile */ false, /* lastInFile */ false)],
newChunkIndex: state.chunkIndex + 1, newChunkIndex: state.chunkIndex + 1,
}; };
} }
@@ -321,13 +321,10 @@
const lineCount = collapsibleChunks.reduce( const lineCount = collapsibleChunks.reduce(
(sum, chunk) => sum + this._commonChunkLength(chunk), 0); (sum, chunk) => sum + this._commonChunkLength(chunk), 0);
const firstChunk = state.chunkIndex === 0;
const lastChunk = state.chunkIndex === chunks.length - 1;
let groups = this._chunksToGroups( let groups = this._chunksToGroups(
collapsibleChunks, collapsibleChunks,
state.lineNums.left + 1, state.lineNums.left + 1,
state.lineNums.right + 1, state.lineNums.right + 1);
firstChunk, lastChunk);
if (this.context !== WHOLE_FILE) { if (this.context !== WHOLE_FILE) {
const hiddenStart = state.chunkIndex === 0 ? 0 : this.context; const hiddenStart = state.chunkIndex === 0 ? 0 : this.context;
@@ -360,17 +357,11 @@
* @param {!Array<!Object>} chunks * @param {!Array<!Object>} chunks
* @param {number} offsetLeft * @param {number} offsetLeft
* @param {number} offsetRight * @param {number} offsetRight
* @param {boolean} firstProcessed
* @param {boolean} lastProcessed
* @return {!Array<!Object>} (GrDiffGroup) * @return {!Array<!Object>} (GrDiffGroup)
*/ */
_chunksToGroups(chunks, offsetLeft, offsetRight, firstProcessed, _chunksToGroups(chunks, offsetLeft, offsetRight) {
lastProcessed) { return chunks.map(chunk => {
return chunks.map((chunk, index) => { const group = this._chunkToGroup(chunk, offsetLeft, offsetRight);
const firstInFile = firstProcessed && index === 0;
const lastInFile = lastProcessed && index === chunks.length - 1;
const group = this._chunkToGroup(chunk, offsetLeft,
offsetRight, firstInFile, lastInFile);
const chunkLength = this._commonChunkLength(chunk); const chunkLength = this._commonChunkLength(chunk);
offsetLeft += chunkLength; offsetLeft += chunkLength;
offsetRight += chunkLength; offsetRight += chunkLength;
@@ -384,10 +375,9 @@
* @param {number} offsetRight * @param {number} offsetRight
* @return {!Object} (GrDiffGroup) * @return {!Object} (GrDiffGroup)
*/ */
_chunkToGroup(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) { _chunkToGroup(chunk, offsetLeft, offsetRight) {
const type = chunk.ab ? GrDiffGroup.Type.BOTH : GrDiffGroup.Type.DELTA; const type = chunk.ab ? GrDiffGroup.Type.BOTH : GrDiffGroup.Type.DELTA;
const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight, const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight);
firstInFile, lastInFile);
const group = new GrDiffGroup(type, lines); const group = new GrDiffGroup(type, lines);
group.keyLocation = chunk.keyLocation; group.keyLocation = chunk.keyLocation;
group.dueToRebase = chunk.due_to_rebase; group.dueToRebase = chunk.due_to_rebase;
@@ -395,12 +385,12 @@
return group; return group;
}, },
_linesFromChunk(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) { _linesFromChunk(chunk, offsetLeft, offsetRight) {
let lines = [];
if (chunk.ab) { if (chunk.ab) {
lines = chunk.ab.map((row, i) => this._lineFromRow( return chunk.ab.map((row, i) => this._lineFromRow(
GrDiffLine.Type.BOTH, offsetLeft, offsetRight, row, i)); GrDiffLine.Type.BOTH, offsetLeft, offsetRight, row, i));
} else { }
let lines = [];
if (chunk.a) { if (chunk.a) {
// Avoiding a.push(...b) because that causes callstack overflows for // Avoiding a.push(...b) because that causes callstack overflows for
// large b, which can occur when large files are added removed. // large b, which can occur when large files are added removed.
@@ -415,11 +405,6 @@
GrDiffLine.Type.ADD, chunk.b, offsetRight, GrDiffLine.Type.ADD, chunk.b, offsetRight,
chunk[DiffHighlights.ADDED])); chunk[DiffHighlights.ADDED]));
} }
}
if (lines.length > 0) {
lines[0].firstInFile = firstInFile;
lines[lines.length - 1].lastInFile = lastInFile;
}
return lines; return lines;
}, },

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

@@ -44,12 +44,6 @@
this.contextGroups = null; this.contextGroups = null;
this.text = ''; this.text = '';
/** @type {boolean} */
this.firstInFile = false;
/** @type {boolean} */
this.lastInFile = false;
} }
GrDiffLine.Type = { GrDiffLine.Type = {

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

@@ -187,8 +187,6 @@ limitations under the License.
} }
.contextControl gr-button { .contextControl gr-button {
display: inline-block; display: inline-block;
margin-left: 1em;
margin-right: 1em;
text-decoration: none; text-decoration: none;
--gr-button: { --gr-button: {
color: var(--diff-context-control-color); color: var(--diff-context-control-color);