Do not collapse delta groups in context control group if they are
focussed
This is done by checking if the focussed range overlaps with the
delta group.
Change-Id: Iae08245503871925a6ca81da34aee1bcb53df3a7
Release-Notes: skip
Google-Bug-Id: b/345126277
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 1561dad..d138414 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -80,10 +80,14 @@
private groups: GrDiffGroup[] = [];
+ // visible for testing
+ diffRangesToFocus?: DiffRangesToFocus;
+
constructor(options: ProcessingOptions) {
this.context = options.context;
this.keyLocations = options.keyLocations ?? {left: {}, right: {}};
this.isBinary = options.isBinary ?? false;
+ this.diffRangesToFocus = options.diffRangesToFocus;
}
/**
@@ -127,7 +131,7 @@
processNext(state: State, chunks: DiffContent[]) {
const firstUncollapsibleChunkIndex = this.firstUncollapsibleChunkIndex(
chunks,
- state.chunkIndex
+ state
);
if (firstUncollapsibleChunkIndex === state.chunkIndex) {
const chunk = chunks[state.chunkIndex];
@@ -162,19 +166,67 @@
return chunk.ab || chunk.b || [];
}
- private firstUncollapsibleChunkIndex(chunks: DiffContent[], offset: number) {
- let chunkIndex = offset;
+ private firstUncollapsibleChunkIndex(chunks: DiffContent[], state: State) {
+ let chunkIndex = state.chunkIndex;
+ let offsetLeft = state.lineNums.left;
+ let offsetRight = state.lineNums.right;
while (
chunkIndex < chunks.length &&
- this.isCollapsibleChunk(chunks[chunkIndex])
+ this.isCollapsibleChunk(chunks[chunkIndex], offsetLeft, offsetRight)
) {
+ offsetLeft += this.chunkLength(chunks[chunkIndex], Side.LEFT);
+ offsetRight += this.chunkLength(chunks[chunkIndex], Side.RIGHT);
chunkIndex++;
}
return chunkIndex;
}
- private isCollapsibleChunk(chunk: DiffContent) {
- return (chunk.ab || chunk.common || chunk.skip) && !chunk.keyLocation;
+ private isCollapsibleChunk(
+ chunk: DiffContent,
+ offsetLeft: number,
+ offsetRight: number
+ ) {
+ return (
+ (chunk.ab ||
+ chunk.common ||
+ chunk.skip ||
+ this.isChunkOutsideOfFocusRange(chunk, offsetLeft, offsetRight)) &&
+ !chunk.keyLocation
+ );
+ }
+
+ private isChunkOutsideOfFocusRange(
+ chunk: DiffContent,
+ offsetLeft: number,
+ offsetRight: number
+ ) {
+ if (!this.diffRangesToFocus) {
+ return false;
+ }
+ const leftLineCount = this.linesLeft(chunk).length;
+ const rightLineCount = this.linesRight(chunk).length;
+ const hasLeftSideOverlap = this.diffRangesToFocus.left.some(range =>
+ this.hasAnyOverlap(
+ {start: offsetLeft, end: offsetLeft + leftLineCount},
+ range
+ )
+ );
+ const hasRightSideOverlap = this.diffRangesToFocus.right.some(range =>
+ this.hasAnyOverlap(
+ {start: offsetRight, end: offsetRight + rightLineCount},
+ range
+ )
+ );
+ return !hasLeftSideOverlap && !hasRightSideOverlap;
+ }
+
+ private hasAnyOverlap(
+ firstRange: {start: number; end: number},
+ secondRange: {start: number; end: number}
+ ) {
+ const startOverlap = Math.max(firstRange.start, secondRange.start);
+ const endOverlap = Math.min(firstRange.end, secondRange.end);
+ return startOverlap <= endOverlap;
}
/**
@@ -196,8 +248,12 @@
state.chunkIndex,
firstUncollapsibleChunkIndex
);
- const lineCount = collapsibleChunks.reduce(
- (sum, chunk) => sum + this.commonChunkLength(chunk),
+ const leftLineCount = collapsibleChunks.reduce(
+ (sum, chunk) => sum + this.chunkLength(chunk, Side.LEFT),
+ 0
+ );
+ const rightLineCount = collapsibleChunks.reduce(
+ (sum, chunk) => sum + this.chunkLength(chunk, Side.RIGHT),
0
);
@@ -208,25 +264,50 @@
);
const hasSkippedGroup = !!groups.find(g => g.skip);
- if (this.context !== FULL_CONTEXT || hasSkippedGroup) {
+ const hasNonCommonDeltaGroup = !!groups.find(
+ g => g.type === GrDiffGroupType.DELTA && !g.ignoredWhitespaceOnly
+ );
+ if (
+ this.context !== FULL_CONTEXT ||
+ hasSkippedGroup ||
+ hasNonCommonDeltaGroup
+ ) {
const contextNumLines = this.context > 0 ? this.context : 0;
const hiddenStart = state.chunkIndex === 0 ? 0 : contextNumLines;
- const hiddenEnd =
- lineCount -
+ const hiddenEndLeft =
+ leftLineCount -
(firstUncollapsibleChunkIndex === chunks.length ? 0 : this.context);
- groups = hideInContextControl(groups, hiddenStart, hiddenEnd);
+ const hiddenEndRight =
+ rightLineCount -
+ (firstUncollapsibleChunkIndex === chunks.length ? 0 : this.context);
+ groups = hideInContextControl(
+ groups,
+ hiddenStart,
+ hiddenEndLeft,
+ hiddenEndRight
+ );
}
return {
lineDelta: {
- left: lineCount,
- right: lineCount,
+ left: leftLineCount,
+ right: rightLineCount,
},
groups,
newChunkIndex: firstUncollapsibleChunkIndex,
};
}
+ private chunkLength(chunk: DiffContent, side: Side) {
+ if (chunk.skip || chunk.common || chunk.ab) {
+ return this.commonChunkLength(chunk);
+ } else if (side === Side.LEFT) {
+ return this.linesLeft(chunk).length;
+ } else {
+ return this.linesRight(chunk).length;
+ }
+ }
+
private commonChunkLength(chunk: DiffContent) {
if (chunk.skip) {
return chunk.skip;
@@ -248,9 +329,8 @@
): GrDiffGroup[] {
return chunks.map(chunk => {
const group = this.chunkToGroup(chunk, offsetLeft, offsetRight);
- const chunkLength = this.commonChunkLength(chunk);
- offsetLeft += chunkLength;
- offsetRight += chunkLength;
+ offsetLeft += this.chunkLength(chunk, Side.LEFT);
+ offsetRight += this.chunkLength(chunk, Side.RIGHT);
return group;
});
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
index f6b9737..3f01096 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
@@ -933,6 +933,128 @@
);
});
});
+
+ suite('with diffRangesToFocus', () => {
+ let state: State;
+ let chunks: DiffContent[];
+
+ setup(() => {
+ state = {
+ lineNums: {left: 0, right: 0},
+ chunkIndex: 0,
+ };
+ processor.context = 3;
+ processor.diffRangesToFocus = {
+ left: [{start: 6, end: 7}],
+ right: [{start: 6, end: 6}],
+ };
+ chunks = [
+ {
+ ab: Array.from<string>({length: 5}).fill(
+ 'all work and no play make jill a dull boy'
+ ),
+ },
+ {
+ a: ['Old ', ' Change!'],
+ b: ['New Change'],
+ },
+ {
+ ab: Array.from<string>({length: 5}).fill(
+ 'all work and no play make jack a dull boy'
+ ),
+ },
+ {
+ a: ['Old ', ' Change!', '1'],
+ b: ['New Change', '2'],
+ },
+ ];
+ });
+
+ test('focussed group is not collapsed in context control group', () => {
+ const result = processor.processNext(state, chunks);
+
+ // This should consider second delta group as focussed and not collapse it.
+ // This result is first chunk itself.
+ assert.equal(result.groups.length, 1);
+ assert.equal(result.groups[0].type, GrDiffGroupType.BOTH);
+ assert.equal(result.groups[0].lines.length, 5);
+ });
+
+ test('collapsing delta group at end in context control group', () => {
+ state = {
+ lineNums: {left: 7, right: 6},
+ chunkIndex: 2,
+ };
+ const result = processor.processNext(state, [
+ ...chunks,
+ {
+ ab: Array.from<string>({length: 5}).fill(
+ 'all work and no play make jack a dull boy'
+ ),
+ },
+ ]);
+
+ // The first chunk is split into two groups:
+ // 1) A common group which is rendered before contextControl group
+ // 2) Second group is a context control which contains split from 4th chunk
+ // and the delta group and the last unchanged group.
+ assert.equal(result.groups.length, 2);
+ assert.equal(result.groups[0].type, GrDiffGroupType.BOTH);
+ assert.equal(result.groups[0].lines.length, 3);
+ assert.equal(result.groups[1].type, GrDiffGroupType.CONTEXT_CONTROL);
+ assert.equal(result.groups[1].contextGroups.length, 3);
+ assert.equal(result.groups[1].contextGroups[0].lines.length, 2);
+ assert.equal(
+ result.groups[1].contextGroups[1].type,
+ GrDiffGroupType.DELTA
+ );
+ assert.equal(
+ result.groups[1].contextGroups[2].type,
+ GrDiffGroupType.BOTH
+ );
+ });
+
+ test('collapsing delta group in middle in context control group', () => {
+ state = {
+ lineNums: {left: 7, right: 6},
+ chunkIndex: 2,
+ };
+ const result = processor.processNext(state, chunks);
+
+ // The first chunk is split into two groups:
+ // 1) A common group which is rendered before contextControl group
+ // 2) Second group is a context control which contains split from 4th chunk
+ // and the delta group.
+ assert.equal(result.groups.length, 2);
+ assert.equal(result.groups[0].type, GrDiffGroupType.BOTH);
+ assert.equal(result.groups[0].lines.length, 3);
+ assert.equal(result.groups[1].type, GrDiffGroupType.CONTEXT_CONTROL);
+ assert.equal(result.groups[1].contextGroups.length, 2);
+ assert.equal(result.groups[1].contextGroups[0].lines.length, 2);
+ assert.equal(
+ result.groups[1].contextGroups[1].type,
+ GrDiffGroupType.DELTA
+ );
+ });
+
+ test('do not collapse if there are not enough context lines', () => {
+ processor.context = 10;
+ state = {
+ lineNums: {left: 7, right: 6},
+ chunkIndex: 2,
+ };
+ const result = processor.processNext(state, chunks);
+
+ // The first chunk is split into two groups:
+ // 1) A common group which is rendered before contextControl group
+ // 2) Second group is a context control which contains split from 4th chunk
+ // and the delta group.
+ assert.equal(result.groups.length, 2);
+ assert.equal(result.groups[0].type, GrDiffGroupType.BOTH);
+ assert.equal(result.groups[0].lines.length, 5);
+ assert.equal(result.groups[1].type, GrDiffGroupType.DELTA);
+ });
+ });
});
suite('gr-diff-processor helpers', () => {
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 c7e9f44..e7df002 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
@@ -42,40 +42,56 @@
* @param hiddenStart The first element to be hidden, as a
* non-negative line number offset relative to the first group's start
* line, left and right respectively.
- * @param hiddenEnd The first visible element after the hidden range,
- * as a non-negative line number offset relative to the first group's
- * start line, left and right respectively.
+ * @param hiddenEndLeft The first visible element after the hidden range,
+ * as a non-negative line number offset for left side relative to the first
+ * group's start line.
+ * @param hiddenEndRight The first visible element after the hidden range,
+ * as a non-negative line number offset for right side relative to the first
+ * group's start line. If not provided hiddenEndLeft will be used.
*/
export function hideInContextControl(
groups: readonly GrDiffGroup[],
hiddenStart: number,
- hiddenEnd: number
+ hiddenEndLeft: number,
+ hiddenEndRight?: number
): GrDiffGroup[] {
if (groups.length === 0) return [];
// Clamp hiddenStart and hiddenEnd - inspired by e.g. substring
hiddenStart = Math.max(hiddenStart, 0);
- hiddenEnd = Math.max(hiddenEnd, hiddenStart);
+ hiddenEndLeft = Math.max(hiddenEndLeft, hiddenStart);
+ hiddenEndRight = Math.max(hiddenEndRight ?? hiddenEndLeft, hiddenStart);
let before: GrDiffGroup[] = [];
let hidden = groups;
let after: readonly GrDiffGroup[] = [];
- const numHidden = hiddenEnd - hiddenStart;
+ const numHiddenLeft = hiddenEndLeft - hiddenStart;
+ const numHiddenRight = hiddenEndRight - hiddenStart;
// Showing a context control row for less than 4 lines does not make much,
// because then that row would consume as much space as the collapsed code.
- if (numHidden > 3) {
+ if (numHiddenLeft > 3 && numHiddenRight > 3) {
if (hiddenStart) {
- [before, hidden] = splitCommonGroups(hidden, hiddenStart);
+ [before, hidden] = splitCommonGroups(hidden, hiddenStart, hiddenStart);
}
- if (hiddenEnd) {
- let beforeLength = 0;
+ if (hiddenEndLeft && hiddenEndRight) {
+ let beforeLengthLeft = 0;
+ let beforeLengthRight = 0;
if (before.length > 0) {
- const beforeStart = before[0].lineRange.left.start_line;
- const beforeEnd = before[before.length - 1].lineRange.left.end_line;
- beforeLength = beforeEnd - beforeStart + 1;
+ const beforeStartLeft = before[0].lineRange.left.start_line;
+ const beforeEndLeft = before[before.length - 1].lineRange.left.end_line;
+ beforeLengthLeft = beforeEndLeft - beforeStartLeft + 1;
+
+ const beforeStartRight = before[0].lineRange.right.start_line;
+ const beforeEndRight =
+ before[before.length - 1].lineRange.right.end_line;
+ beforeLengthRight = beforeEndRight - beforeStartRight + 1;
}
- [hidden, after] = splitCommonGroups(hidden, hiddenEnd - beforeLength);
+ [hidden, after] = splitCommonGroups(
+ hidden,
+ hiddenEndLeft - beforeLengthLeft,
+ hiddenEndRight - beforeLengthRight
+ );
}
} else {
[hidden, after] = [[], hidden];
@@ -168,18 +184,21 @@
* 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.
+ * @param splitLeft A line number offset for left side relative to the first
+ * group's start line at which the groups should be split.
+ * @param splitRight A line number offset for right side relative to the first
+ * group's start line at which the groups should be split.
* @return The outer array has 2 elements, the
* list of groups before and the list of groups after the split.
*/
function splitCommonGroups(
groups: readonly GrDiffGroup[],
- split: number
+ splitLeft: number,
+ splitRight: number
): GrDiffGroup[][] {
if (groups.length === 0) return [[], []];
- const leftSplit = groups[0].lineRange.left.start_line + split;
- const rightSplit = groups[0].lineRange.right.start_line + split;
+ const leftSplit = groups[0].lineRange.left.start_line + splitLeft;
+ const rightSplit = groups[0].lineRange.right.start_line + splitRight;
let isSplitDone = false;
const beforeGroups = [];
const afterGroups = [];
