Merge "Move 650 lines of gr-diff css into its own file" into stable-3.8
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 396e9e2..328b577 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -14,7 +14,6 @@
} from './gr-diff-builder';
import {GrDiffBuilderImage} from './gr-diff-builder-image';
import {GrDiffBuilderBinary} from './gr-diff-builder-binary';
-import {CancelablePromise, makeCancelable} from '../../../utils/async-util';
import {BlameInfo, ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {CoverageRange, DiffLayer} from '../../../types/types';
@@ -130,13 +129,6 @@
// visible for testing
showTrailingWhitespace?: boolean;
- /**
- * The promise last returned from `render()` while the asynchronous
- * rendering is running - `null` otherwise. Provides a `cancel()`
- * method that rejects it with `{isCancelled: true}`.
- */
- private cancelableRenderPromise: CancelablePromise<unknown> | null = null;
-
private coverageLayerLeft = new GrCoverageLayer(Side.LEFT);
private coverageLayerRight = new GrCoverageLayer(Side.RIGHT);
@@ -144,7 +136,7 @@
private rangeLayer?: GrRangedCommentLayer;
// visible for testing
- processor = new GrDiffProcessor();
+ processor?: GrDiffProcessor;
/**
* Groups are mostly just passed on to the diff builder (this.builder). But
@@ -156,10 +148,6 @@
*/
private groups: GrDiffGroup[] = [];
- constructor() {
- this.processor.consumer = this;
- }
-
updateCommentRanges(ranges: CommentRangeLayer[]) {
this.rangeLayer?.updateRanges(ranges);
}
@@ -186,8 +174,16 @@
this.builder = this.getDiffBuilder();
this.init();
+ // 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.prefs.context;
this.processor.keyLocations = keyLocations;
+ if (this.renderPrefs?.num_lines_rendered_at_once) {
+ this.processor.asyncThreshold =
+ this.renderPrefs.num_lines_rendered_at_once;
+ }
this.clearDiffContent();
this.builder.addColumns(
@@ -198,14 +194,9 @@
const isBinary = !!(this.isImageDiff || this.diff.binary);
fire(this.diffElement, 'render-start', {});
- // TODO: processor.process() returns a cancelable promise already.
- // Why wrap another one around it?
- this.cancelableRenderPromise = makeCancelable(
- this.processor.process(this.diff.content, isBinary)
- );
- // All then/catch/finally clauses must be outside of makeCancelable().
return (
- this.cancelableRenderPromise
+ this.processor
+ .process(this.diff.content, isBinary)
.then(async () => {
if (isImageDiffBuilder(this.builder)) {
this.builder.renderImageDiff();
@@ -222,9 +213,6 @@
if (!e.isCanceled) return Promise.reject(e);
return;
})
- .finally(() => {
- this.cancelableRenderPromise = null;
- })
);
}
@@ -376,10 +364,8 @@
* gr-diff re-connects.
*/
cleanup() {
- this.processor.cancel();
+ this.processor?.cancel();
this.builder?.cleanup();
- this.cancelableRenderPromise?.cancel();
- this.cancelableRenderPromise = null;
this.diffElement?.removeEventListener(
'diff-context-expanded',
this.onDiffContextExpanded
@@ -584,6 +570,5 @@
updateRenderPrefs(renderPrefs: RenderPreferences) {
this.builder?.updateRenderPrefs(renderPrefs);
- this.processor.updateRenderPrefs(renderPrefs);
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index d776164..da2e9f1 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -27,6 +27,7 @@
import {fixture, html, assert} from '@open-wc/testing';
import {GrDiffRow} from './gr-diff-row';
import {GrDiffBuilder} from './gr-diff-builder';
+import {querySelectorAll} from '../../../utils/dom-util';
const DEFAULT_PREFS = createDefaultDiffPrefs();
@@ -470,15 +471,11 @@
});
suite('rendering text, images and binary files', () => {
- let processStub: sinon.SinonStub;
let keyLocations: KeyLocations;
let content: DiffContent[] = [];
setup(() => {
element.viewMode = 'SIDE_BY_SIDE';
- processStub = sinon
- .stub(element.processor, 'process')
- .returns(Promise.resolve());
keyLocations = {left: {}, right: {}};
element.prefs = {
...DEFAULT_PREFS,
@@ -503,8 +500,7 @@
element.diff = {...createEmptyDiff(), content};
element.render(keyLocations);
await waitForEventOnce(diffTable, 'render-content');
- assert.isTrue(processStub.calledOnce);
- assert.isFalse(processStub.lastCall.args[1]);
+ assert.equal(querySelectorAll(diffTable, 'tbody')?.length, 4);
});
test('image', async () => {
@@ -512,16 +508,14 @@
element.isImageDiff = true;
element.render(keyLocations);
await waitForEventOnce(diffTable, 'render-content');
- assert.isTrue(processStub.calledOnce);
- assert.isTrue(processStub.lastCall.args[1]);
+ assert.equal(querySelectorAll(diffTable, 'tbody')?.length, 4);
});
test('binary', async () => {
element.diff = {...createEmptyDiff(), content, binary: true};
element.render(keyLocations);
await waitForEventOnce(diffTable, 'render-content');
- assert.isTrue(processStub.calledOnce);
- assert.isTrue(processStub.lastCall.args[1]);
+ assert.equal(querySelectorAll(diffTable, 'tbody')?.length, 3);
});
});
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 22a71a5..05e5d3b 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
@@ -15,12 +15,10 @@
GrDiffGroupType,
hideInContextControl,
} from '../gr-diff/gr-diff-group';
-import {CancelablePromise, makeCancelable} from '../../../utils/async-util';
import {DiffContent} from '../../../types/diff';
import {Side} from '../../../constants/constants';
import {debounce, DelayedTask} from '../../../utils/async-util';
-import {RenderPreferences} from '../../../api/diff';
-import {assertIsDefined} from '../../../utils/common-util';
+import {assert, assertIsDefined} from '../../../utils/common-util';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
const WHOLE_FILE = -1;
@@ -95,15 +93,17 @@
keyLocations: KeyLocations = {left: {}, right: {}};
- private asyncThreshold = 64;
-
- private nextStepHandle: number | null = null;
-
- private processPromise: CancelablePromise<void> | null = null;
+ asyncThreshold = 64;
// visible for testing
isScrolling?: boolean;
+ /** Just for making sure that process() is only called once. */
+ private isStarted = false;
+
+ /** Indicates that processing should be stopped. */
+ private isCancelled = false;
+
private resetIsScrollingTask?: DelayedTask;
private readonly handleWindowScroll = () => {
@@ -123,9 +123,9 @@
* array of GrDiffGroups when the diff is completely processed.
*/
process(chunks: DiffContent[], isBinary: boolean) {
- // Cancel any still running process() calls, because they append to the
- // same groups field.
- this.cancel();
+ assert(this.isStarted === false, 'diff processor cannot be started twice');
+ this.isStarted = true;
+
window.addEventListener('scroll', this.handleWindowScroll);
assertIsDefined(this.consumer, 'consumer');
@@ -133,84 +133,61 @@
this.consumer.addGroup(this.makeGroup('LOST'));
this.consumer.addGroup(this.makeGroup(FILE));
- // If it's a binary diff, we won't be rendering hunks of text differences
- // so finish processing.
- if (isBinary) {
- return Promise.resolve();
- }
+ if (isBinary) return Promise.resolve();
- // TODO: Canceling this promise does not help much. `nextStep` will continue
- // to be scheduled anyway. So either just remove the cancelable promise, so
- // future programmers are not fooled about this promise can do. Or fix the
- // scheduling of `nextStep` such that cancellation is taken into account.
- // The easiest approach is likely to just not re-use the processor for
- // multiple processing passes. There is no benefit from doing so.
- this.processPromise = makeCancelable(
- new Promise(resolve => {
- const state = {
- lineNums: {left: 0, right: 0},
- chunkIndex: 0,
- };
+ return new Promise<void>(resolve => {
+ const state = {
+ lineNums: {left: 0, right: 0},
+ chunkIndex: 0,
+ };
- chunks = this.splitLargeChunks(chunks);
- chunks = this.splitCommonChunksWithKeyLocations(chunks);
+ chunks = this.splitLargeChunks(chunks);
+ chunks = this.splitCommonChunksWithKeyLocations(chunks);
- let currentBatch = 0;
- const nextStep = () => {
- if (this.isScrolling) {
- this.nextStepHandle = window.setTimeout(nextStep, 100);
- return;
- }
- // If we are done, resolve the promise.
- if (state.chunkIndex >= chunks.length) {
- resolve();
- this.nextStepHandle = null;
- return;
- }
+ let currentBatch = 0;
+ const nextStep = () => {
+ if (this.isCancelled || state.chunkIndex >= chunks.length) {
+ resolve();
+ return;
+ }
+ if (this.isScrolling) {
+ window.setTimeout(nextStep, 100);
+ return;
+ }
- // Process the next chunk and incorporate the result.
- const stateUpdate = this.processNext(state, chunks);
- for (const group of stateUpdate.groups) {
- assertIsDefined(this.consumer, 'consumer');
- this.consumer.addGroup(group);
- currentBatch += group.lines.length;
- }
- state.lineNums.left += stateUpdate.lineDelta.left;
- state.lineNums.right += stateUpdate.lineDelta.right;
+ const stateUpdate = this.processNext(state, chunks);
+ for (const group of stateUpdate.groups) {
+ this.consumer?.addGroup(group);
+ currentBatch += group.lines.length;
+ }
+ state.lineNums.left += stateUpdate.lineDelta.left;
+ state.lineNums.right += stateUpdate.lineDelta.right;
- // Increment the index and recurse.
- state.chunkIndex = stateUpdate.newChunkIndex;
- if (currentBatch >= this.asyncThreshold) {
- currentBatch = 0;
- this.nextStepHandle = window.setTimeout(nextStep, 1);
- } else {
- nextStep.call(this);
- }
- };
+ state.chunkIndex = stateUpdate.newChunkIndex;
+ if (currentBatch >= this.asyncThreshold) {
+ currentBatch = 0;
+ window.setTimeout(nextStep, 1);
+ } else {
+ nextStep.call(this);
+ }
+ };
- nextStep.call(this);
- })
- );
- return this.processPromise.finally(() => {
- this.processPromise = null;
- window.removeEventListener('scroll', this.handleWindowScroll);
+ nextStep.call(this);
+ }).finally(() => {
+ this.finish();
});
}
- /**
- * Cancel any jobs that are running.
- */
- cancel() {
- if (this.nextStepHandle !== null) {
- window.clearTimeout(this.nextStepHandle);
- this.nextStepHandle = null;
- }
- if (this.processPromise) {
- this.processPromise.cancel();
- }
+ finish() {
+ this.consumer = undefined;
window.removeEventListener('scroll', this.handleWindowScroll);
}
+ cancel() {
+ this.isCancelled = true;
+ this.finish();
+ }
+
/**
* Process the next uncollapsible chunk, or the next collapsible chunks.
*/
@@ -739,10 +716,4 @@
return this.breakdown(head, size).concat([tail]);
}
-
- updateRenderPrefs(renderPrefs: RenderPreferences) {
- if (renderPrefs.num_lines_rendered_at_once) {
- this.asyncThreshold = renderPrefs.num_lines_rendered_at_once;
- }
- }
}
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 5b23ee7..adcfff8 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
@@ -784,13 +784,16 @@
]);
});
- test('scrolling pauses rendering', () => {
+ test('isScrolling paused', () => {
const content = Array(200).fill({ab: ['', '']});
element.isScrolling = true;
element.process(content, false);
- // Just the files group - no more processing during scrolling.
+ // 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);
// More groups have been processed. How many does not matter here.
