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 69621215e0..a01938846b 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} contextGroups (!Array) - */ - 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 49fdc06421..b917845d44 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 @@ limitations under the License. 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 d1c58b27d3..d4b4e2bfd9 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} chunks * @param {number} offsetLeft * @param {number} offsetRight - * @param {boolean} firstProcessed - * @param {boolean} lastProcessed * @return {!Array} (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 49d583f000..b64385d7d6 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 a9755ceb27..bc8af9d8bd 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 @@ limitations under the License. } .contextControl gr-button { display: inline-block; - margin-left: 1em; - margin-right: 1em; text-decoration: none; --gr-button: { color: var(--diff-context-control-color);