Merge "Fix events emitted by gr-diff-builder-element"
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts
index 4322c64..430e770 100644
--- a/polygerrit-ui/app/api/diff.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -340,6 +340,7 @@
lineNum: LineNumber;
}
+// TODO: Currently unused and not fired.
export declare interface RenderProgressEventDetail {
linesRendered: number;
}
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 d200c75..d2704ac 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
@@ -26,11 +26,7 @@
GrRangedCommentLayer,
} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
import {GrCoverageLayer} from '../gr-coverage-layer/gr-coverage-layer';
-import {
- DiffViewMode,
- RenderPreferences,
- RenderProgressEventDetail,
-} from '../../../api/diff';
+import {DiffViewMode, RenderPreferences} from '../../../api/diff';
import {createDefaultDiffPrefs, Side} from '../../../constants/constants';
import {GrDiffLine, LineNumber} from '../gr-diff/gr-diff-line';
import {
@@ -39,13 +35,9 @@
hideInContextControl,
} from '../gr-diff/gr-diff-group';
import {getLineNumber, getSideByLineEl} from '../gr-diff/gr-diff-utils';
-import {
- fireAlert,
- fire,
- HTMLElementEventDetailType,
-} from '../../../utils/event-util';
+import {fireAlert, fireEvent} from '../../../utils/event-util';
import {assertIsDefined} from '../../../utils/common-util';
-import {afterNextRender} from '../../../utils/dom-util';
+import {untilRendered} from '../../../utils/dom-util';
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
const COMMIT_MSG_PATH = '/COMMIT_MSG';
@@ -59,11 +51,6 @@
*/
'render-start': CustomEvent<{}>;
/**
- * Fired whenever a new chunk of lines has been rendered synchronously - this
- * only happens for full renders.
- */
- 'render-progress': CustomEvent<RenderProgressEventDetail>;
- /**
* Fired when the diff finishes rendering text content - both for full
* renders and for partial rerenders.
*/
@@ -163,6 +150,16 @@
// visible for testing
processor = new GrDiffProcessor();
+ /**
+ * Groups are mostly just passed on to the diff builder (this.builder). But
+ * we also keep track of them here for being able to fire a `render-content`
+ * event when .element of each group has rendered.
+ *
+ * TODO: Refactor DiffBuilderElement and DiffBuilders with a cleaner
+ * separation of responsibilities.
+ */
+ private groups: GrDiffGroup[] = [];
+
constructor() {
this.processor.consumer = this;
}
@@ -210,7 +207,7 @@
const isBinary = !!(this.isImageDiff || this.diff.binary);
- this.fireDiffEvent('render-start', {});
+ this.fireDiffEvent('render-start');
this.cancelableRenderPromise = makeCancelable(
this.processor
.process(this.diff.content, isBinary)
@@ -218,7 +215,10 @@
if (this.isImageDiff) {
(this.builder as GrDiffBuilderImage).renderDiff();
}
- afterNextRender(() => this.fireDiffEvent('render-content', {}));
+ return this.untilGroupsRendered();
+ })
+ .then(() => {
+ this.fireDiffEvent('render-content');
})
// Mocha testing does not like uncaught rejections, so we catch
// the cancels which are expected and should not throw errors in
@@ -233,6 +233,18 @@
);
}
+ // visible for testing
+ async untilGroupsRendered(groups: readonly GrDiffGroup[] = this.groups) {
+ for (const g of groups) {
+ // The LOST or FILE lines may be hidden and thus never resolve an
+ // untilRendered() promise.
+ const lineNumber = g.lines?.[0]?.beforeNumber;
+ if (g.skip || lineNumber === 'LOST' || lineNumber === 'FILE') continue;
+ assertIsDefined(g.element);
+ await untilRendered(g.element);
+ }
+ }
+
private onDiffContextExpanded = (
e: CustomEvent<DiffContextExpandedEventDetail>
) => {
@@ -241,17 +253,9 @@
this.replaceGroup(e.detail.contextGroup, e.detail.groups);
};
- private fireDiffEvent<K extends keyof HTMLElementEventMap>(
- type: K,
- detail: HTMLElementEventDetailType<K>
- ) {
+ private fireDiffEvent<K extends keyof HTMLElementEventMap>(type: K) {
assertIsDefined(this.diffElement, 'diff table');
- fire(this.diffElement, type, detail);
- }
-
- private fireDiffEventRenderProgress(detail: RenderProgressEventDetail) {
- assertIsDefined(this.diffElement, 'diff table');
- fire(this.diffElement, 'render-progress', detail);
+ fireEvent(this.diffElement, type);
}
// visible for testing
@@ -364,15 +368,12 @@
newGroups: readonly GrDiffGroup[]
) {
if (!this.builder) return;
- this.fireDiffEvent('render-start', {});
- const linesRendered = newGroups.reduce(
- (sum, group) => sum + group.lines.length,
- 0
- );
+ this.fireDiffEvent('render-start');
this.builder.replaceGroup(contextGroup, newGroups);
- afterNextRender(() => {
- this.fireDiffEvent('render-progress', {linesRendered});
- this.fireDiffEvent('render-content', {});
+ this.groups = this.groups.filter(g => g !== contextGroup);
+ this.groups.push(...newGroups);
+ this.untilGroupsRendered(newGroups).then(() => {
+ this.fireDiffEvent('render-content');
});
}
@@ -464,6 +465,7 @@
*/
clearGroups() {
if (!this.builder) return;
+ this.groups = [];
this.builder.clearGroups();
}
@@ -473,9 +475,7 @@
addGroup(group: GrDiffGroup) {
if (!this.builder) return;
this.builder.addGroups([group]);
- afterNextRender(() =>
- this.fireDiffEventRenderProgress({linesRendered: group.lines.length})
- );
+ this.groups.push(group);
}
// visible for testing
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 8ae08f4..d37afc0 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
@@ -10,11 +10,7 @@
createEmptyDiff,
} from '../../../test/test-data-generators';
import './gr-diff-builder-element';
-import {
- nextRender,
- queryAndAssert,
- stubBaseUrl,
-} from '../../../test/test-utils';
+import {queryAndAssert, stubBaseUrl, waitUntil} from '../../../test/test-utils';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {GrDiffLine, GrDiffLineType} from '../gr-diff/gr-diff-line';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
@@ -601,6 +597,7 @@
addColumnsStub = sinon.stub(builder, 'addColumns');
builder.buildSectionElement = function (group) {
const section = document.createElement('stub');
+ section.style.display = 'block';
section.textContent = group.lines.reduce(
(acc, line) => acc + line.text,
''
@@ -639,12 +636,11 @@
});
test('render-start and render-content are fired', async () => {
- await nextRender();
+ await waitUntil(() => dispatchStub.callCount >= 1);
let firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-start');
- assert.include(firedEventTypes, 'render-progress');
- await nextRender();
+ await waitUntil(() => dispatchStub.callCount >= 2);
firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-content');
});
@@ -678,7 +674,7 @@
};
element.render(keyLocations);
// Make sure all listeners are installed.
- await nextRender();
+ await element.untilGroupsRendered();
});
test('hides lines behind two context controls', () => {
@@ -742,7 +738,7 @@
assert.include(diffRows[8].textContent, 'after');
assert.include(diffRows[9].textContent, 'unchanged 11');
- await nextRender();
+ await element.untilGroupsRendered();
const firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-content');
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
index e80d86b..e00353a 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -336,10 +336,6 @@
this.preventAutoScrollOnManualScroll = true;
};
- private _boundHandleDiffRenderProgress = () => {
- this._updateStops();
- };
-
private _boundHandleDiffRenderContent = () => {
this._updateStops();
// When done rendering, turn focus on move and automatic scrolling back on
@@ -551,10 +547,6 @@
);
diff.removeEventListener('render-start', this._boundHandleDiffRenderStart);
diff.removeEventListener(
- 'render-progress',
- this._boundHandleDiffRenderProgress
- );
- diff.removeEventListener(
'render-content',
this._boundHandleDiffRenderContent
);
@@ -570,10 +562,6 @@
this.boundHandleDiffLoadingChanged
);
diff.addEventListener('render-start', this._boundHandleDiffRenderStart);
- diff.addEventListener(
- 'render-progress',
- this._boundHandleDiffRenderProgress
- );
diff.addEventListener('render-content', this._boundHandleDiffRenderContent);
diff.addEventListener('line-selected', this._boundHandleDiffLineSelected);
}
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 6f71c65..616a36f 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
@@ -614,6 +614,7 @@
const result = [];
let lastChunkEndOffset = 0;
for (const {offset, keyLocation} of chunkEnds) {
+ if (lastChunkEndOffset === offset) continue;
result.push({
lines: lines.slice(lastChunkEndOffset, offset),
keyLocation,
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 b8262e0..e015d49 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
@@ -17,6 +17,7 @@
import {BLANK_LINE, GrDiffLine, GrDiffLineType} from './gr-diff-line';
import {LineRange, Side} from '../../../api/diff';
import {LineNumber} from './gr-diff-line';
+import {assertIsDefined, check} from '../../../utils/common-util';
export enum GrDiffGroupType {
/** Unchanged context. */
@@ -275,7 +276,9 @@
},
};
} else {
- for (const line of options.lines ?? []) {
+ assertIsDefined(options.lines);
+ check(options.lines.length > 0, 'diff group must have lines');
+ for (const line of options.lines) {
this.addLine(line);
}
}
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 43a56d1..bbf03ae 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
@@ -46,6 +46,16 @@
]);
});
+ test('group must have lines', () => {
+ try {
+ new GrDiffGroup({type: GrDiffGroupType.BOTH});
+ } catch (e) {
+ // expected
+ return;
+ }
+ assert.fail('a standard diff group cannot be empty');
+ });
+
test('group/header line pairs', () => {
const l1 = new GrDiffLine(GrDiffLineType.BOTH, 64, 128);
const l2 = new GrDiffLine(GrDiffLineType.BOTH, 65, 129);
@@ -232,11 +242,6 @@
assert.isTrue(group.isTotal());
});
- test('not total for empty', () => {
- const group = new GrDiffGroup({type: GrDiffGroupType.BOTH});
- assert.isFalse(group.isTotal());
- });
-
test('not total for non-delta', () => {
const lines = [];
for (let idx = 0; idx < 10; idx++) {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index 0eed83d..1422223 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -613,8 +613,7 @@
threadEl.setAttribute('slot', 'right-1');
const content = [
{
- a: [],
- b: [],
+ a: ['asdf'],
},
{
ab: Array(13).fill('text'),
@@ -624,7 +623,7 @@
await waitForEventOnce(element, 'render');
element.appendChild(threadEl);
- await flush();
+ await waitForEventOnce(element, 'render');
const hint = queryAndAssert<GrRangedCommentHint>(
element,
@@ -651,21 +650,18 @@
firstHint.setAttribute('slot', 'right-1');
const content = [
{
- a: [],
- b: [],
+ a: ['asdf'],
},
{
ab: Array(13).fill('text'),
},
];
setupSampleDiff({content});
+ await waitForEventOnce(element, 'render');
element.appendChild(firstHint);
- await flush();
- element._handleRenderContent();
- await flush();
element.appendChild(threadEl);
- await flush();
+ await waitForEventOnce(element, 'render');
assert.equal(
element.querySelectorAll('gr-ranged-comment-hint').length,
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index ea7865e..985bec1 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -27,7 +27,7 @@
import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
import {queryAndAssert, query} from '../utils/common-util';
import {FlagsService} from '../services/flags/flags';
-import {afterNextRender, Key, Modifier} from '../utils/dom-util';
+import {Key, Modifier} from '../utils/dom-util';
import {Observable} from 'rxjs';
import {filter, take, timeout} from 'rxjs/operators';
import {HighlightService} from '../services/highlight/highlight-service';
@@ -224,10 +224,6 @@
return waitUntil(() => stub.called, `${name} was not called`);
}
-export async function nextRender() {
- return new Promise(resolve => afterNextRender(resolve));
-}
-
/**
* Subscribes to the observable and resolves once it emits a matching value.
* Usage:
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index f2e0994..be6aa06 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -491,8 +491,18 @@
return false;
}
+/** Returns a promise that waits for the element's height to become > 0. */
+export function untilRendered(el: HTMLElement) {
+ return new Promise(resolve => {
+ whenRendered(el, resolve);
+ });
+}
+
/** Executes the given callback when the element's height is > 0. */
-export function whenRendered(el: HTMLElement, callback: () => void) {
+export function whenRendered(
+ el: HTMLElement,
+ callback: (value?: unknown) => void
+) {
if (el.clientHeight > 0) {
callback();
return;
@@ -505,14 +515,3 @@
});
obs.observe(el);
}
-
-/**
- * Mimics a Polymer utility. `requestAnimationFrame` is called before the next
- * browser paint. An additional `setTimeout` ensures that the paint has
- * actually happened.
- */
-export function afterNextRender(callback: (value?: unknown) => void) {
- requestAnimationFrame(() => {
- setTimeout(callback);
- });
-}
