Create diff processing options object Callers of GrDiffProcessor should not have to set public properties. Instead the parameters should be passed in a constructor or a method. This change prepares change 373057, which will move the responsibility of diff processing from GrDiff into the model. Release-Notes: skip Google-Bug-Id: b/280019334 Change-Id: I7c925bd6c46f3f4727f8de7d1d75a998de204932
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 256dc11..6346573 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
@@ -12,9 +12,9 @@ import {DiffContent} from '../../../types/diff'; import {Side} from '../../../constants/constants'; import {debounce, DelayedTask} from '../../../utils/async-util'; -import {assert, assertIsDefined} from '../../../utils/common-util'; +import {assert} from '../../../utils/common-util'; import {GrAnnotation} from '../gr-diff-highlight/gr-annotation'; -import {FILE, GrDiffLineType, LOST, LineNumber} from '../../../api/diff'; +import {FILE, GrDiffLineType, LineNumber} from '../../../api/diff'; const WHOLE_FILE = -1; @@ -56,6 +56,14 @@ clearGroups(): void; } +/** Interface for listening to the output of the processor. */ +export interface ProcessingOptions { + context: number; + keyLocations?: KeyLocations; + asyncThreshold?: number; + isBinary?: boolean; +} + /** * Converts the API's `DiffContent`s to `GrDiffGroup`s for rendering. * @@ -82,13 +90,15 @@ * the rest is not. */ export class GrDiffProcessor { - context = 3; + // visible for testing + context: number; - consumer?: GroupConsumer; + // visible for testing + keyLocations: KeyLocations; - keyLocations: KeyLocations = {left: {}, right: {}}; + private asyncThreshold: number; - asyncThreshold = 64; + private isBinary: boolean; // visible for testing isScrolling?: boolean; @@ -101,6 +111,17 @@ private resetIsScrollingTask?: DelayedTask; + constructor( + private consumer: GroupConsumer | undefined, + options: ProcessingOptions + ) { + this.consumer = consumer; + this.context = options.context; + this.asyncThreshold = options.asyncThreshold ?? 64; + this.keyLocations = options.keyLocations ?? {left: {}, right: {}}; + this.isBinary = options.isBinary ?? false; + } + private readonly handleWindowScroll = () => { this.isScrolling = true; this.resetIsScrollingTask = debounce( @@ -117,18 +138,16 @@ * @return A promise that resolves with an * array of GrDiffGroups when the diff is completely processed. */ - process(chunks: DiffContent[], isBinary: boolean) { + process(chunks: DiffContent[]) { assert(this.isStarted === false, 'diff processor cannot be started twice'); - this.isStarted = true; window.addEventListener('scroll', this.handleWindowScroll); - assertIsDefined(this.consumer, 'consumer'); - this.consumer.clearGroups(); - this.consumer.addGroup(this.makeGroup(LOST)); - this.consumer.addGroup(this.makeGroup(FILE)); + this.consumer?.clearGroups(); + this.consumer?.addGroup(this.makeGroup('LOST')); + this.consumer?.addGroup(this.makeGroup(FILE)); - if (isBinary) return Promise.resolve(); + if (this.isBinary) return Promise.resolve(); return new Promise<void>(resolve => { const state = {
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 335f0d0..706c208 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
@@ -7,7 +7,12 @@ import './gr-diff-processor'; import {GrDiffLine} from '../gr-diff/gr-diff-line'; import {GrDiffGroup, GrDiffGroupType} from '../gr-diff/gr-diff-group'; -import {GrDiffProcessor, State} from './gr-diff-processor'; +import { + GrDiffProcessor, + GroupConsumer, + ProcessingOptions, + State, +} from './gr-diff-processor'; import {DiffContent} from '../../../types/diff'; import {assert} from '@open-wc/testing'; import {FILE, GrDiffLineType} from '../../../api/diff'; @@ -21,24 +26,27 @@ 'Eos cu aliquam labores qualisque, usu postea inermis te, et solum ' + 'fugit assum per.'; - let element: GrDiffProcessor; + let processor: GrDiffProcessor; + let options: ProcessingOptions = { + context: 4, + }; let groups: GrDiffGroup[]; + const consumer: GroupConsumer = { + addGroup(group: GrDiffGroup) { + groups.push(group); + }, + clearGroups() { + groups = []; + }, + }; setup(() => {}); suite('not logged in', () => { setup(() => { groups = []; - element = new GrDiffProcessor(); - element.consumer = { - addGroup(group: GrDiffGroup) { - groups.push(group); - }, - clearGroups() { - groups = []; - }, - }; - element.context = 4; + options = {context: 4}; + processor = new GrDiffProcessor(consumer, options); }); test('process loaded content', () => { @@ -59,7 +67,7 @@ }, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup assert.equal(groups.length, 4); @@ -120,7 +128,7 @@ test('first group is for file', () => { const content = [{b: ['foo']}]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup assert.equal(groups[0].type, GrDiffGroupType.BOTH); @@ -133,7 +141,8 @@ suite('context groups', () => { test('at the beginning, larger than context', () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ { ab: Array.from<string>({length: 100}).fill( @@ -143,28 +152,28 @@ {a: ['all work and no play make andybons a dull boy']}, ]; - return element.process(content, false).then(() => { - groups.shift(); // remove portedThreadsWithoutRangeGroup + return processor.process(content).then(() => { + // group[0] is the LOST group + // group[1] is the FILE group - // group[0] is the file group - - assert.equal(groups[1].type, GrDiffGroupType.CONTEXT_CONTROL); - assert.instanceOf(groups[1].contextGroups[0], GrDiffGroup); - assert.equal(groups[1].contextGroups[0].lines.length, 90); - for (const l of groups[1].contextGroups[0].lines) { + assert.equal(groups[2].type, GrDiffGroupType.CONTEXT_CONTROL); + assert.instanceOf(groups[2].contextGroups[0], GrDiffGroup); + assert.equal(groups[2].contextGroups[0].lines.length, 90); + for (const l of groups[2].contextGroups[0].lines) { assert.equal(l.text, 'all work and no play make jack a dull boy'); } - assert.equal(groups[2].type, GrDiffGroupType.BOTH); - assert.equal(groups[2].lines.length, 10); - for (const l of groups[2].lines) { + assert.equal(groups[3].type, GrDiffGroupType.BOTH); + assert.equal(groups[3].lines.length, 10); + for (const l of groups[3].lines) { assert.equal(l.text, 'all work and no play make jack a dull boy'); } }); }); test('at the beginning with skip chunks', async () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ { ab: Array.from<string>({length: 20}).fill( @@ -176,7 +185,7 @@ {a: ['some other content']}, ]; - await element.process(content, false); + await processor.process(content); groups.shift(); // remove portedThreadsWithoutRangeGroup @@ -216,7 +225,8 @@ }); test('at the beginning, smaller than context', () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ { ab: Array.from<string>({length: 5}).fill( @@ -226,7 +236,7 @@ {a: ['all work and no play make andybons a dull boy']}, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup // group[0] is the file group @@ -240,7 +250,8 @@ }); test('at the end, larger than context', () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ {a: ['all work and no play make andybons a dull boy']}, { @@ -250,7 +261,7 @@ }, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup // group[0] is the file group @@ -272,7 +283,7 @@ }); test('at the end, smaller than context', () => { - element.context = 10; + options.context = 10; const content = [ {a: ['all work and no play make andybons a dull boy']}, { @@ -282,7 +293,7 @@ }, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup // group[0] is the file group @@ -297,7 +308,8 @@ }); test('for interleaved ab and common: true chunks', () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ {a: ['all work and no play make andybons a dull boy']}, { @@ -335,7 +347,7 @@ }, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup // group[0] is the file group @@ -412,7 +424,8 @@ }); test('in the middle, larger than context', () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ {a: ['all work and no play make andybons a dull boy']}, { @@ -423,7 +436,7 @@ {a: ['all work and no play make andybons a dull boy']}, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup // group[0] is the file group @@ -451,7 +464,8 @@ }); test('in the middle, smaller than context', () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ {a: ['all work and no play make andybons a dull boy']}, { @@ -462,7 +476,7 @@ {a: ['all work and no play make andybons a dull boy']}, ]; - return element.process(content, false).then(() => { + return processor.process(content).then(() => { groups.shift(); // remove portedThreadsWithoutRangeGroup // group[0] is the file group @@ -478,7 +492,8 @@ }); test('in the middle with skip chunks', async () => { - element.context = 10; + options.context = 10; + processor = new GrDiffProcessor(consumer, options); const content = [ {a: ['all work and no play make andybons a dull boy']}, { @@ -495,7 +510,7 @@ {a: ['all work and no play make andybons a dull boy']}, ]; - await element.process(content, false); + await processor.process(content); groups.shift(); // remove portedThreadsWithoutRangeGroup @@ -531,7 +546,8 @@ }); test('works with skip === 0', async () => { - element.context = 3; + options.context = 3; + processor = new GrDiffProcessor(consumer, options); const content = [ { skip: 0, @@ -547,14 +563,15 @@ ], }, ]; - await element.process(content, false); + await processor.process(content); }); test('break up common diff chunks', () => { - element.keyLocations = { + options.keyLocations = { left: {1: true}, right: {10: true}, }; + processor = new GrDiffProcessor(consumer, options); const content = [ { @@ -575,7 +592,7 @@ ], }, ]; - const result = element.splitCommonChunksWithKeyLocations(content); + const result = processor.splitCommonChunksWithKeyLocations(content); assert.deepEqual(result, [ { ab: ['copy'], @@ -603,8 +620,8 @@ .fill(0) .map(() => `${Math.random()}`); const content = [{ab}]; - element.context = -1; - const result = element.splitLargeChunks(content); + processor.context = -1; + const result = processor.splitLargeChunks(content); assert.equal(result.length, 2); assert.deepEqual(result[0].ab, content[0].ab.slice(0, maxGroupSize)); assert.deepEqual(result[1].ab, content[0].ab.slice(maxGroupSize)); @@ -616,8 +633,8 @@ const content = Array(size) .fill(0) .map(() => `${Math.random()}`); - element.context = 5; - const splitContent = element + processor.context = 5; + const splitContent = processor .splitLargeChunks([{a: [], b: content}]) .map(r => r.b); assert.equal(splitContent.length, 3); @@ -632,8 +649,8 @@ const content = Array(size) .fill(0) .map(() => `${Math.random()}`); - element.context = 5; - const splitContent = element + processor.context = 5; + const splitContent = processor .splitLargeChunks([{a: content, b: []}]) .map(r => r.a); assert.equal(splitContent.length, 3); @@ -647,8 +664,8 @@ const content = Array(size) .fill(0) .map(() => `${Math.random()}`); - element.context = 5; - const splitContent = element + processor.context = 5; + const splitContent = processor .splitLargeChunks([ { a: content, @@ -666,8 +683,8 @@ .fill(0) .map(() => `${Math.random()}`); const content = [{ab}]; - element.context = 4; - const result = element.splitCommonChunksWithKeyLocations(content); + processor.context = 4; + const result = processor.splitCommonChunksWithKeyLocations(content); assert.equal(result.length, 1); assert.deepEqual(result[0].ab, content[0].ab); assert.isFalse(result[0].keyLocation); @@ -687,7 +704,7 @@ [42, 26], ]; - let results = element.convertIntralineInfos(content, highlights); + let results = processor.convertIntralineInfos(content, highlights); assert.deepEqual(results, [ { contentIndex: 0, @@ -704,7 +721,7 @@ startIndex: 75, }, ]); - const lines = element.linesFromRows( + const lines = processor.linesFromRows( GrDiffLineType.BOTH, content, 0, @@ -736,7 +753,7 @@ [12, 67], [14, 29], ]; - results = element.convertIntralineInfos(content, highlights); + results = processor.convertIntralineInfos(content, highlights); assert.deepEqual(results, [ { contentIndex: 0, @@ -767,7 +784,7 @@ content = ['🙈 a', '🙉 b', '🙊 c']; highlights = [[2, 7]]; - results = element.convertIntralineInfos(content, highlights); + results = processor.convertIntralineInfos(content, highlights); assert.deepEqual(results, [ { contentIndex: 0, @@ -787,23 +804,25 @@ test('isScrolling paused', () => { const content = Array(200).fill({ab: ['', '']}); - element.isScrolling = true; - element.process(content, false); + processor.isScrolling = true; + processor.process(content); // Just the FILE and LOST groups. assert.equal(groups.length, 2); }); test('isScrolling unpaused', () => { const content = Array(200).fill({ab: ['', '']}); - element.isScrolling = false; - element.process(content, false); + processor.isScrolling = false; + processor.process(content); // More groups have been processed. How many does not matter here. assert.isAtLeast(groups.length, 3); }); test('image diffs', () => { const content = Array(200).fill({ab: ['', '']}); - element.process(content, true); + options.isBinary = true; + processor = new GrDiffProcessor(consumer, options); + processor.process(content); assert.equal(groups.length, 2); // Image diffs don't process content, just the 'FILE' line. @@ -818,13 +837,13 @@ }); test('WHOLE_FILE', () => { - element.context = WHOLE_FILE; + processor.context = WHOLE_FILE; const state: State = { lineNums: {left: 10, right: 100}, chunkIndex: 1, }; const chunks = [{a: ['foo']}, {ab: rows}, {a: ['bar']}]; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // Results in one, uncollapsed group with all rows. assert.equal(result.groups.length, 1); @@ -852,7 +871,7 @@ }); test('WHOLE_FILE with skip chunks still get collapsed', () => { - element.context = WHOLE_FILE; + processor.context = WHOLE_FILE; const lineNums = {left: 10, right: 100}; const state = { lineNums, @@ -860,7 +879,7 @@ }; const skip = 10000; const chunks = [{a: ['foo']}, {skip}, {ab: rows}, {a: ['bar']}]; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // Results in one, uncollapsed group with all rows. assert.equal(result.groups.length, 1); assert.equal(result.groups[0].type, GrDiffGroupType.CONTEXT_CONTROL); @@ -894,21 +913,21 @@ }); test('with context', () => { - element.context = 10; + processor.context = 10; const state = { lineNums: {left: 10, right: 100}, chunkIndex: 1, }; const chunks = [{a: ['foo']}, {ab: rows}, {a: ['bar']}]; - const result = element.processNext(state, chunks); - const expectedCollapseSize = rows.length - 2 * element.context; + const result = processor.processNext(state, chunks); + const expectedCollapseSize = rows.length - 2 * processor.context; assert.equal(result.groups.length, 3, 'Results in three groups'); // The first and last are uncollapsed context, whereas the middle has // a single context-control line. - assert.equal(result.groups[0].lines.length, element.context); - assert.equal(result.groups[2].lines.length, element.context); + assert.equal(result.groups[0].lines.length, processor.context); + assert.equal(result.groups[2].lines.length, processor.context); // The collapsed group has the hidden lines as its context group. assert.equal( @@ -918,19 +937,19 @@ }); test('first', () => { - element.context = 10; + processor.context = 10; const state = { lineNums: {left: 10, right: 100}, chunkIndex: 0, }; const chunks = [{ab: rows}, {a: ['foo']}, {a: ['bar']}]; - const result = element.processNext(state, chunks); - const expectedCollapseSize = rows.length - element.context; + const result = processor.processNext(state, chunks); + const expectedCollapseSize = rows.length - processor.context; assert.equal(result.groups.length, 2, 'Results in two groups'); // Only the first group is collapsed. - assert.equal(result.groups[1].lines.length, element.context); + assert.equal(result.groups[1].lines.length, processor.context); // The collapsed group has the hidden lines as its context group. assert.equal( @@ -942,13 +961,13 @@ test('few-rows', () => { // Only ten rows. rows = rows.slice(0, 10); - element.context = 10; + processor.context = 10; const state = { lineNums: {left: 10, right: 100}, chunkIndex: 0, }; const chunks = [{ab: rows}, {a: ['foo']}, {a: ['bar']}]; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // Results in one uncollapsed group with all rows. assert.equal(result.groups.length, 1, 'Results in one group'); @@ -957,13 +976,13 @@ test('no single line collapse', () => { rows = rows.slice(0, 7); - element.context = 3; + processor.context = 3; const state = { lineNums: {left: 10, right: 100}, chunkIndex: 1, }; const chunks = [{a: ['foo']}, {ab: rows}, {a: ['bar']}]; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // Results in one uncollapsed group with all rows. assert.equal(result.groups.length, 1, 'Results in one group'); @@ -979,13 +998,13 @@ lineNums: {left: 10, right: 100}, chunkIndex: 0, }; - element.context = 10; + processor.context = 10; chunks = [{ab: rows}, {ab: ['foo'], keyLocation: true}, {ab: rows}]; }); test('context before', () => { state.chunkIndex = 0; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // The first chunk is split into two groups: // 1) A context-control, hiding everything but the context before @@ -996,14 +1015,14 @@ // The collapsed group has the hidden lines as its context group. assert.equal( result.groups[0].contextGroups[0].lines.length, - rows.length - element.context + rows.length - processor.context ); - assert.equal(result.groups[1].lines.length, element.context); + assert.equal(result.groups[1].lines.length, processor.context); }); test('key location itself', () => { state.chunkIndex = 1; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // The second chunk results in a single group, that is just the // line with the key location @@ -1015,18 +1034,18 @@ test('context after', () => { state.chunkIndex = 2; - const result = element.processNext(state, chunks); + const result = processor.processNext(state, chunks); // The last chunk is split into two groups: // 1) The context after the key location. // 1) A context-control, hiding everything but the context after the // key location. assert.equal(result.groups.length, 2); - assert.equal(result.groups[0].lines.length, element.context); + assert.equal(result.groups[0].lines.length, processor.context); // The collapsed group has the hidden lines as its context group. assert.equal( result.groups[1].contextGroups[0].lines.length, - rows.length - element.context + rows.length - processor.context ); }); }); @@ -1041,7 +1060,7 @@ test('linesFromRows', () => { const startLineNum = 10; - let result = element.linesFromRows( + let result = processor.linesFromRows( GrDiffLineType.ADD, rows, startLineNum + 1 @@ -1058,7 +1077,7 @@ ); assert.notOk(result[result.length - 1].beforeNumber); - result = element.linesFromRows( + result = processor.linesFromRows( GrDiffLineType.REMOVE, rows, startLineNum + 1 @@ -1079,17 +1098,17 @@ suite('breakdown*', () => { test('breakdownChunk breaks down additions', () => { - const breakdownSpy = sinon.spy(element, 'breakdown'); + const breakdownSpy = sinon.spy(processor, 'breakdown'); const chunk = {b: ['blah', 'blah', 'blah']}; - const result = element.breakdownChunk(chunk); + const result = processor.breakdownChunk(chunk); assert.deepEqual(result, [chunk]); assert.isTrue(breakdownSpy.called); }); test('breakdownChunk keeps due_to_rebase for broken down additions', () => { - sinon.spy(element, 'breakdown'); + sinon.spy(processor, 'breakdown'); const chunk = {b: ['blah', 'blah', 'blah'], due_to_rebase: true}; - const result = element.breakdownChunk(chunk); + const result = processor.breakdownChunk(chunk); for (const subResult of result) { assert.isTrue(subResult.due_to_rebase); } @@ -1101,7 +1120,7 @@ ); const size = 3; - const result = element.breakdown(array, size); + const result = processor.breakdown(array, size); for (const subResult of result) { assert.isAtMost(subResult.length, size); @@ -1117,7 +1136,7 @@ const size = 10; const expected = [array]; - const result = element.breakdown(array, size); + const result = processor.breakdown(array, size); assert.deepEqual(result, expected); }); @@ -1127,7 +1146,7 @@ const size = 10; const expected: string[][] = []; - const result = element.breakdown(array, size); + const result = processor.breakdown(array, size); assert.deepEqual(result, expected); });
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts index d87d4d8..32ffc73 100644 --- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts +++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -45,6 +45,7 @@ import { GrDiffProcessor, KeyLocations, + ProcessingOptions, } from '../gr-diff-processor/gr-diff-processor'; import {fire, fireAlert} from '../../../utils/event-util'; import {MovedLinkClickedEvent, ValueChangedEvent} from '../../../types/events'; @@ -1153,26 +1154,23 @@ this.builder = this.getDiffBuilder(); this.diffBuilderInit(); - // TODO: Just pass along the diff model here instead of setting many - // individual properties. - this.processor = new GrDiffProcessor(); - this.processor.consumer = this; - this.processor.context = this.getBypassPrefs().context; - this.processor.keyLocations = this.computeKeyLocations(); - if (this.renderPrefs?.num_lines_rendered_at_once) { - this.processor.asyncThreshold = - this.renderPrefs.num_lines_rendered_at_once; - } - this.diffTable.innerHTML = ''; this.builder.addColumns(this.diffTable, getLineNumberCellWidth(this.prefs)); - const isBinary = !!(this.isImageDiff || this.diff.binary); + const options: ProcessingOptions = { + context: this.getBypassPrefs().context, + keyLocations: this.computeKeyLocations(), + isBinary: !!(this.isImageDiff || this.diff.binary), + }; + if (this.renderPrefs?.num_lines_rendered_at_once) { + options.asyncThreshold = this.renderPrefs.num_lines_rendered_at_once; + } + this.processor = new GrDiffProcessor(this, options); fire(this.diffTable, 'render-start', {}); return ( this.processor - .process(this.diff.content, isBinary) + .process(this.diff.content) .then(async () => { if (isImageDiffBuilder(this.builder)) { this.builder.renderImageDiff();