Revert "gr-diff-builder: Changing single-directional context expansion to bi-directional ('+10' button)"
This reverts commit 22b5757ca55ebc57d97d92c43cb763101ae76ef4.
Reason for revert: We will wait for UX feedback to be sure.
Change-Id: I71d81bc46c47863b932ed22e98ee2392dc4cd54e
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 6962121..a019388 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -83,16 +83,11 @@
};
GrDiffBuilder.ContextButtonType = {
- STEP: 'step',
+ ABOVE: 'above',
+ BELOW: 'below',
ALL: 'all',
};
- const ContextPlacement = {
- LAST: 'last',
- FIRST: 'first',
- MIDDLE: 'middle',
- };
-
const PARTIAL_CONTEXT_AMOUNT = 10;
/**
@@ -236,53 +231,40 @@
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) {
- const contextGroups = line.contextGroups;
- if (!contextGroups) return null;
- const numLines = contextGroups[contextGroups.length - 1].lineRange.left.end
- - contextGroups[0].lineRange.left.start + 1;
+ if (!line.contextGroups) return null;
+
+ const numLines =
+ line.contextGroups[line.contextGroups.length - 1].lineRange.left.end -
+ line.contextGroups[0].lineRange.left.start + 1;
+
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');
- if (showStepExpansion) {
+ const showPartialLinks = numLines > PARTIAL_CONTEXT_AMOUNT;
+
+ if (showPartialLinks) {
td.appendChild(this._createContextButton(
- GrDiffBuilder.ContextButtonType.STEP, section, line,
- numLines, contextPlacement));
+ GrDiffBuilder.ContextButtonType.ABOVE, section, line, numLines));
+ td.appendChild(document.createTextNode(' - '));
}
+
td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.ALL, section, line, numLines));
+ if (showPartialLinks) {
+ td.appendChild(document.createTextNode(' - '));
+ td.appendChild(this._createContextButton(
+ GrDiffBuilder.ContextButtonType.BELOW, section, line, numLines));
+ }
+
return td;
};
GrDiffBuilder.prototype._createContextButton = function(type, section, line,
- numLines, contextPlacement) {
+ numLines) {
+ const context = PARTIAL_CONTEXT_AMOUNT;
+
const button = this._createElement('gr-button', 'showContext');
button.setAttribute('link', true);
button.setAttribute('no-uppercase', true);
@@ -294,14 +276,14 @@
text = 'Show ' + numLines + ' common line';
if (numLines > 1) { text += 's'; }
groups.push(...line.contextGroups);
- } else if (type === GrDiffBuilder.ContextButtonType.STEP) {
- const linesToShowAbove = contextPlacement === ContextPlacement.FIRST ?
- 0 : PARTIAL_CONTEXT_AMOUNT;
- const linesToShowBelow = contextPlacement === ContextPlacement.LAST ?
- 0 : PARTIAL_CONTEXT_AMOUNT;
- text = '+' + PARTIAL_CONTEXT_AMOUNT + ' Lines';
- groups = GrDiffGroup.hideInContextControl(
- line.contextGroups, linesToShowAbove, numLines - linesToShowBelow);
+ } else if (type === GrDiffBuilder.ContextButtonType.ABOVE) {
+ text = '+' + context + '↑';
+ groups = GrDiffGroup.hideInContextControl(line.contextGroups,
+ context, numLines);
+ } else if (type === GrDiffBuilder.ContextButtonType.BELOW) {
+ text = '+' + context + '↓';
+ groups = GrDiffGroup.hideInContextControl(line.contextGroups,
+ 0, numLines - context);
}
Polymer.dom(button).textContent = text;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 49fdc06..b917845 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -89,92 +89,44 @@
assert.isTrue(node.classList.contains('classes'));
});
- function assertContextControl(buttons, expectedButtons) {
- const actualButtonLabels = buttons.map(
- b => Polymer.dom(b).textContent);
- assert.deepEqual(actualButtonLabels, expectedButtons);
- }
-
- function createContextControl({numLines,
- firstInFile, lastInFile,
- expectStepExpansion}) {
+ test('context control buttons', () => {
+ // Create 10 lines.
const lines = [];
- for (let i = 0; i < numLines; i++) {
+ for (let i = 0; i < 10; i++) {
const line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = i + 1;
line.afterNumber = i + 1;
line.text = 'lorem upsum';
lines.push(line);
}
- lines[0].firstInFile = !!firstInFile;
- lines[lines.length - 1].lastInFile = !!lastInFile;
+
const contextLine = {
contextGroups: [new GrDiffGroup(GrDiffGroup.Type.BOTH, lines)],
};
+
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.
- let buttons = createContextControl({numLines: 10, firstInFile: true});
- assertContextControl(buttons, ['Show 10 common lines']);
+ let td = builder._createContextControl(section, contextLine);
+ let buttons = td.querySelectorAll('gr-button.showContext');
- // Includes +10 button when there are at least 11 lines.
- buttons = createContextControl({numLines: 11, firstInFile: true});
- assertContextControl(buttons, ['+10 Lines', 'Show 11 common lines']);
+ assert.equal(buttons.length, 1);
+ assert.equal(Polymer.dom(buttons[0]).textContent, 'Show 10 common lines');
- // When clicked with 12 Lines expands only up and remaining context in the beginning of the file.
- buttons = createContextControl({numLines: 12, firstInFile: true});
- assertContextControl(buttons, ['+10 Lines', 'Show 12 common lines']);
- const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type);
- assert.deepEqual(newGroupTypes,
- [GrDiffLine.Type.CONTEXT_CONTROL, GrDiffLine.Type.BOTH]);
- });
+ // Add another line.
+ const line = new GrDiffLine(GrDiffLine.Type.BOTH);
+ line.text = 'lorem upsum';
+ line.beforeNumber = 11;
+ line.afterNumber = 11;
+ contextLine.contextGroups[0].addLine(line);
- test('context control buttons in the end of the file', () => {
- // Does not include +10 buttons when there are fewer than 11 lines.
- let buttons = createContextControl({numLines: 10, lastInFile: true});
- assertContextControl(buttons, ['Show 10 common lines']);
+ // Includes +10 buttons when there are at least 11 lines.
+ td = builder._createContextControl(section, contextLine);
+ buttons = td.querySelectorAll('gr-button.showContext');
- // Includes +10 button when there are at least 11 lines.
- buttons = createContextControl({numLines: 11, lastInFile: true});
- 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.
- 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]);
+ assert.equal(buttons.length, 3);
+ assert.equal(Polymer.dom(buttons[0]).textContent, '+10↑');
+ assert.equal(Polymer.dom(buttons[1]).textContent, 'Show 11 common lines');
+ assert.equal(Polymer.dom(buttons[2]).textContent, '+10↓');
});
test('newlines 1', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
index d1c58b2..d4b4e2b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
@@ -66,6 +66,7 @@
ADDED: 'edit_b',
REMOVED: 'edit_a',
};
+
/**
* 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.
@@ -268,8 +269,7 @@
right: this._linesRight(chunk).length,
},
groups: [this._chunkToGroup(
- chunk, state.lineNums.left + 1, state.lineNums.right + 1,
- /* firstInFile */ false, /* lastInFile */ false)],
+ chunk, state.lineNums.left + 1, state.lineNums.right + 1)],
newChunkIndex: state.chunkIndex + 1,
};
}
@@ -321,13 +321,10 @@
const lineCount = collapsibleChunks.reduce(
(sum, chunk) => sum + this._commonChunkLength(chunk), 0);
- const firstChunk = state.chunkIndex === 0;
- const lastChunk = state.chunkIndex === chunks.length - 1;
let groups = this._chunksToGroups(
collapsibleChunks,
state.lineNums.left + 1,
- state.lineNums.right + 1,
- firstChunk, lastChunk);
+ state.lineNums.right + 1);
if (this.context !== WHOLE_FILE) {
const hiddenStart = state.chunkIndex === 0 ? 0 : this.context;
@@ -360,17 +357,11 @@
* @param {!Array<!Object>} chunks
* @param {number} offsetLeft
* @param {number} offsetRight
- * @param {boolean} firstProcessed
- * @param {boolean} lastProcessed
* @return {!Array<!Object>} (GrDiffGroup)
*/
- _chunksToGroups(chunks, offsetLeft, offsetRight, firstProcessed,
- lastProcessed) {
- return chunks.map((chunk, index) => {
- const firstInFile = firstProcessed && index === 0;
- const lastInFile = lastProcessed && index === chunks.length - 1;
- const group = this._chunkToGroup(chunk, offsetLeft,
- offsetRight, firstInFile, lastInFile);
+ _chunksToGroups(chunks, offsetLeft, offsetRight) {
+ return chunks.map(chunk => {
+ const group = this._chunkToGroup(chunk, offsetLeft, offsetRight);
const chunkLength = this._commonChunkLength(chunk);
offsetLeft += chunkLength;
offsetRight += chunkLength;
@@ -384,10 +375,9 @@
* @param {number} offsetRight
* @return {!Object} (GrDiffGroup)
*/
- _chunkToGroup(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) {
+ _chunkToGroup(chunk, offsetLeft, offsetRight) {
const type = chunk.ab ? GrDiffGroup.Type.BOTH : GrDiffGroup.Type.DELTA;
- const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight,
- firstInFile, lastInFile);
+ const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight);
const group = new GrDiffGroup(type, lines);
group.keyLocation = chunk.keyLocation;
group.dueToRebase = chunk.due_to_rebase;
@@ -395,30 +385,25 @@
return group;
},
- _linesFromChunk(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) {
- let lines = [];
+ _linesFromChunk(chunk, offsetLeft, offsetRight) {
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));
- } else {
- if (chunk.a) {
- // Avoiding a.push(...b) because that causes callstack overflows for
- // large b, which can occur when large files are added removed.
- lines = lines.concat(this._linesFromRows(
- GrDiffLine.Type.REMOVE, chunk.a, offsetLeft,
- chunk[DiffHighlights.REMOVED]));
- }
- if (chunk.b) {
- // Avoiding a.push(...b) because that causes callstack overflows for
- // large b, which can occur when large files are added removed.
- lines = lines.concat(this._linesFromRows(
- GrDiffLine.Type.ADD, chunk.b, offsetRight,
- chunk[DiffHighlights.ADDED]));
- }
}
- if (lines.length > 0) {
- lines[0].firstInFile = firstInFile;
- lines[lines.length - 1].lastInFile = lastInFile;
+ let lines = [];
+ if (chunk.a) {
+ // Avoiding a.push(...b) because that causes callstack overflows for
+ // large b, which can occur when large files are added removed.
+ lines = lines.concat(this._linesFromRows(
+ GrDiffLine.Type.REMOVE, chunk.a, offsetLeft,
+ chunk[DiffHighlights.REMOVED]));
+ }
+ if (chunk.b) {
+ // Avoiding a.push(...b) because that causes callstack overflows for
+ // large b, which can occur when large files are added removed.
+ lines = lines.concat(this._linesFromRows(
+ GrDiffLine.Type.ADD, chunk.b, offsetRight,
+ chunk[DiffHighlights.ADDED]));
}
return lines;
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-line.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-line.js
index 49d583f..b64385d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-line.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-line.js
@@ -44,12 +44,6 @@
this.contextGroups = null;
this.text = '';
-
- /** @type {boolean} */
- this.firstInFile = false;
-
- /** @type {boolean} */
- this.lastInFile = false;
}
GrDiffLine.Type = {
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 a9755ce..bc8af9d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -187,8 +187,6 @@
}
.contextControl gr-button {
display: inline-block;
- margin-left: 1em;
- margin-right: 1em;
text-decoration: none;
--gr-button: {
color: var(--diff-context-control-color);
