Do not split DELTA groups in context control which are not common. Note: Till now gr-diff-processor made sure only groups with common lines are hidden under contextControl group. The existing test was working because the delta used for split had same number of lines on both sides of diff. Step 2 of go/hide_diff_with_delta Change-Id: I117fae54ab25c00546b004950071a055d3052b6d Release-Notes: skip Google-Bug-Id: b/345126277
diff --git a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts index 79feca0..1ec5262e 100644 --- a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts +++ b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
@@ -380,7 +380,7 @@ let classes = 'contextControlButton showContext '; if (type === ContextButtonType.ALL) { - if (this.group.hasDeltaGroup()) { + if (this.group.hasNonCommonDeltaGroup()) { text = '+ Unrelated changes'; ariaLabel = 'Show unrelated changes'; classes += ' unrelatedChanges '; @@ -499,7 +499,7 @@ * Creates a container div with partial (+10) expansion buttons (above and/or below). */ private createPartialExpansionButtons() { - if (!this.showPartialLinks() || this.group?.hasDeltaGroup()) { + if (!this.showPartialLinks() || this.group?.hasNonCommonDeltaGroup()) { return undefined; } let aboveButton; @@ -532,7 +532,7 @@ !this.showPartialLinks() || !this.renderPreferences?.use_block_expansion || this.group?.hasSkipGroup() || - this.group?.hasDeltaGroup() + this.group?.hasNonCommonDeltaGroup() ) { return undefined; }
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts index 0739562..c7e9f44 100644 --- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts +++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
@@ -165,7 +165,8 @@ * Groups where all lines are before or all lines are after the split will be * retained as is and put into the first or second list respectively. 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. + * two groups, which will be put into the first and second list. Groups with + * type DELTA which are not common will not be split. * * @param split A line number offset relative to the first group's * start line at which the groups should be split. @@ -179,31 +180,41 @@ if (groups.length === 0) return [[], []]; const leftSplit = groups[0].lineRange.left.start_line + split; const rightSplit = groups[0].lineRange.right.start_line + split; - + let isSplitDone = false; const beforeGroups = []; const afterGroups = []; for (const group of groups) { - const isCompletelyBefore = - group.lineRange.left.end_line < leftSplit || - group.lineRange.right.end_line < rightSplit; - const isCompletelyAfter = - leftSplit <= group.lineRange.left.start_line || - rightSplit <= group.lineRange.right.start_line; - if (isCompletelyBefore) { - beforeGroups.push(group); - } else if (isCompletelyAfter) { + if (isSplitDone) { afterGroups.push(group); + } else if ( + group.type === GrDiffGroupType.DELTA && + !group.ignoredWhitespaceOnly + ) { + beforeGroups.push(group); } else { - const {beforeSplit, afterSplit} = splitGroupInTwo( - group, - leftSplit, - rightSplit - ); - if (beforeSplit) { - beforeGroups.push(beforeSplit); - } - if (afterSplit) { - afterGroups.push(afterSplit); + const isCompletelyBefore = + group.lineRange.left.end_line < leftSplit || + group.lineRange.right.end_line < rightSplit; + const isCompletelyAfter = + leftSplit <= group.lineRange.left.start_line || + rightSplit <= group.lineRange.right.start_line; + if (isCompletelyBefore) { + beforeGroups.push(group); + } else if (isCompletelyAfter) { + afterGroups.push(group); + } else { + const {beforeSplit, afterSplit} = splitGroupInTwo( + group, + leftSplit, + rightSplit + ); + if (beforeSplit) { + beforeGroups.push(beforeSplit); + } + if (afterSplit) { + afterGroups.push(afterSplit); + } + isSplitDone = true; } } } @@ -438,9 +449,13 @@ ); } - /** Returns true if it contains a DELTA group. */ - hasDeltaGroup() { - return this.contextGroups?.some(g => g.type === GrDiffGroupType.DELTA); + /** Returns true if it contains a DELTA group excluding whitespace only + * changes. + */ + hasNonCommonDeltaGroup() { + return this.contextGroups?.some( + g => g.type === GrDiffGroupType.DELTA && !g.ignoredWhitespaceOnly + ); } containsLine(side: Side, line: LineNumber) {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts index bbbb4ad..c8446f0 100644 --- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts +++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
@@ -97,6 +97,8 @@ suite('hideInContextControl', () => { let groups: GrDiffGroup[]; + let groupsWithDelta: GrDiffGroup[]; + let groupsWithWhiteSpaceOnlyChange: GrDiffGroup[]; setup(() => { groups = [ new GrDiffGroup({ @@ -108,6 +110,46 @@ ], }), new GrDiffGroup({ + type: GrDiffGroupType.BOTH, + lines: [ + new GrDiffLine(GrDiffLineType.BOTH, 8, 10), + new GrDiffLine(GrDiffLineType.BOTH, 9, 11), + new GrDiffLine(GrDiffLineType.BOTH, 10, 12), + new GrDiffLine(GrDiffLineType.BOTH, 11, 13), + ], + }), + new GrDiffGroup({ + type: GrDiffGroupType.BOTH, + lines: [ + new GrDiffLine(GrDiffLineType.BOTH, 12, 14), + new GrDiffLine(GrDiffLineType.BOTH, 13, 15), + new GrDiffLine(GrDiffLineType.BOTH, 14, 16), + ], + }), + ]; + + groupsWithWhiteSpaceOnlyChange = [ + groups[0], + new GrDiffGroup({ + type: GrDiffGroupType.DELTA, + lines: [ + new GrDiffLine(GrDiffLineType.REMOVE, 8), + new GrDiffLine(GrDiffLineType.ADD, 0, 10), + new GrDiffLine(GrDiffLineType.REMOVE, 9), + new GrDiffLine(GrDiffLineType.ADD, 0, 11), + new GrDiffLine(GrDiffLineType.REMOVE, 10), + new GrDiffLine(GrDiffLineType.ADD, 0, 12), + new GrDiffLine(GrDiffLineType.REMOVE, 11), + new GrDiffLine(GrDiffLineType.ADD, 0, 13), + ], + ignoredWhitespaceOnly: true, + }), + groups[2], + ]; + + groupsWithDelta = [ + groups[0], + new GrDiffGroup({ type: GrDiffGroupType.DELTA, lines: [ new GrDiffLine(GrDiffLineType.REMOVE, 8), @@ -120,14 +162,7 @@ new GrDiffLine(GrDiffLineType.ADD, 0, 13), ], }), - new GrDiffGroup({ - type: GrDiffGroupType.BOTH, - lines: [ - new GrDiffLine(GrDiffLineType.BOTH, 12, 14), - new GrDiffLine(GrDiffLineType.BOTH, 13, 15), - new GrDiffLine(GrDiffLineType.BOTH, 14, 16), - ], - }), + groups[2], ]; }); @@ -144,29 +179,33 @@ assert.equal(collapsedGroups[2], groups[2]); }); + test('does not hides when split is at delta group in context control', () => { + const collapsedGroups = hideInContextControl(groupsWithDelta, 3, 7); + assert.equal(collapsedGroups.length, 3); + + assert.equal(collapsedGroups[0], groupsWithDelta[0]); + assert.equal(collapsedGroups[1], groupsWithDelta[1]); + assert.equal(collapsedGroups[2], groupsWithDelta[2]); + }); + test('splits partially hidden groups', () => { const collapsedGroups = hideInContextControl(groups, 4, 8); assert.equal(collapsedGroups.length, 4); assert.equal(collapsedGroups[0], groups[0]); - assert.equal(collapsedGroups[1].type, GrDiffGroupType.DELTA); - assert.deepEqual(collapsedGroups[1].adds, [groups[1].adds[0]]); - assert.deepEqual(collapsedGroups[1].removes, [groups[1].removes[0]]); + assert.equal(collapsedGroups[1].type, GrDiffGroupType.BOTH); + assert.deepEqual(collapsedGroups[1].lines, [groups[1].lines[0]]); assert.equal(collapsedGroups[2].type, GrDiffGroupType.CONTEXT_CONTROL); assert.equal(collapsedGroups[2].contextGroups.length, 2); assert.equal( collapsedGroups[2].contextGroups[0].type, - GrDiffGroupType.DELTA + GrDiffGroupType.BOTH ); assert.deepEqual( - collapsedGroups[2].contextGroups[0].adds, - groups[1].adds.slice(1) - ); - assert.deepEqual( - collapsedGroups[2].contextGroups[0].removes, - groups[1].removes.slice(1) + collapsedGroups[2].contextGroups[0].lines, + groups[1].lines.slice(1) ); assert.equal( @@ -181,6 +220,54 @@ assert.deepEqual(collapsedGroups[3].lines, groups[2].lines.slice(1)); }); + test('splits partially hidden common delta groups', () => { + const collapsedGroups = hideInContextControl( + groupsWithWhiteSpaceOnlyChange, + 4, + 8 + ); + assert.equal(collapsedGroups.length, 4); + assert.equal(collapsedGroups[0], groupsWithWhiteSpaceOnlyChange[0]); + + assert.equal(collapsedGroups[1].type, GrDiffGroupType.DELTA); + assert.deepEqual(collapsedGroups[1].adds, [ + groupsWithWhiteSpaceOnlyChange[1].adds[0], + ]); + assert.deepEqual(collapsedGroups[1].removes, [ + groupsWithWhiteSpaceOnlyChange[1].removes[0], + ]); + + assert.equal(collapsedGroups[2].type, GrDiffGroupType.CONTEXT_CONTROL); + assert.equal(collapsedGroups[2].contextGroups.length, 2); + + assert.equal( + collapsedGroups[2].contextGroups[0].type, + GrDiffGroupType.DELTA + ); + assert.deepEqual( + collapsedGroups[2].contextGroups[0].adds, + groupsWithWhiteSpaceOnlyChange[1].adds.slice(1) + ); + assert.deepEqual( + collapsedGroups[2].contextGroups[0].removes, + groupsWithWhiteSpaceOnlyChange[1].removes.slice(1) + ); + + assert.equal( + collapsedGroups[2].contextGroups[1].type, + GrDiffGroupType.BOTH + ); + assert.deepEqual(collapsedGroups[2].contextGroups[1].lines, [ + groupsWithWhiteSpaceOnlyChange[2].lines[0], + ]); + + assert.equal(collapsedGroups[3].type, GrDiffGroupType.BOTH); + assert.deepEqual( + collapsedGroups[3].lines, + groupsWithWhiteSpaceOnlyChange[2].lines.slice(1) + ); + }); + suite('with skip chunks', () => { setup(() => { const skipGroup = new GrDiffGroup({