gr-diff-builder: Changing single-directional context expansion to bi-directional ('+10' button)
Before/After screenshots: https://imgur.com/a/Y9yIZcB
Change-Id: I41c5a21474a78c24da0f04259dd1a0508dc73e15
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 a019388..6962121 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,11 +83,16 @@
};
GrDiffBuilder.ContextButtonType = {
- ABOVE: 'above',
- BELOW: 'below',
+ STEP: 'step',
ALL: 'all',
};
+ const ContextPlacement = {
+ LAST: 'last',
+ FIRST: 'first',
+ MIDDLE: 'middle',
+ };
+
const PARTIAL_CONTEXT_AMOUNT = 10;
/**
@@ -231,40 +236,53 @@
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) {
- if (!line.contextGroups) return null;
-
- const numLines =
- line.contextGroups[line.contextGroups.length - 1].lineRange.left.end -
- line.contextGroups[0].lineRange.left.start + 1;
-
+ const contextGroups = line.contextGroups;
+ if (!contextGroups) return null;
+ const numLines = contextGroups[contextGroups.length - 1].lineRange.left.end
+ - 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');
- const showPartialLinks = numLines > PARTIAL_CONTEXT_AMOUNT;
-
- if (showPartialLinks) {
+ if (showStepExpansion) {
td.appendChild(this._createContextButton(
- GrDiffBuilder.ContextButtonType.ABOVE, section, line, numLines));
- td.appendChild(document.createTextNode(' - '));
+ GrDiffBuilder.ContextButtonType.STEP, section, line,
+ numLines, contextPlacement));
}
-
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) {
- const context = PARTIAL_CONTEXT_AMOUNT;
-
+ numLines, contextPlacement) {
const button = this._createElement('gr-button', 'showContext');
button.setAttribute('link', true);
button.setAttribute('no-uppercase', true);
@@ -276,14 +294,14 @@
text = 'Show ' + numLines + ' common line';
if (numLines > 1) { text += 's'; }
groups.push(...line.contextGroups);
- } 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);
+ } 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);
}
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 b917845..49fdc06 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,44 +89,92 @@
assert.isTrue(node.classList.contains('classes'));
});
- test('context control buttons', () => {
- // Create 10 lines.
+ function assertContextControl(buttons, expectedButtons) {
+ const actualButtonLabels = buttons.map(
+ b => Polymer.dom(b).textContent);
+ assert.deepEqual(actualButtonLabels, expectedButtons);
+ }
+
+ function createContextControl({numLines,
+ firstInFile, lastInFile,
+ expectStepExpansion}) {
const lines = [];
- for (let i = 0; i < 10; i++) {
+ for (let i = 0; i < numLines; 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 td = builder._createContextControl(section, contextLine);
- let buttons = td.querySelectorAll('gr-button.showContext');
+ let buttons = createContextControl({numLines: 10, firstInFile: true});
+ assertContextControl(buttons, ['Show 10 common lines']);
- assert.equal(buttons.length, 1);
- assert.equal(Polymer.dom(buttons[0]).textContent, 'Show 10 common lines');
+ // Includes +10 button when there are at least 11 lines.
+ buttons = createContextControl({numLines: 11, firstInFile: true});
+ assertContextControl(buttons, ['+10 Lines', 'Show 11 common lines']);
- // 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);
+ // 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]);
+ });
- // Includes +10 buttons when there are at least 11 lines.
- td = builder._createContextControl(section, contextLine);
- buttons = td.querySelectorAll('gr-button.showContext');
+ 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']);
- 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↓');
+ // 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]);
});
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 d4b4e2b..d1c58b2 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,7 +66,6 @@
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.
@@ -269,7 +268,8 @@
right: this._linesRight(chunk).length,
},
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,
};
}
@@ -321,10 +321,13 @@
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);
+ state.lineNums.right + 1,
+ firstChunk, lastChunk);
if (this.context !== WHOLE_FILE) {
const hiddenStart = state.chunkIndex === 0 ? 0 : this.context;
@@ -357,11 +360,17 @@
* @param {!Array<!Object>} chunks
* @param {number} offsetLeft
* @param {number} offsetRight
+ * @param {boolean} firstProcessed
+ * @param {boolean} lastProcessed
* @return {!Array<!Object>} (GrDiffGroup)
*/
- _chunksToGroups(chunks, offsetLeft, offsetRight) {
- return chunks.map(chunk => {
- const group = this._chunkToGroup(chunk, offsetLeft, offsetRight);
+ _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);
const chunkLength = this._commonChunkLength(chunk);
offsetLeft += chunkLength;
offsetRight += chunkLength;
@@ -375,9 +384,10 @@
* @param {number} offsetRight
* @return {!Object} (GrDiffGroup)
*/
- _chunkToGroup(chunk, offsetLeft, offsetRight) {
+ _chunkToGroup(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) {
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);
group.keyLocation = chunk.keyLocation;
group.dueToRebase = chunk.due_to_rebase;
@@ -385,25 +395,30 @@
return group;
},
- _linesFromChunk(chunk, offsetLeft, offsetRight) {
- if (chunk.ab) {
- return chunk.ab.map((row, i) => this._lineFromRow(
- GrDiffLine.Type.BOTH, offsetLeft, offsetRight, row, i));
- }
+ _linesFromChunk(chunk, offsetLeft, offsetRight, firstInFile, 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.ab) {
+ lines = 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 (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;
}
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 b64385d..49d583f 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,6 +44,12 @@
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 bc8af9d..a9755ce 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -187,6 +187,8 @@
}
.contextControl gr-button {
display: inline-block;
+ margin-left: 1em;
+ margin-right: 1em;
text-decoration: none;
--gr-button: {
color: var(--diff-context-control-color);
