Make GrDiffGroup immutable
...and consolidate all three location that were computing lineRange in
GrDiffGroup. This is to prevent lineRange from getting out of sync.
Change-Id: I44ac7d5cafa1ce811a2db824f66abcf3c154f03d
diff --git a/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts b/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts
index 1d68335..773e22e 100644
--- a/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts
+++ b/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts
@@ -242,8 +242,11 @@
}
private numLines() {
- const {leftStart, leftEnd} = this.contextRange();
- return leftEnd - leftStart + 1;
+ assertIsDefined(this.group);
+ // In context groups, there is the same number of lines left and right
+ const left = this.group.lineRange.left;
+ // Both start and end inclusive, so we need to add 1.
+ return left.end_line - left.start_line + 1;
}
private createExpandAllButtonContainer() {
@@ -351,21 +354,11 @@
groups: GrDiffGroup[]
) {
return (e: Event) => {
+ assertIsDefined(this.group);
e.stopPropagation();
if (type === ContextButtonType.ALL && this.partialContent) {
- const {leftStart, leftEnd, rightStart, rightEnd} = this.contextRange();
- const lineRange = {
- left: {
- start_line: leftStart,
- end_line: leftEnd,
- },
- right: {
- start_line: rightStart,
- end_line: rightEnd,
- },
- };
fire(this, 'content-load-needed', {
- lineRange,
+ lineRange: this.group.lineRange,
});
} else {
assertIsDefined(this.section, 'section');
@@ -423,6 +416,7 @@
* Creates a container div with block expansion buttons (above and/or below).
*/
private createBlockExpansionButtons() {
+ assertIsDefined(this.group, 'group');
if (
!this.showPartialLinks() ||
!this.renderPreferences?.use_block_expansion ||
@@ -436,14 +430,14 @@
aboveBlockButton = this.createBlockButton(
ContextButtonType.BLOCK_ABOVE,
this.numLines(),
- this.contextRange().rightStart - 1
+ this.group.lineRange.right.start_line - 1
);
}
if (this.showBelow()) {
belowBlockButton = this.createBlockButton(
ContextButtonType.BLOCK_BELOW,
this.numLines(),
- this.contextRange().rightEnd + 1
+ this.group.lineRange.right.end_line + 1
);
}
if (aboveBlockButton || belowBlockButton) {
@@ -503,22 +497,6 @@
return this.createContextButton(buttonType, linesToExpand, tooltip);
}
- private contextRange() {
- if (!this.group) {
- throw new Error('Range can only be computed when group is set.');
- }
- return {
- leftStart: this.group.contextGroups[0].lineRange.left.start_line,
- leftEnd:
- this.group.contextGroups[this.group.contextGroups.length - 1].lineRange
- .left.end_line,
- rightStart: this.group.contextGroups[0].lineRange.right.start_line,
- rightEnd:
- this.group.contextGroups[this.group.contextGroups.length - 1].lineRange
- .right.end_line,
- };
- }
-
private hasValidProperties() {
return !!(this.diff && this.section && this.group?.contextGroups?.length);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls_test.ts b/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls_test.ts
index a5f8566..92debda 100644
--- a/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls_test.ts
@@ -50,9 +50,10 @@
line.text = 'lorem upsum';
lines.push(line);
}
- const result = new GrDiffGroup(GrDiffGroupType.CONTEXT_CONTROL);
- result.contextGroups = [new GrDiffGroup(GrDiffGroupType.BOTH, lines)];
- return result;
+ return new GrDiffGroup({
+ type: GrDiffGroupType.CONTEXT_CONTROL,
+ contextGroups: [new GrDiffGroup({type: GrDiffGroupType.BOTH, lines})],
+ });
}
test('no +10 buttons for 10 or less lines', async () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
index 44b0b8b..413a292 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
@@ -252,31 +252,34 @@
suite('_isTotal', () => {
test('is total for add', () => {
- const group = new GrDiffGroup(GrDiffGroupType.DELTA);
+ const lines = [];
for (let idx = 0; idx < 10; idx++) {
- group.addLine(new GrDiffLine(GrDiffLineType.ADD));
+ lines.push(new GrDiffLine(GrDiffLineType.ADD));
}
+ const group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines});
assert.isTrue(GrDiffBuilder.prototype._isTotal(group));
});
test('is total for remove', () => {
- const group = new GrDiffGroup(GrDiffGroupType.DELTA);
+ const lines = [];
for (let idx = 0; idx < 10; idx++) {
- group.addLine(new GrDiffLine(GrDiffLineType.REMOVE));
+ lines.push(new GrDiffLine(GrDiffLineType.REMOVE));
}
+ const group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines});
assert.isTrue(GrDiffBuilder.prototype._isTotal(group));
});
test('not total for empty', () => {
- const group = new GrDiffGroup(GrDiffGroupType.BOTH);
+ const group = new GrDiffGroup({type: GrDiffGroupType.BOTH});
assert.isFalse(GrDiffBuilder.prototype._isTotal(group));
});
test('not total for non-delta', () => {
- const group = new GrDiffGroup(GrDiffGroupType.DELTA);
+ const lines = [];
for (let idx = 0; idx < 10; idx++) {
- group.addLine(new GrDiffLine(GrDiffLineType.BOTH));
+ lines.push(new GrDiffLine(GrDiffLineType.BOTH));
}
+ const group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines});
assert.isFalse(GrDiffBuilder.prototype._isTotal(group));
});
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js
index 2be85c3..5f3fb72 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js
@@ -52,7 +52,7 @@
lines[1].text = ' print "Hello World";';
lines[2].text = ' return True';
- group = new GrDiffGroup(GrDiffGroupType.BOTH, lines);
+ group = new GrDiffGroup({type: GrDiffGroupType.BOTH, lines});
});
test('creates the section', () => {
@@ -104,7 +104,7 @@
];
lines[0].text = 'def hello_world():';
lines[1].text = ' print "Hello World"';
- const group = new GrDiffGroup(GrDiffGroupType.DELTA, lines);
+ const group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines});
group.moveDetails = {changed: false};
const sectionEl = diffBuilder.buildSectionElement(group);
@@ -127,7 +127,7 @@
];
lines[0].text = 'def hello_world():';
lines[1].text = ' print "Hello World"';
- const group = new GrDiffGroup(GrDiffGroupType.DELTA, lines);
+ const group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines});
group.moveDetails = {changed: false};
const sectionEl = diffBuilder.buildSectionElement(group);
@@ -160,7 +160,7 @@
lines[2].text = 'def hello_universe()';
lines[3].text = ' print "Hello Universe"';
- group = new GrDiffGroup(GrDiffGroupType.DELTA, lines);
+ group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines});
});
test('creates the section', () => {
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 b1e15a8..4d8efed 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
@@ -365,22 +365,27 @@
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.moveDetails = chunk.move_details;
- group.skip = chunk.skip;
- group.ignoredWhitespaceOnly = !!chunk.common;
+ const options = {
+ moveDetails: chunk.move_details,
+ dueToRebase: !!chunk.due_to_rebase,
+ ignoredWhitespaceOnly: !!chunk.common,
+ keyLocation: !!chunk.keyLocation,
+ };
if (chunk.skip) {
- group.lineRange = {
- left: {start_line: offsetLeft, end_line: offsetLeft + chunk.skip - 1},
- right: {
- start_line: offsetRight,
- end_line: offsetRight + chunk.skip - 1,
- },
- };
+ return new GrDiffGroup({
+ type,
+ skip: chunk.skip,
+ offsetLeft,
+ offsetRight,
+ ...options,
+ });
+ } else {
+ return new GrDiffGroup({
+ type,
+ lines,
+ ...options,
+ });
}
- return group;
}
_linesFromChunk(chunk: DiffContent, offsetLeft: number, offsetRight: number) {
@@ -456,7 +461,7 @@
const line = new GrDiffLine(GrDiffLineType.BOTH);
line.beforeNumber = number;
line.afterNumber = number;
- return new GrDiffGroup(GrDiffGroupType.BOTH, [line]);
+ return new GrDiffGroup({type: GrDiffGroupType.BOTH, lines: [line]});
}
/**
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 40047f3..9778bb7 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
@@ -90,9 +90,12 @@
const result = [...before];
if (hidden.length) {
- const ctxGroup = new GrDiffGroup(GrDiffGroupType.CONTEXT_CONTROL, []);
- ctxGroup.contextGroups = [...hidden];
- result.push(ctxGroup);
+ result.push(
+ new GrDiffGroup({
+ type: GrDiffGroupType.CONTEXT_CONTROL,
+ contextGroups: [...hidden],
+ })
+ );
}
result.push(...after);
return result;
@@ -210,57 +213,131 @@
return [beforeGroups, afterGroups];
}
+export interface GrMoveDetails {
+ changed: boolean;
+ range?: {
+ start: number;
+ end: number;
+ };
+}
+
+/** A chunk of the diff that should be rendered together. */
export class GrDiffGroup {
- /**
- * A chunk of the diff that should be rendered together.
- *
- * @constructor
- * @param type
- * @param opt_lines
- */
- constructor(readonly type: GrDiffGroupType, lines: GrDiffLine[] = []) {
- lines.forEach((line: GrDiffLine) => this.addLine(line));
+ constructor(
+ options:
+ | {
+ type: GrDiffGroupType.BOTH | GrDiffGroupType.DELTA;
+ lines?: GrDiffLine[];
+ skip?: undefined;
+ moveDetails?: GrMoveDetails;
+ dueToRebase?: boolean;
+ ignoredWhitespaceOnly?: boolean;
+ keyLocation?: boolean;
+ }
+ | {
+ type: GrDiffGroupType.BOTH | GrDiffGroupType.DELTA;
+ lines?: undefined;
+ skip: number;
+ offsetLeft: number;
+ offsetRight: number;
+ moveDetails?: GrMoveDetails;
+ dueToRebase?: boolean;
+ ignoredWhitespaceOnly?: boolean;
+ keyLocation?: boolean;
+ }
+ | {
+ type: GrDiffGroupType.CONTEXT_CONTROL;
+ contextGroups: GrDiffGroup[];
+ }
+ ) {
+ this.type = options.type;
+ switch (options.type) {
+ case GrDiffGroupType.BOTH:
+ case GrDiffGroupType.DELTA: {
+ this.moveDetails = options.moveDetails;
+ this.dueToRebase = options.dueToRebase ?? false;
+ this.ignoredWhitespaceOnly = options.ignoredWhitespaceOnly ?? false;
+ this.keyLocation = options.keyLocation ?? false;
+ if (options.skip && options.lines) {
+ throw new Error('Cannot set skip and lines');
+ }
+ this.skip = options.skip;
+ if (options.skip) {
+ this.lineRange = {
+ left: {
+ start_line: options.offsetLeft,
+ end_line: options.offsetLeft + options.skip - 1,
+ },
+ right: {
+ start_line: options.offsetRight,
+ end_line: options.offsetRight + options.skip - 1,
+ },
+ };
+ } else {
+ for (const line of options.lines ?? []) {
+ this.addLine(line);
+ }
+ }
+ break;
+ }
+ case GrDiffGroupType.CONTEXT_CONTROL: {
+ this.contextGroups = options.contextGroups;
+ if (this.contextGroups.length > 0) {
+ const firstGroup = this.contextGroups[0];
+ const lastGroup = this.contextGroups[this.contextGroups.length - 1];
+ this.lineRange = {
+ left: {
+ start_line: firstGroup.lineRange.left.start_line,
+ end_line: lastGroup.lineRange.left.end_line,
+ },
+ right: {
+ start_line: firstGroup.lineRange.right.start_line,
+ end_line: lastGroup.lineRange.right.end_line,
+ },
+ };
+ }
+ break;
+ }
+ default:
+ throw new Error(`Unknown group type: ${this.type}`);
+ }
}
- dueToRebase = false;
+ readonly type: GrDiffGroupType;
+
+ readonly dueToRebase: boolean = false;
/**
* True means all changes in this line are whitespace changes that should
* not be highlighted as changed as per the user settings.
*/
- ignoredWhitespaceOnly = false;
+ readonly ignoredWhitespaceOnly: boolean = false;
/**
* True means it should not be collapsed (because it was in the URL, or
* there is a comment on that line)
*/
- keyLocation = false;
+ readonly keyLocation: boolean = false;
element?: HTMLElement;
- lines: GrDiffLine[] = [];
+ readonly lines: GrDiffLine[] = [];
- adds: GrDiffLine[] = [];
+ readonly adds: GrDiffLine[] = [];
- removes: GrDiffLine[] = [];
+ readonly removes: GrDiffLine[] = [];
- contextGroups: GrDiffGroup[] = [];
+ readonly contextGroups: GrDiffGroup[] = [];
- skip?: number;
+ readonly skip?: number;
/** Both start and end line are inclusive. */
- lineRange = {
- [Side.LEFT]: {start_line: 0, end_line: 0} as LineRange,
- [Side.RIGHT]: {start_line: 0, end_line: 0} as LineRange,
+ readonly lineRange: {[side in Side]: LineRange} = {
+ [Side.LEFT]: {start_line: 0, end_line: 0},
+ [Side.RIGHT]: {start_line: 0, end_line: 0},
};
- moveDetails?: {
- changed: boolean;
- range?: {
- start: number;
- end: number;
- };
- };
+ readonly moveDetails?: GrMoveDetails;
/**
* Creates a new group with the same properties but different lines.
@@ -269,13 +346,22 @@
* rendering of the old lines, so that would not make sense.
*/
cloneWithLines(lines: GrDiffLine[]): GrDiffGroup {
- const group = new GrDiffGroup(this.type, lines);
- group.dueToRebase = this.dueToRebase;
- group.ignoredWhitespaceOnly = this.ignoredWhitespaceOnly;
+ if (
+ this.type !== GrDiffGroupType.BOTH &&
+ this.type !== GrDiffGroupType.DELTA
+ ) {
+ throw new Error('Cannot clone context group with lines');
+ }
+ const group = new GrDiffGroup({
+ type: this.type,
+ lines,
+ dueToRebase: this.dueToRebase,
+ ignoredWhitespaceOnly: this.ignoredWhitespaceOnly,
+ });
return group;
}
- addLine(line: GrDiffLine) {
+ private addLine(line: GrDiffLine) {
this.lines.push(line);
const notDelta =
@@ -293,7 +379,7 @@
} else if (line.type === GrDiffLineType.REMOVE) {
this.removes.push(line);
}
- this._updateRange(line);
+ this._updateRangeWithNewLine(line);
}
getSideBySidePairs(): GrDiffLinePair[] {
@@ -323,7 +409,7 @@
return pairs;
}
- _updateRange(line: GrDiffLine) {
+ private _updateRangeWithNewLine(line: GrDiffLine) {
if (
line.beforeNumber === 'FILE' ||
line.afterNumber === 'FILE' ||
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 4c7d346..73c12bf 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
@@ -21,13 +21,12 @@
suite('gr-diff-group tests', () => {
test('delta line pairs', () => {
- let group = new GrDiffGroup(GrDiffGroupType.DELTA);
const l1 = new GrDiffLine(GrDiffLineType.ADD, 0, 128);
const l2 = new GrDiffLine(GrDiffLineType.ADD, 0, 129);
const l3 = new GrDiffLine(GrDiffLineType.REMOVE, 64, 0);
- group.addLine(l1);
- group.addLine(l2);
- group.addLine(l3);
+ let group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines: [
+ l1, l2, l3,
+ ]});
assert.deepEqual(group.lines, [l1, l2, l3]);
assert.deepEqual(group.adds, [l1, l2]);
assert.deepEqual(group.removes, [l3]);
@@ -42,7 +41,7 @@
{left: BLANK_LINE, right: l2},
]);
- group = new GrDiffGroup(GrDiffGroupType.DELTA, [l1, l2, l3]);
+ group = new GrDiffGroup({type: GrDiffGroupType.DELTA, lines: [l1, l2, l3]});
assert.deepEqual(group.lines, [l1, l2, l3]);
assert.deepEqual(group.adds, [l1, l2]);
assert.deepEqual(group.removes, [l3]);
@@ -59,7 +58,8 @@
const l2 = new GrDiffLine(GrDiffLineType.BOTH, 65, 129);
const l3 = new GrDiffLine(GrDiffLineType.BOTH, 66, 130);
- let group = new GrDiffGroup(GrDiffGroupType.BOTH, [l1, l2, l3]);
+ const group = new GrDiffGroup({
+ type: GrDiffGroupType.BOTH, lines: [l1, l2, l3]});
assert.deepEqual(group.lines, [l1, l2, l3]);
assert.deepEqual(group.adds, []);
@@ -70,19 +70,7 @@
right: {start_line: 128, end_line: 130},
});
- let pairs = group.getSideBySidePairs();
- assert.deepEqual(pairs, [
- {left: l1, right: l1},
- {left: l2, right: l2},
- {left: l3, right: l3},
- ]);
-
- group = new GrDiffGroup(GrDiffGroupType.CONTEXT_CONTROL, [l1, l2, l3]);
- assert.deepEqual(group.lines, [l1, l2, l3]);
- assert.deepEqual(group.adds, []);
- assert.deepEqual(group.removes, []);
-
- pairs = group.getSideBySidePairs();
+ const pairs = group.getSideBySidePairs();
assert.deepEqual(pairs, [
{left: l1, right: l1},
{left: l2, right: l2},
@@ -95,27 +83,20 @@
const l2 = new GrDiffLine(GrDiffLineType.REMOVE);
const l3 = new GrDiffLine(GrDiffLineType.BOTH);
- let group = new GrDiffGroup(GrDiffGroupType.BOTH);
- assert.throws(group.addLine.bind(group, l1));
- assert.throws(group.addLine.bind(group, l2));
- assert.doesNotThrow(group.addLine.bind(group, l3));
-
- group = new GrDiffGroup(GrDiffGroupType.CONTEXT_CONTROL);
- assert.throws(group.addLine.bind(group, l1));
- assert.throws(group.addLine.bind(group, l2));
- assert.doesNotThrow(group.addLine.bind(group, l3));
+ assert.throws(() =>
+ new GrDiffGroup({type: GrDiffGroupType.BOTH, lines: [l1, l2, l3]}));
});
suite('hideInContextControl', () => {
let groups;
setup(() => {
groups = [
- new GrDiffGroup(GrDiffGroupType.BOTH, [
+ new GrDiffGroup({type: GrDiffGroupType.BOTH, lines: [
new GrDiffLine(GrDiffLineType.BOTH, 5, 7),
new GrDiffLine(GrDiffLineType.BOTH, 6, 8),
new GrDiffLine(GrDiffLineType.BOTH, 7, 9),
- ]),
- new GrDiffGroup(GrDiffGroupType.DELTA, [
+ ]}),
+ new GrDiffGroup({type: GrDiffGroupType.DELTA, lines: [
new GrDiffLine(GrDiffLineType.REMOVE, 8),
new GrDiffLine(GrDiffLineType.ADD, 0, 10),
new GrDiffLine(GrDiffLineType.REMOVE, 9),
@@ -124,12 +105,12 @@
new GrDiffLine(GrDiffLineType.ADD, 0, 12),
new GrDiffLine(GrDiffLineType.REMOVE, 11),
new GrDiffLine(GrDiffLineType.ADD, 0, 13),
- ]),
- new GrDiffGroup(GrDiffGroupType.BOTH, [
+ ]}),
+ new GrDiffGroup({type: GrDiffGroupType.BOTH, lines: [
new GrDiffLine(GrDiffLineType.BOTH, 12, 14),
new GrDiffLine(GrDiffLineType.BOTH, 13, 15),
new GrDiffLine(GrDiffLineType.BOTH, 14, 16),
- ]),
+ ]}),
];
});
@@ -181,24 +162,23 @@
suite('with skip chunks', () => {
setup(() => {
- const skipGroup = new GrDiffGroup(GrDiffGroupType.BOTH);
- skipGroup.skip = 60;
- skipGroup.lineRange = {
- left: {start_line: 8, end_line: 67},
- right: {start_line: 10, end_line: 69},
- };
+ const skipGroup = new GrDiffGroup({
+ type: GrDiffGroupType.BOTH,
+ skip: 60,
+ offsetLeft: 8,
+ offsetRight: 10});
groups = [
- new GrDiffGroup(GrDiffGroupType.BOTH, [
+ new GrDiffGroup({type: GrDiffGroupType.BOTH, lines: [
new GrDiffLine(GrDiffLineType.BOTH, 5, 7),
new GrDiffLine(GrDiffLineType.BOTH, 6, 8),
new GrDiffLine(GrDiffLineType.BOTH, 7, 9),
- ]),
+ ]}),
skipGroup,
- new GrDiffGroup(GrDiffGroupType.BOTH, [
+ new GrDiffGroup({type: GrDiffGroupType.BOTH, lines: [
new GrDiffLine(GrDiffLineType.BOTH, 68, 70),
new GrDiffLine(GrDiffLineType.BOTH, 69, 71),
new GrDiffLine(GrDiffLineType.BOTH, 70, 72),
- ]),
+ ]}),
];
});
