Merge "Do not split DELTA groups in context control which are not common."
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({
