Add support to skip chunks Change-Id: I4769eb25e9d990328e661f48230e2719d1515a0a
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts index d1f52ae..91fd137 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
@@ -18,6 +18,7 @@ import {GrDiffLine, GrDiffLineType, LineNumber} from '../gr-diff/gr-diff-line'; import { GrDiffGroup, + GrDiffGroupRange, GrDiffGroupType, hideInContextControl, rangeBySide, @@ -64,6 +65,10 @@ }; } +export interface ContentLoadNeededEventDetail { + lineRange: GrDiffGroupRange; +} + export abstract class GrDiffBuilder { private readonly _diff: DiffInfo; @@ -93,7 +98,7 @@ this._numLinesLeft = this._diff.content ? this._diff.content.reduce((sum, chunk) => { const left = chunk.a || chunk.ab; - return sum + (left ? left.length : 0); + return sum + (left?.length || chunk.skip || 0); }, 0) : 0; this._prefs = prefs; @@ -311,8 +316,13 @@ const td = this._createElement('td'); const showPartialLinks = numLines > PARTIAL_CONTEXT_AMOUNT; - - if (showPartialLinks && leftStart > 1) { + const firstGroupIsSkipped = !!contextGroups[0].skip; + const lastGroupIsSkipped = !!contextGroups[contextGroups.length - 1].skip; + const showAboveButton = + showPartialLinks && leftStart > 1 && !firstGroupIsSkipped; + const showBelowButton = + showPartialLinks && leftEnd < this._numLinesLeft && !lastGroupIsSkipped; + if (showAboveButton) { td.appendChild( this._createContextButton( ContextButtonType.ABOVE, @@ -322,7 +332,6 @@ ) ); } - td.appendChild( this._createContextButton( ContextButtonType.ALL, @@ -331,8 +340,7 @@ numLines ) ); - - if (showPartialLinks && leftEnd < this._numLinesLeft) { + if (showBelowButton) { td.appendChild( this._createContextButton( ContextButtonType.BELOW, @@ -342,7 +350,6 @@ ) ); } - return td; } @@ -359,7 +366,9 @@ let text = ''; let groups: GrDiffGroup[] = []; // The groups that replace this one if tapped. + let requiresLoad = false; if (type === GrDiffBuilder.ContextButtonType.ALL) { + requiresLoad = contextGroups.find(c => !!c.skip) !== undefined; const icon = this._createElement('iron-icon', 'showContext'); icon.setAttribute('icon', 'gr-icons:unfold-more'); button.appendChild(icon); @@ -368,6 +377,10 @@ if (numLines > 1) { text += 's'; } + if (requiresLoad) { + // Expanding content would require load of more data + text += ' (too large)'; + } groups.push(...contextGroups); } else if (type === GrDiffBuilder.ContextButtonType.ABOVE) { text = `+${context} above`; @@ -380,15 +393,36 @@ textSpan.textContent = text; button.appendChild(textSpan); - button.addEventListener('tap', e => { - const event = e as ContextEvent; - event.detail = { - groups, - section, - numLines, - }; - // Let it bubble up the DOM tree. - }); + if (requiresLoad) { + button.addEventListener('tap', e => { + e.stopPropagation(); + const firstRange = groups[0].lineRange; + const lastRange = groups[groups.length - 1].lineRange; + const lineRange = { + left: {start: firstRange.left.start, end: lastRange.left.end}, + right: {start: firstRange.right.start, end: lastRange.right.end}, + }; + button.dispatchEvent( + new CustomEvent<ContentLoadNeededEventDetail>('content-load-needed', { + detail: { + lineRange, + }, + bubbles: true, + composed: true, + }) + ); + }); + } else { + button.addEventListener('tap', e => { + const event = e as ContextEvent; + event.detail = { + groups, + section, + numLines, + }; + // Let it bubble up the DOM tree. + }); + } return button; }
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts index 39e12ca..08ea1a6 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
@@ -274,7 +274,7 @@ } _isCollapsibleChunk(chunk: DiffContent) { - return (chunk.ab || chunk.common) && !chunk.keyLocation; + return (chunk.ab || chunk.common || chunk.skip) && !chunk.keyLocation; } /** @@ -326,7 +326,11 @@ } _commonChunkLength(chunk: DiffContent) { + if (chunk.skip) { + return chunk.skip; + } console.assert(!!chunk.ab || !!chunk.common); + console.assert( !chunk.a || (!!chunk.b && chunk.a.length === chunk.b.length), 'common chunk needs same number of a and b lines: ', @@ -354,13 +358,21 @@ offsetLeft: number, offsetRight: number ): GrDiffGroup { - const type = chunk.ab ? GrDiffGroupType.BOTH : GrDiffGroupType.DELTA; + const type = + chunk.ab || chunk.skip ? GrDiffGroupType.BOTH : GrDiffGroupType.DELTA; const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight); const group = new GrDiffGroup(type, lines); group.keyLocation = !!chunk.keyLocation; group.dueToRebase = !!chunk.due_to_rebase; group.dueToMove = !!chunk.due_to_move; + group.skip = chunk.skip; group.ignoredWhitespaceOnly = !!chunk.common; + if (chunk.skip) { + group.lineRange = { + left: {start: offsetLeft, end: offsetLeft + chunk.skip - 1}, + right: {start: offsetRight, end: offsetRight + chunk.skip - 1}, + }; + } return group; } @@ -497,7 +509,7 @@ for (const chunk of chunks) { // If it isn't a common chunk, append it as-is and update line numbers. - if (!chunk.ab && !chunk.common) { + if (!chunk.ab && !chunk.skip && !chunk.common) { if (chunk.a) { leftLineNum += chunk.a.length; } @@ -522,7 +534,13 @@ leftLineNum += numLines; rightLineNum += numLines; - if (chunk.ab) { + if (chunk.skip) { + result.push({ + ...chunk, + skip: chunk.skip, + keyLocation: false, + }); + } else if (chunk.ab) { result.push( ...this._splitAtChunkEnds(chunk.ab, chunkEnds).map( ({lines, keyLocation}) => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js index 3a074bb..eed5900 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
@@ -171,6 +171,56 @@ }); }); + test('at the beginning with skip chunks', async () => { + element.context = 10; + const content = [ + {ab: new Array(20) + .fill('all work and no play make jack a dull boy')}, + {skip: 43900}, + {ab: new Array(30) + .fill('some other content')}, + {a: ['some other content']}, + ]; + + await element.process(content); + + const groups = element.groups; + + // group[0] is the file group + + const commonGroup = groups[1]; + + // Hidden context before + assert.equal(commonGroup.type, GrDiffGroupType.CONTEXT_CONTROL); + assert.instanceOf(commonGroup.contextGroups[0], GrDiffGroup); + assert.equal(commonGroup.contextGroups[0].lines.length, 20); + for (const l of commonGroup.contextGroups[0].lines) { + assert.equal(l.text, 'all work and no play make jack a dull boy'); + } + + // Skipped group + const skipGroup = commonGroup.contextGroups[1]; + assert.equal(skipGroup.skip, 43900); + const expectedRange = { + left: {start: 21, end: 43920}, + right: {start: 21, end: 43920}, + }; + assert.deepEqual(skipGroup.lineRange, expectedRange); + + // Hidden context after + assert.equal(commonGroup.contextGroups[2].lines.length, 20); + for (const l of commonGroup.contextGroups[2].lines) { + assert.equal(l.text, 'some other content'); + } + + // Displayed lines + assert.equal(groups[2].type, GrDiffGroupType.BOTH); + assert.equal(groups[2].lines.length, 10); + for (const l of groups[2].lines) { + assert.equal(l.text, 'some other content'); + } + }); + test('at the beginning, smaller than context', () => { element.context = 10; const content = [ @@ -414,6 +464,55 @@ }); }); + test('in the middle with skip chunks', async () => { + element.context = 10; + const content = [ + {a: ['all work and no play make andybons a dull boy']}, + {ab: new Array(20) + .fill('all work and no play make jill a dull girl')}, + {skip: 60}, + {ab: new Array(20) + .fill('all work and no play make jill a dull girl')}, + {a: ['all work and no play make andybons a dull boy']}, + ]; + + await element.process(content); + + const groups = element.groups; + + // group[0] is the file group + // group[1] is the chunk with a + // group[2] is the displayed part of ab before + + const commonGroup = groups[3]; + + // Hidden context before + assert.equal(commonGroup.type, GrDiffGroupType.CONTEXT_CONTROL); + assert.instanceOf(commonGroup.contextGroups[0], GrDiffGroup); + assert.equal(commonGroup.contextGroups[0].lines.length, 10); + for (const l of commonGroup.contextGroups[0].lines) { + assert.equal( + l.text, 'all work and no play make jill a dull girl'); + } + + // Skipped group + const skipGroup = commonGroup.contextGroups[1]; + assert.equal(skipGroup.skip, 60); + const expectedRange = { + left: {start: 22, end: 81}, + right: {start: 21, end: 80}, + }; + assert.deepEqual(skipGroup.lineRange, expectedRange); + + // Hidden context after + assert.equal(commonGroup.contextGroups[2].lines.length, 10); + for (const l of commonGroup.contextGroups[2].lines) { + assert.equal( + l.text, 'all work and no play make jill a dull girl'); + } + // group[4] is the displayed part of the second ab + }); + test('break up common diff chunks', () => { element.keyLocations = { left: {1: true},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts index 9f5cdf3..588b9d1 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts
@@ -38,7 +38,7 @@ end: number | null; } -interface GrDiffGroupRange { +export interface GrDiffGroupRange { left: Range; right: Range; } @@ -89,7 +89,13 @@ [before, hidden] = _splitCommonGroups(hidden, hiddenStart); } if (hiddenEnd) { - [hidden, after] = _splitCommonGroups(hidden, hiddenEnd - hiddenStart); + let beforeLength = 0; + if (before.length > 0) { + const beforeStart = before[0].lineRange.left.start || 0; + const beforeEnd = before[before.length - 1].lineRange.left.end || 0; + beforeLength = beforeEnd - beforeStart + 1; + } + [hidden, after] = _splitCommonGroups(hidden, hiddenEnd - beforeLength); } } else { [hidden, after] = [[], hidden]; @@ -106,6 +112,67 @@ } /** + * Splits a group in two, defined by leftSplit and rightSplit. Primarily to be + * used in function _splitCommonGroups + * Groups with some lines before and some lines after the split will be split + * into two groups, which will be put into the first and second list. + * + * @param group The group to be split in two + * @param leftSplit The line number relative to the split on the left side + * @param rightSplit The line number relative to the split on the right side + * @return two new groups, one before the split and another after it + */ +function _splitGroupInTwo( + group: GrDiffGroup, + leftSplit: number, + rightSplit: number +) { + let beforeSplit: GrDiffGroup | undefined; + let afterSplit: GrDiffGroup | undefined; + // split line is in the middle of a group, we need to break the group + // in lines before and after the split. + if (group.skip) { + // Currently we assume skip chunks "refuse" to be split. Expanding this + // group will in the future mean load more data - and therefore we want to + // fire an event when user wants to do it. + const closerToStartThanEnd = + leftSplit - (group.lineRange.left.start || 0) < + (group.lineRange.right.end || 0) - leftSplit; + if (closerToStartThanEnd) { + afterSplit = group; + } else { + beforeSplit = group; + } + } else { + const before = []; + const after = []; + for (const line of group.lines) { + if ( + (line.beforeNumber && line.beforeNumber < leftSplit) || + (line.afterNumber && line.afterNumber < rightSplit) + ) { + before.push(line); + } else { + after.push(line); + } + } + if (before.length) { + beforeSplit = + before.length === group.lines.length + ? group + : group.cloneWithLines(before); + } + if (after.length) { + afterSplit = + after.length === group.lines.length + ? group + : group.cloneWithLines(after); + } + } + return {beforeSplit, afterSplit}; +} + +/** * Splits a list of common groups into two lists of groups. * * Groups where all lines are before or all lines are after the split will be @@ -129,47 +196,28 @@ const beforeGroups = []; const afterGroups = []; for (const group of groups) { - if ( + const isCompletelyBefore = (group.lineRange.left.end || 0) < leftSplit || - (group.lineRange.right.end || 0) < rightSplit - ) { - beforeGroups.push(group); - continue; - } - if ( + (group.lineRange.right.end || 0) < rightSplit; + const isCompletelyAfter = leftSplit <= (group.lineRange.left.start || 0) || - rightSplit <= (group.lineRange.right.start || 0) - ) { + rightSplit <= (group.lineRange.right.start || 0); + if (isCompletelyBefore) { + beforeGroups.push(group); + } else if (isCompletelyAfter) { afterGroups.push(group); - continue; - } - - const before = []; - const after = []; - for (const line of group.lines) { - if ( - (line.beforeNumber && line.beforeNumber < leftSplit) || - (line.afterNumber && line.afterNumber < rightSplit) - ) { - before.push(line); - } else { - after.push(line); + } else { + const {beforeSplit, afterSplit} = _splitGroupInTwo( + group, + leftSplit, + rightSplit + ); + if (beforeSplit) { + beforeGroups.push(beforeSplit); } - } - - if (before.length) { - beforeGroups.push( - before.length === group.lines.length - ? group - : group.cloneWithLines(before) - ); - } - if (after.length) { - afterGroups.push( - after.length === group.lines.length - ? group - : group.cloneWithLines(after) - ); + if (afterSplit) { + afterGroups.push(afterSplit); + } } } return [beforeGroups, afterGroups]; @@ -213,6 +261,8 @@ contextGroups: GrDiffGroup[] = []; + skip?: number; + /** Both start and end line are inclusive. */ lineRange: GrDiffGroupRange = { left: {start: null, end: null},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js index 13fc4d1..3423834 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js
@@ -177,6 +177,35 @@ assert.deepEqual(collapsedGroups[3].lines, groups[2].lines.slice(1)); }); + suite('with skip chunks', () => { + setup(() => { + const skipGroup = new GrDiffGroup(GrDiffGroupType.BOTH); + skipGroup.skip = 60; + skipGroup.lineRange = { + left: {start: 8, end: 67}, + right: {start: 10, end: 69}, + }; + groups = [ + new GrDiffGroup(GrDiffGroupType.BOTH, [ + new GrDiffLine(GrDiffLineType.BOTH, 5, 7), + new GrDiffLine(GrDiffLineType.BOTH, 6, 8), + new GrDiffLine(GrDiffLineType.BOTH, 7, 9), + ]), + skipGroup, + new GrDiffGroup(GrDiffGroupType.BOTH, [ + new GrDiffLine(GrDiffLineType.BOTH, 68, 70), + new GrDiffLine(GrDiffLineType.BOTH, 69, 71), + new GrDiffLine(GrDiffLineType.BOTH, 70, 72), + ]), + ]; + }); + + test('refuses to split skip group when closer to before', () => { + const collapsedGroups = hideInContextControl(groups, 4, 10); + assert.deepEqual(groups, collapsedGroups); + }); + }); + test('groups unchanged if the hidden range is empty', () => { assert.deepEqual( hideInContextControl(groups, 0, 0), groups);
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts index 6b184b9..96cb02a 100644 --- a/polygerrit-ui/app/types/common.ts +++ b/polygerrit-ui/app/types/common.ts
@@ -1254,7 +1254,7 @@ edit_b?: number[][]; due_to_rebase?: boolean; due_to_move?: boolean; - skip?: string; + skip?: number; common?: string; keyLocation?: boolean; }