Migrate gr-diff-builder-element from Polymer to plain class
Release-Notes: skip
Change-Id: I2dc83705b66c3cb11d9861b578a6db04599d6f4a
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index dda8490..88aae5c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -313,7 +313,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diff.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.$.diff.diffBuilder.builder, GrDiffBuilderImage);
// Left image rendered with the parent commit's version of the file.
const leftImage =
@@ -381,7 +381,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diff.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.$.diff.diffBuilder.builder, GrDiffBuilderImage);
// Left image rendered with the parent commit's version of the file.
const leftImage =
@@ -445,7 +445,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diff.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.$.diff.diffBuilder.builder, GrDiffBuilderImage);
const leftImage =
element.$.diff.$.diffTable.querySelector('td.left img');
@@ -493,7 +493,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diff.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.$.diff.diffBuilder.builder, GrDiffBuilderImage);
const leftImage =
element.$.diff.$.diffTable.querySelector('td.left img');
@@ -543,7 +543,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diff.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.$.diff.diffBuilder.builder, GrDiffBuilderImage);
const leftImage =
element.$.diff.$.diffTable.querySelector('td.left img');
assert.isNotOk(leftImage);
@@ -623,7 +623,7 @@
test('clearBlame', () => {
element._blame = [];
- const setBlameSpy = sinon.spy(element.$.diff.$.diffBuilder, 'setBlame');
+ const setBlameSpy = sinon.spy(element.$.diff.diffBuilder, 'setBlame');
element.clearBlame();
assert.isNull(element._blame);
assert.isTrue(setBlameSpy.calledWithExactly(null));
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 efd6f10..d200c75 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
@@ -1,24 +1,11 @@
/**
* @license
- * Copyright (C) 2016 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Copyright 2016 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
*/
import '../gr-diff-processor/gr-diff-processor';
import '../../../elements/shared/gr-hovercard/gr-hovercard';
import './gr-diff-builder-side-by-side';
-import {PolymerElement} from '@polymer/polymer/polymer-element';
-import {htmlTemplate} from './gr-diff-builder-element_html';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {DiffBuilder, DiffContextExpandedEventDetail} from './gr-diff-builder';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
@@ -26,7 +13,6 @@
import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
import {GrDiffBuilderBinary} from './gr-diff-builder-binary';
import {CancelablePromise, makeCancelable} from '../../../scripts/util';
-import {customElement, property, observe} from '@polymer/decorators';
import {BlameInfo, ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {CoverageRange, DiffLayer} from '../../../types/types';
@@ -53,12 +39,15 @@
hideInContextControl,
} from '../gr-diff/gr-diff-group';
import {getLineNumber, getSideByLineEl} from '../gr-diff/gr-diff-utils';
-import {fireAlert, fireEvent, fire} from '../../../utils/event-util';
-import {afterNextRender} from '@polymer/polymer/lib/utils/render-status';
+import {
+ fireAlert,
+ fire,
+ HTMLElementEventDetailType,
+} from '../../../utils/event-util';
+import {assertIsDefined} from '../../../utils/common-util';
+import {afterNextRender} from '../../../utils/dom-util';
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
-
-// https://gerrit.googlesource.com/gerrit/+/234616a8627334686769f1de989d286039f4d6a5/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js#740
const COMMIT_MSG_PATH = '/COMMIT_MSG';
const COMMIT_MSG_LINE_LENGTH = 72;
@@ -111,91 +100,59 @@
}
}
-@customElement('gr-diff-builder')
-export class GrDiffBuilderElement
- extends PolymerElement
- implements GroupConsumer
-{
- static get template() {
- return htmlTemplate;
- }
-
- @property({type: Object})
+// TODO: Rename the class and the file and remove "element". This is not an
+// element anymore.
+export class GrDiffBuilderElement implements GroupConsumer {
diff?: DiffInfo;
- @property({type: String})
+ diffElement?: HTMLTableElement;
+
viewMode?: string;
- @property({type: Boolean})
isImageDiff?: boolean;
- @property({type: Object})
baseImage: ImageInfo | null = null;
- @property({type: Object})
revisionImage: ImageInfo | null = null;
- @property({type: Number})
- parentIndex?: number;
-
- @property({type: String})
path?: string;
- @property({type: Object})
prefs: DiffPreferencesInfo = createDefaultDiffPrefs();
- @property({type: Object})
renderPrefs?: RenderPreferences;
- @property({type: Object})
- _builder?: DiffBuilder;
-
- /**
- * The gr-diff-processor adds (and only adds!) to this array. It does so by
- * using `this.push()` and Polymer's two-way data binding.
- * Below (@observe('_groups.splices')) we are observing the groups that the
- * processor adds, and pass them on to the builder for rendering. Henceforth
- * the builder groups are the source of truth, because when
- * expanding/collapsing groups only the builder is updated. This field and the
- * corresponsing one in the processor are not updated.
- */
- @property({type: Array})
- _groups: GrDiffGroup[] = [];
+ useNewImageDiffUi = false;
/**
* Layers passed in from the outside.
+ *
+ * See `layersInternal` for where these layers will end up together with the
+ * internal layers.
*/
- @property({type: Array})
layers: DiffLayer[] = [];
+ // visible for testing
+ builder?: DiffBuilder;
+
/**
- * All layers, both from the outside and the default ones.
+ * All layers, both from the outside and the default ones. See `layers` for
+ * the property that can be set from the outside.
*/
- @property({type: Array})
- _layers: DiffLayer[] = [];
+ // visible for testing
+ layersInternal: DiffLayer[] = [];
- @property({type: Boolean})
- _showTabs?: boolean;
+ // visible for testing
+ showTabs?: boolean;
- @property({type: Boolean})
- _showTrailingWhitespace?: boolean;
-
- @property({type: Array})
- commentRanges: CommentRangeLayer[] = [];
-
- @property({type: Array, observer: 'coverageObserver'})
- coverageRanges: CoverageRange[] = [];
-
- @property({type: Boolean})
- useNewImageDiffUi = false;
+ // 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}`.
*/
- @property({type: Object})
- _cancelableRenderPromise: CancelablePromise<unknown> | null = null;
+ private cancelableRenderPromise: CancelablePromise<unknown> | null = null;
private coverageLayerLeft = new GrCoverageLayer(Side.LEFT);
@@ -207,48 +164,16 @@
processor = new GrDiffProcessor();
constructor() {
- super();
- afterNextRender(this, () => {
- this.addEventListener(
- 'diff-context-expanded',
- (e: CustomEvent<DiffContextExpandedEventDetail>) => {
- // Don't stop propagation. The host may listen for reporting or
- // resizing.
- this.replaceGroup(e.detail.contextGroup, e.detail.groups);
- }
- );
- });
this.processor.consumer = this;
}
- override disconnectedCallback() {
- this.processor.cancel();
- if (this._builder) {
- this._builder.clear();
- }
- super.disconnectedCallback();
+ updateCommentRanges(ranges: CommentRangeLayer[]) {
+ this.rangeLayer.updateRanges(ranges);
}
- get diffElement(): HTMLTableElement {
- // Not searching in shadowRoot, because the diff table is slotted!
- return this.querySelector('#diffTable') as HTMLTableElement;
- }
-
- @observe('commentRanges.*')
- rangeObserver() {
- this.rangeLayer.updateRanges(this.commentRanges);
- }
-
- coverageObserver(coverageRanges: CoverageRange[]) {
- const leftRanges = coverageRanges.filter(
- range => range && range.side === Side.LEFT
- );
- this.coverageLayerLeft.setRanges(leftRanges);
-
- const rightRanges = coverageRanges.filter(
- range => range && range.side === Side.RIGHT
- );
- this.coverageLayerRight.setRanges(rightRanges);
+ updateCoverageRanges(rs: CoverageRange[]) {
+ this.coverageLayerLeft.setRanges(rs.filter(r => r?.side === Side.LEFT));
+ this.coverageLayerRight.setRanges(rs.filter(r => r?.side === Side.RIGHT));
}
render(keyLocations: KeyLocations): void {
@@ -256,42 +181,44 @@
// installed, and |render| satisfies the requirement, however,
// |attached| doesn't because in the diff view page, the element is
// attached before plugins are installed.
- this._setupAnnotationLayers();
+ this.setupAnnotationLayers();
- this._showTabs = this.prefs.show_tabs;
- this._showTrailingWhitespace = this.prefs.show_whitespace_errors;
+ this.showTabs = this.prefs.show_tabs;
+ this.showTrailingWhitespace = this.prefs.show_whitespace_errors;
// Stop the processor if it's running.
this.cancel();
- if (this._builder) {
- this._builder.clear();
- }
- if (!this.diff) {
- throw Error('Cannot render a diff without DiffInfo.');
- }
- this._builder = this._getDiffBuilder();
+ this.builder?.clear();
+ assertIsDefined(this.diff, 'diff');
+ assertIsDefined(this.diffElement, 'diff table');
+ this.builder = this.getDiffBuilder();
this.processor.context = this.prefs.context;
this.processor.keyLocations = keyLocations;
- this._clearDiffContent();
- this._builder.addColumns(
+ this.diffElement.addEventListener(
+ 'diff-context-expanded',
+ this.onDiffContextExpanded
+ );
+
+ this.clearDiffContent();
+ this.builder.addColumns(
this.diffElement,
getLineNumberCellWidth(this.prefs)
);
const isBinary = !!(this.isImageDiff || this.diff.binary);
- fireEvent(this, 'render-start');
- this._cancelableRenderPromise = makeCancelable(
+ this.fireDiffEvent('render-start', {});
+ this.cancelableRenderPromise = makeCancelable(
this.processor
.process(this.diff.content, isBinary)
.then(() => {
if (this.isImageDiff) {
- (this._builder as GrDiffBuilderImage).renderDiff();
+ (this.builder as GrDiffBuilderImage).renderDiff();
}
- afterNextRender(this, () => fireEvent(this, 'render-content'));
+ afterNextRender(() => 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
@@ -301,17 +228,39 @@
return;
})
.finally(() => {
- this._cancelableRenderPromise = null;
+ this.cancelableRenderPromise = null;
})
);
}
- _setupAnnotationLayers() {
+ private onDiffContextExpanded = (
+ e: CustomEvent<DiffContextExpandedEventDetail>
+ ) => {
+ // Don't stop propagation. The host may listen for reporting or
+ // resizing.
+ this.replaceGroup(e.detail.contextGroup, e.detail.groups);
+ };
+
+ private fireDiffEvent<K extends keyof HTMLElementEventMap>(
+ type: K,
+ detail: HTMLElementEventDetailType<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);
+ }
+
+ // visible for testing
+ setupAnnotationLayers() {
const layers: DiffLayer[] = [
- this._createTrailingWhitespaceLayer(),
- this._createIntralineLayer(),
- this._createTabIndicatorLayer(),
- this._createSpecialCharacterIndicatorLayer(),
+ this.createTrailingWhitespaceLayer(),
+ this.createIntralineLayer(),
+ this.createTabIndicatorLayer(),
+ this.createSpecialCharacterIndicatorLayer(),
this.rangeLayer,
this.coverageLayerLeft,
this.coverageLayerRight,
@@ -320,15 +269,15 @@
if (this.layers) {
layers.push(...this.layers);
}
- this._layers = layers;
+ this.layersInternal = layers;
}
getContentTdByLine(lineNumber: LineNumber, side?: Side, root?: Element) {
- if (!this._builder) return null;
- return this._builder.getContentTdByLine(lineNumber, side, root);
+ if (!this.builder) return null;
+ return this.builder.getContentTdByLine(lineNumber, side, root);
}
- _getDiffRowByChild(child: Element) {
+ private getDiffRowByChild(child: Element) {
while (!child.classList.contains('diff-row') && child.parentElement) {
child = child.parentElement;
}
@@ -342,23 +291,23 @@
const side = getSideByLineEl(lineEl);
// Performance optimization because we already have an element in the
// correct row
- const row = this._getDiffRowByChild(lineEl);
+ const row = this.getDiffRowByChild(lineEl);
return this.getContentTdByLine(line, side, row);
}
getLineElByNumber(lineNumber: LineNumber, side?: Side) {
- if (!this._builder) return null;
- return this._builder.getLineElByNumber(lineNumber, side);
+ if (!this.builder) return null;
+ return this.builder.getLineElByNumber(lineNumber, side);
}
getLineNumberRows() {
- if (!this._builder) return [];
- return this._builder.getLineNumberRows();
+ if (!this.builder) return [];
+ return this.builder.getLineNumberRows();
}
getLineNumEls(side: Side) {
- if (!this._builder) return [];
- return this._builder.getLineNumEls(side);
+ if (!this.builder) return [];
+ return this.builder.getLineNumEls(side);
}
/**
@@ -370,8 +319,8 @@
* @param side The side the line number refer to.
*/
unhideLine(lineNum: number, side: Side) {
- if (!this._builder) return;
- const group = this._builder.findGroup(side, lineNum);
+ if (!this.builder) return;
+ const group = this.builder.findGroup(side, lineNum);
// Cannot unhide a line that is not part of the diff.
if (!group) return;
// If it's already visible, great!
@@ -414,45 +363,50 @@
contextGroup: GrDiffGroup,
newGroups: readonly GrDiffGroup[]
) {
- if (!this._builder) return;
- fireEvent(this, 'render-start');
+ if (!this.builder) return;
+ this.fireDiffEvent('render-start', {});
const linesRendered = newGroups.reduce(
(sum, group) => sum + group.lines.length,
0
);
- this._builder.replaceGroup(contextGroup, newGroups);
- afterNextRender(this, () => {
- fire(this, 'render-progress', {linesRendered});
- fireEvent(this, 'render-content');
+ this.builder.replaceGroup(contextGroup, newGroups);
+ afterNextRender(() => {
+ this.fireDiffEvent('render-progress', {linesRendered});
+ this.fireDiffEvent('render-content', {});
});
}
cancel() {
this.processor.cancel();
- if (this._cancelableRenderPromise) {
- this._cancelableRenderPromise.cancel();
- this._cancelableRenderPromise = null;
- }
+ this.builder?.clear();
+ this.cancelableRenderPromise?.cancel();
+ this.cancelableRenderPromise = null;
+ this.diffElement?.removeEventListener(
+ 'diff-context-expanded',
+ this.onDiffContextExpanded
+ );
}
- _handlePreferenceError(pref: string): never {
+ // visible for testing
+ handlePreferenceError(pref: string): never {
const message =
`The value of the '${pref}' user preference is ` +
'invalid. Fix in diff preferences';
- fireAlert(this, message);
+ assertIsDefined(this.diffElement, 'diff table');
+ fireAlert(this.diffElement, message);
throw Error(`Invalid preference value: ${pref}`);
}
- _getDiffBuilder(): DiffBuilder {
- if (!this.diff) {
- throw Error('Cannot render a diff without DiffInfo.');
- }
+ // visible for testing
+ getDiffBuilder(): DiffBuilder {
+ assertIsDefined(this.diff, 'diff');
+ assertIsDefined(this.diffElement, 'diff table');
if (isNaN(this.prefs.tab_size) || this.prefs.tab_size <= 0) {
- this._handlePreferenceError('tab size');
+ this.handlePreferenceError('tab size');
}
if (isNaN(this.prefs.line_length) || this.prefs.line_length <= 0) {
- this._handlePreferenceError('diff width');
+ this.handlePreferenceError('diff width');
}
const localPrefs = {...this.prefs};
@@ -481,7 +435,7 @@
this.diff,
localPrefs,
this.diffElement,
- this._layers,
+ this.layersInternal,
this.renderPrefs
);
} else if (this.viewMode === DiffViewMode.UNIFIED) {
@@ -489,7 +443,7 @@
this.diff,
localPrefs,
this.diffElement,
- this._layers,
+ this.layersInternal,
this.renderPrefs
);
}
@@ -499,7 +453,8 @@
return builder;
}
- _clearDiffContent() {
+ private clearDiffContent() {
+ assertIsDefined(this.diffElement, 'diff table');
this.diffElement.innerHTML = '';
}
@@ -508,22 +463,23 @@
* server into chunks.
*/
clearGroups() {
- if (!this._builder) return;
- this._builder.clearGroups();
+ if (!this.builder) return;
+ this.builder.clearGroups();
}
/**
* Called when the processor is done converting a chunk of the diff.
*/
addGroup(group: GrDiffGroup) {
- if (!this._builder) return;
- this._builder.addGroups([group]);
- afterNextRender(this, () =>
- fire(this, 'render-progress', {linesRendered: group.lines.length})
+ if (!this.builder) return;
+ this.builder.addGroups([group]);
+ afterNextRender(() =>
+ this.fireDiffEventRenderProgress({linesRendered: group.lines.length})
);
}
- _createIntralineLayer(): DiffLayer {
+ // visible for testing
+ createIntralineLayer(): DiffLayer {
return {
// Take a DIV.contentText element and a line object with intraline
// differences to highlight and apply them to the element as
@@ -555,8 +511,9 @@
};
}
- _createTabIndicatorLayer(): DiffLayer {
- const show = () => this._showTabs;
+ // visible for testing
+ createTabIndicatorLayer(): DiffLayer {
+ const show = () => this.showTabs;
return {
annotate(contentEl: HTMLElement, _: HTMLElement, line: GrDiffLine) {
// If visible tabs are disabled, do nothing.
@@ -570,7 +527,7 @@
};
}
- _createSpecialCharacterIndicatorLayer(): DiffLayer {
+ private createSpecialCharacterIndicatorLayer(): DiffLayer {
return {
annotate(contentEl: HTMLElement, _: HTMLElement, line: GrDiffLine) {
// Find and annotate the locations of soft hyphen (\u00AD)
@@ -586,8 +543,9 @@
};
}
- _createTrailingWhitespaceLayer(): DiffLayer {
- const show = () => this._showTrailingWhitespace;
+ // visible for testing
+ createTrailingWhitespaceLayer(): DiffLayer {
+ const show = () => this.showTrailingWhitespace;
return {
annotate(contentEl: HTMLElement, _: HTMLElement, line: GrDiffLine) {
@@ -615,18 +573,12 @@
}
setBlame(blame: BlameInfo[] | null) {
- if (!this._builder) return;
- this._builder.setBlame(blame ?? []);
+ if (!this.builder) return;
+ this.builder.setBlame(blame ?? []);
}
updateRenderPrefs(renderPrefs: RenderPreferences) {
- this._builder?.updateRenderPrefs(renderPrefs);
+ this.builder?.updateRenderPrefs(renderPrefs);
this.processor.updateRenderPrefs(renderPrefs);
}
}
-
-declare global {
- interface HTMLElementTagNameMap {
- 'gr-diff-builder': GrDiffBuilderElement;
- }
-}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_html.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_html.ts
deleted file mode 100644
index bd0e034..0000000
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_html.ts
+++ /dev/null
@@ -1,23 +0,0 @@
-/**
- * @license
- * Copyright (C) 2020 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-import {html} from '@polymer/polymer/lib/utils/html-tag';
-
-export const htmlTemplate = html`
- <div class="contentWrapper">
- <slot></slot>
- </div>
-`;
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 b441ad3..8ae08f4 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,14 @@
createEmptyDiff,
} from '../../../test/test-data-generators';
import './gr-diff-builder-element';
-import {queryAndAssert, stubBaseUrl} from '../../../test/test-utils';
+import {
+ nextRender,
+ queryAndAssert,
+ stubBaseUrl,
+} 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';
-import {html} from '@polymer/polymer/lib/utils/html-tag';
import {
DiffContent,
DiffInfo,
@@ -24,35 +27,20 @@
Side,
} from '../../../api/diff';
import {stubRestApi} from '../../../test/test-utils';
-import {afterNextRender} from '@polymer/polymer/lib/utils/render-status';
import {GrDiffBuilderLegacy} from './gr-diff-builder-legacy';
import {waitForEventOnce} from '../../../utils/event-util';
import {GrDiffBuilderElement} from './gr-diff-builder-element';
import {createDefaultDiffPrefs} from '../../../constants/constants';
import {KeyLocations} from '../gr-diff-processor/gr-diff-processor';
import {BlameInfo} from '../../../types/common';
-
-const basicFixture = fixtureFromTemplate(html`
- <gr-diff-builder>
- <table id="diffTable"></table>
- </gr-diff-builder>
-`);
-
-const divWithTextFixture = fixtureFromTemplate(html`
- <div>Lorem ipsum dolor sit amet, suspendisse inceptos vehicula</div>
-`);
-
-const mockDiffFixture = fixtureFromTemplate(html`
- <gr-diff-builder view-mode="SIDE_BY_SIDE">
- <table id="diffTable"></table>
- </gr-diff-builder>
-`);
+import {fixture, html} from '@open-wc/testing-helpers';
const DEFAULT_PREFS = createDefaultDiffPrefs();
suite('gr-diff-builder tests', () => {
let element: GrDiffBuilderElement;
let builder: GrDiffBuilderLegacy;
+ let diffTable: HTMLTableElement;
const LINE_BREAK_HTML = '<span class="style-scope gr-diff br"></span>';
const WBR_HTML = '<wbr class="style-scope gr-diff">';
@@ -61,7 +49,7 @@
builder = new GrDiffBuilderSideBySide(
createEmptyDiff(),
{...createDefaultDiffPrefs(), ...prefs},
- element.diffElement
+ diffTable
);
};
@@ -71,8 +59,10 @@
return line;
};
- setup(() => {
- element = basicFixture.instantiate() as GrDiffBuilderElement;
+ setup(async () => {
+ diffTable = await fixture(html`<table id="diffTable"></table>`);
+ element = new GrDiffBuilderElement();
+ element.diffElement = diffTable;
stubRestApi('getLoggedIn').returns(Promise.resolve(false));
stubRestApi('getProjectConfig').returns(Promise.resolve(createConfig()));
stubBaseUrl('/r');
@@ -107,7 +97,7 @@
tab_size: 4,
line_length: 50,
};
- builder = element._getDiffBuilder() as GrDiffBuilderLegacy;
+ builder = element.getDiffBuilder() as GrDiffBuilderLegacy;
assert.equal(builder._prefs.line_length, 50);
});
@@ -120,7 +110,7 @@
tab_size: 4,
line_length: 50,
};
- builder = element._getDiffBuilder() as GrDiffBuilderLegacy;
+ builder = element.getDiffBuilder() as GrDiffBuilderLegacy;
assert.equal(builder._prefs.line_length, 72);
});
});
@@ -142,13 +132,13 @@
test('_handlePreferenceError throws with invalid preference', () => {
element.prefs = {...createDefaultDiffPrefs(), tab_size: 0};
- assert.throws(() => element._getDiffBuilder());
+ assert.throws(() => element.getDiffBuilder());
});
test('_handlePreferenceError triggers alert and javascript error', () => {
const errorStub = sinon.stub();
- element.addEventListener('show-alert', errorStub);
- assert.throws(() => element._handlePreferenceError('tab size'));
+ diffTable.addEventListener('show-alert', errorStub);
+ assert.throws(() => element.handlePreferenceError('tab size'));
assert.equal(
errorStub.lastCall.args[0].detail.message,
"The value of the 'tab size' user preference is invalid. " +
@@ -167,11 +157,13 @@
return Array.from(str).slice(start, end).join('');
}
- setup(() => {
- el = divWithTextFixture.instantiate() as HTMLElement;
+ setup(async () => {
+ el = await fixture(html`
+ <div>Lorem ipsum dolor sit amet, suspendisse inceptos vehicula</div>
+ `);
str = el.textContent ?? '';
annotateElementSpy = sinon.spy(GrAnnotation, 'annotateElement');
- layer = document.createElement('gr-diff-builder')._createIntralineLayer();
+ layer = element.createIntralineLayer();
});
test('annotate no highlights', () => {
@@ -297,14 +289,12 @@
});
suite('tab indicators', () => {
- let element: GrDiffBuilderElement;
let layer: DiffLayer;
const lineNumberEl = document.createElement('td');
setup(() => {
- element = basicFixture.instantiate() as GrDiffBuilderElement;
- element._showTabs = true;
- layer = element._createTabIndicatorLayer();
+ element.showTabs = true;
+ layer = element.createTabIndicatorLayer();
});
test('does nothing with empty line', () => {
@@ -347,7 +337,7 @@
});
test('does not annotate when disabled', () => {
- element._showTabs = false;
+ element.showTabs = false;
const str = '\tlorem upsum';
const l = line(str);
@@ -403,49 +393,44 @@
});
suite('layers', () => {
- let element: GrDiffBuilderElement;
let initialLayersCount = 0;
let withLayerCount = 0;
setup(() => {
const layers: DiffLayer[] = [];
- element = basicFixture.instantiate() as GrDiffBuilderElement;
element.layers = layers;
- element._showTrailingWhitespace = true;
- element._setupAnnotationLayers();
- initialLayersCount = element._layers.length;
+ element.showTrailingWhitespace = true;
+ element.setupAnnotationLayers();
+ initialLayersCount = element.layersInternal.length;
});
test('no layers', () => {
- element._setupAnnotationLayers();
- assert.equal(element._layers.length, initialLayersCount);
+ element.setupAnnotationLayers();
+ assert.equal(element.layersInternal.length, initialLayersCount);
});
suite('with layers', () => {
const layers: DiffLayer[] = [{annotate: () => {}}, {annotate: () => {}}];
setup(() => {
- element = basicFixture.instantiate() as GrDiffBuilderElement;
element.layers = layers;
- element._showTrailingWhitespace = true;
- element._setupAnnotationLayers();
- withLayerCount = element._layers.length;
+ element.showTrailingWhitespace = true;
+ element.setupAnnotationLayers();
+ withLayerCount = element.layersInternal.length;
});
test('with layers', () => {
- element._setupAnnotationLayers();
- assert.equal(element._layers.length, withLayerCount);
+ element.setupAnnotationLayers();
+ assert.equal(element.layersInternal.length, withLayerCount);
assert.equal(initialLayersCount + layers.length, withLayerCount);
});
});
});
suite('trailing whitespace', () => {
- let element: GrDiffBuilderElement;
let layer: DiffLayer;
const lineNumberEl = document.createElement('td');
setup(() => {
- element = basicFixture.instantiate() as GrDiffBuilderElement;
- element._showTrailingWhitespace = true;
- layer = element._createTrailingWhitespaceLayer();
+ element.showTrailingWhitespace = true;
+ layer = element.createTrailingWhitespaceLayer();
});
test('does nothing with empty line', () => {
@@ -515,7 +500,7 @@
});
test('does not annotate when disabled', () => {
- element._showTrailingWhitespace = false;
+ element.showTrailingWhitespace = false;
const str = 'lorem upsum\t \t ';
const l = line(str);
const el = document.createElement('div');
@@ -532,7 +517,6 @@
let content: DiffContent[] = [];
setup(() => {
- element = basicFixture.instantiate() as GrDiffBuilderElement;
element.viewMode = 'SIDE_BY_SIDE';
processStub = sinon
.stub(element.processor, 'process')
@@ -560,7 +544,7 @@
test('text', async () => {
element.diff = {...createEmptyDiff(), content};
element.render(keyLocations);
- await waitForEventOnce(element, 'render-content');
+ await waitForEventOnce(diffTable, 'render-content');
assert.isTrue(processStub.calledOnce);
assert.isFalse(processStub.lastCall.args[1]);
});
@@ -569,7 +553,7 @@
element.diff = {...createEmptyDiff(), content, binary: true};
element.isImageDiff = true;
element.render(keyLocations);
- await waitForEventOnce(element, 'render-content');
+ await waitForEventOnce(diffTable, 'render-content');
assert.isTrue(processStub.calledOnce);
assert.isTrue(processStub.lastCall.args[1]);
});
@@ -577,7 +561,7 @@
test('binary', async () => {
element.diff = {...createEmptyDiff(), content, binary: true};
element.render(keyLocations);
- await waitForEventOnce(element, 'render-content');
+ await waitForEventOnce(diffTable, 'render-content');
assert.isTrue(processStub.calledOnce);
assert.isTrue(processStub.lastCall.args[1]);
});
@@ -605,11 +589,10 @@
],
},
];
- element = basicFixture.instantiate() as GrDiffBuilderElement;
- dispatchStub = sinon.stub(element, 'dispatchEvent');
- outputEl = queryAndAssert(element, '#diffTable');
+ dispatchStub = sinon.stub(diffTable, 'dispatchEvent');
+ outputEl = element.diffElement!;
keyLocations = {left: {}, right: {}};
- sinon.stub(element, '_getDiffBuilder').callsFake(() => {
+ sinon.stub(element, 'getDiffBuilder').callsFake(() => {
builder = new GrDiffBuilderSideBySide(
{...createEmptyDiff(), content},
prefs,
@@ -656,9 +639,13 @@
});
test('render-start and render-content are fired', async () => {
- await new Promise(resolve => afterNextRender(element, resolve));
- const firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
+ await nextRender();
+ let firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-start');
+ assert.include(firedEventTypes, 'render-progress');
+
+ await nextRender();
+ firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-content');
});
@@ -673,11 +660,7 @@
let dispatchStub: sinon.SinonStub;
setup(async () => {
- element = basicFixture.instantiate() as GrDiffBuilderElement;
- dispatchStub = sinon.stub(element, 'dispatchEvent');
- const afterNextRenderPromise = new Promise(resolve => {
- afterNextRender(element, resolve);
- });
+ dispatchStub = sinon.stub(diffTable, 'dispatchEvent');
element.diff = {
...createEmptyDiff(),
content: [
@@ -695,14 +678,14 @@
};
element.render(keyLocations);
// Make sure all listeners are installed.
- await afterNextRenderPromise;
+ await nextRender();
});
test('hides lines behind two context controls', () => {
- const contextControls = element.querySelectorAll('gr-context-controls');
+ const contextControls = diffTable.querySelectorAll('gr-context-controls');
assert.equal(contextControls.length, 2);
- const diffRows = element.querySelectorAll('.diff-row');
+ const diffRows = diffTable.querySelectorAll('.diff-row');
// The first two are LOST and FILE line
assert.equal(diffRows.length, 2 + 1 + 1 + 1);
assert.include(diffRows[2].textContent, 'unchanged 10');
@@ -712,7 +695,7 @@
});
test('clicking +x common lines expands those lines', () => {
- const contextControls = element.querySelectorAll('gr-context-controls');
+ const contextControls = diffTable.querySelectorAll('gr-context-controls');
const topExpandCommonButton =
contextControls[0].shadowRoot?.querySelectorAll<HTMLElement>(
'.showContext'
@@ -720,7 +703,7 @@
assert.isOk(topExpandCommonButton);
assert.include(topExpandCommonButton!.textContent, '+9 common lines');
topExpandCommonButton!.click();
- const diffRows = element.querySelectorAll('.diff-row');
+ const diffRows = diffTable.querySelectorAll('.diff-row');
// The first two are LOST and FILE line
assert.equal(diffRows.length, 2 + 10 + 1 + 1);
assert.include(diffRows[2].textContent, 'unchanged 1');
@@ -742,7 +725,7 @@
dispatchStub.reset();
element.unhideLine(4, Side.LEFT);
- const diffRows = element.querySelectorAll('.diff-row');
+ const diffRows = diffTable.querySelectorAll('.diff-row');
// The first two are LOST and FILE line
// Lines 3-5 (Line 4 plus 1 context in each direction) will be expanded
// Because context expanders do not hide <3 lines, lines 1-2 will also
@@ -759,20 +742,19 @@
assert.include(diffRows[8].textContent, 'after');
assert.include(diffRows[9].textContent, 'unchanged 11');
- await new Promise(resolve => afterNextRender(element, resolve));
+ await nextRender();
const firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
assert.include(firedEventTypes, 'render-content');
});
});
suite('mock-diff', () => {
- let element: GrDiffBuilderElement;
let builder: GrDiffBuilderSideBySide;
let diff: DiffInfo;
let keyLocations: KeyLocations;
setup(() => {
- element = mockDiffFixture.instantiate() as GrDiffBuilderElement;
+ element.viewMode = DiffViewMode.SIDE_BY_SIDE;
diff = createDiff();
element.diff = diff;
@@ -785,11 +767,11 @@
tab_size: 4,
};
element.render(keyLocations);
- builder = element._builder as GrDiffBuilderSideBySide;
+ builder = element.builder as GrDiffBuilderSideBySide;
});
test('aria-labels on added line numbers', () => {
- const deltaLineNumberButton = element.diffElement.querySelectorAll(
+ const deltaLineNumberButton = diffTable.querySelectorAll(
'.lineNumButton.right'
)[5];
@@ -798,7 +780,7 @@
});
test('aria-labels on removed line numbers', () => {
- const deltaLineNumberButton = element.diffElement.querySelectorAll(
+ const deltaLineNumberButton = diffTable.querySelectorAll(
'.lineNumButton.left'
)[10];
@@ -826,7 +808,7 @@
});
test('getContentTdByLineEl works both with button and td', () => {
- const diffRow = element.diffElement.querySelectorAll('tr.diff-row')[2];
+ const diffRow = diffTable.querySelectorAll('tr.diff-row')[2];
const lineNumTdLeft = queryAndAssert(diffRow, 'td.lineNum.left');
const lineNumButtonLeft = queryAndAssert(lineNumTdLeft, 'button');
@@ -918,7 +900,7 @@
const contentEl = builder.getContentByLine(
5,
Side.LEFT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
assert.isOk(contentEl);
const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.LEFT);
@@ -931,7 +913,7 @@
const contentEl = builder.getContentByLine(
5,
Side.RIGHT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
assert.isOk(contentEl);
const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.RIGHT);
@@ -944,12 +926,12 @@
// Re-render as unified:
element.viewMode = 'UNIFIED_DIFF';
element.render(keyLocations);
- builder = element._builder as GrDiffBuilderSideBySide;
+ builder = element.builder as GrDiffBuilderSideBySide;
const contentEl = builder.getContentByLine(
5,
Side.LEFT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
assert.isOk(contentEl);
const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.LEFT);
@@ -962,12 +944,12 @@
// Re-render as unified:
element.viewMode = 'UNIFIED_DIFF';
element.render(keyLocations);
- builder = element._builder as GrDiffBuilderSideBySide;
+ builder = element.builder as GrDiffBuilderSideBySide;
const contentEl = builder.getContentByLine(
5,
Side.RIGHT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
assert.isOk(contentEl);
const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.RIGHT);
@@ -980,7 +962,7 @@
const startElem = builder.getContentByLine(
5,
Side.LEFT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
assert.isOk(startElem);
const expectedStartString = diff.content[2].ab?.[0];
@@ -996,7 +978,7 @@
const startElem = builder.getContentByLine(
5,
Side.RIGHT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
const expectedStartString = diff.content[1].b?.[0];
const expectedNextString = diff.content[1].b?.[1];
@@ -1012,12 +994,12 @@
// Re-render as unified:
element.viewMode = 'UNIFIED_DIFF';
element.render(keyLocations);
- builder = element._builder as GrDiffBuilderSideBySide;
+ builder = element.builder as GrDiffBuilderSideBySide;
const startElem = builder.getContentByLine(
5,
Side.LEFT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
const expectedStartString = diff.content[2].ab?.[0];
const expectedNextString = diff.content[2].ab?.[1];
@@ -1033,12 +1015,12 @@
// Re-render as unified:
element.viewMode = 'UNIFIED_DIFF';
element.render(keyLocations);
- builder = element._builder as GrDiffBuilderSideBySide;
+ builder = element.builder as GrDiffBuilderSideBySide;
const startElem = builder.getContentByLine(
5,
Side.RIGHT,
- element.$.diffTable as HTMLTableElement
+ element.diffElement as HTMLTableElement
);
const expectedStartString = diff.content[1].b?.[0];
const expectedNextString = diff.content[1].b?.[1];
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.js b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.js
index f48d673..9c6fad6 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.js
+++ b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.js
@@ -19,11 +19,11 @@
import '../gr-diff/gr-diff.js';
import './gr-diff-cursor.js';
import {fixture, html} from '@open-wc/testing-helpers';
-import {listenOnce, mockPromise} from '../../../test/test-utils.js';
+import {mockPromise} from '../../../test/test-utils.js';
import {createDiff} from '../../../test/test-data-generators.js';
import {createDefaultDiffPrefs} from '../../../constants/constants.js';
import {GrDiffCursor} from './gr-diff-cursor.js';
-import {afterNextRender} from '@polymer/polymer/lib/utils/render-status';
+import {waitForEventOnce} from '../../../utils/event-util.js';
suite('gr-diff-cursor tests', () => {
let cursor;
@@ -110,7 +110,8 @@
// The file comment button, if present, is a cursor stop. Ensure
// moveToFirstChunk() works correctly even if the button is not shown.
diffElement.prefs.show_file_comment_button = false;
- await flush();
+ await waitForEventOnce(diffElement, 'render');
+
cursor._updateStops();
const chunks = Array.from(diffElement.root.querySelectorAll(
@@ -160,7 +161,7 @@
};
diffElement.diff = diff;
- await new Promise(resolve => afterNextRender(diffElement, resolve));
+ await waitForEventOnce(diffElement, 'render');
cursor._updateStops();
const chunks = Array.from(diffElement.root.querySelectorAll(
@@ -215,8 +216,7 @@
suite('unified diff', () => {
setup(async () => {
diffElement.viewMode = 'UNIFIED_DIFF';
- // We must allow the diff to re-render after setting the viewMode.
- await new Promise(resolve => afterNextRender(diffElement, resolve));
+ await waitForEventOnce(diffElement, 'render');
cursor.reInitCursor();
});
@@ -453,7 +453,7 @@
});
diffElement._diffChanged(createDiff());
- await new Promise(resolve => afterNextRender(diffElement, resolve));
+ await waitForEventOnce(diffElement, 'render');
cursor.reInitCursor();
assert.isFalse(moveToNumStub.called);
assert.isTrue(moveToChunkStub.called);
@@ -472,7 +472,7 @@
cursor.side = 'right';
diffElement._diffChanged(createDiff());
- await new Promise(resolve => afterNextRender(diffElement, resolve));
+ await waitForEventOnce(diffElement, 'render');
cursor.reInitCursor();
assert.isFalse(moveToChunkStub.called);
assert.isTrue(moveToNumStub.called);
@@ -601,7 +601,7 @@
MockInteractions.tap(diffElement.shadowRoot
.querySelector('gr-context-controls').shadowRoot
.querySelector('.showContext'));
- await new Promise(resolve => afterNextRender(diffElement, resolve));
+ await waitForEventOnce(diffElement, 'render');
assert.isTrue(cursor._updateStops.called);
});
@@ -641,13 +641,10 @@
}
test('do not skip loading diffs', async () => {
- const diffRenderedPromises =
- diffElements.map(diffEl => listenOnce(diffEl, 'render'));
-
diffElements[0].diff = createDiff();
diffElements[2].diff = createDiff();
- await Promise.all([diffRenderedPromises[0], diffRenderedPromises[2]]);
- await new Promise(resolve => afterNextRender(diffElements[0], resolve));
+ await waitForEventOnce(diffElements[0], 'render');
+ await waitForEventOnce(diffElements[2], 'render');
const lastLine = diffElements[0].diff.meta_b.lines;
@@ -668,8 +665,7 @@
// Diff 1 finishing to load
diffElements[1].diff = createDiff();
- await diffRenderedPromises[1];
- await new Promise(resolve => afterNextRender(diffElements[0], resolve));
+ await waitForEventOnce(diffElements[1], 'render');
// Now we can go down
cursor.moveDown();
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 e5f9de0..40696fb 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -100,7 +100,6 @@
export interface GrDiff {
$: {
- diffBuilder: GrDiffBuilderElement;
diffTable: HTMLTableElement;
};
}
@@ -179,7 +178,7 @@
@property({type: Object})
highlightRange?: CommentRange;
- @property({type: Array})
+ @property({type: Array, observer: '_coverageRangesObserver'})
coverageRanges: CoverageRange[] = [];
@property({type: Boolean, observer: '_lineWrappingObserver'})
@@ -248,9 +247,6 @@
@property({type: Object, observer: '_blameChanged'})
blame: BlameInfo[] | null = null;
- @property({type: Number})
- parentIndex?: number;
-
@property({type: Boolean})
showNewlineWarningLeft = false;
@@ -298,6 +294,9 @@
private highlights = new GrDiffHighlight();
+ // visible for testing
+ diffBuilder = new GrDiffBuilderElement();
+
constructor() {
super();
this._setLoading(true);
@@ -321,11 +320,12 @@
this._unobserveNodes();
this.diffSelection.cleanup();
this.highlights.cleanup();
+ this.diffBuilder.cancel();
super.disconnectedCallback();
}
getLineNumEls(side: Side): HTMLElement[] {
- return this.$.diffBuilder.getLineNumEls(side);
+ return this.diffBuilder.getLineNumEls(side);
}
showNoChangeMessage(
@@ -426,19 +426,25 @@
cr.side === removedCommentRange.side &&
rangesEqual(cr.range, removedCommentRange.range)
);
- this.splice('_commentRanges', i, 1);
+ this._commentRanges.splice(i, 1);
}
- if (addedCommentRanges && addedCommentRanges.length) {
- this.push('_commentRanges', ...addedCommentRanges);
+ if (addedCommentRanges?.length) {
+ this._commentRanges.push(...addedCommentRanges);
}
if (this.highlightRange) {
- this.push('_commentRanges', {
+ this._commentRanges.push({
side: Side.RIGHT,
range: this.highlightRange,
rootId: '',
});
}
+
+ this.diffBuilder.updateCommentRanges(this._commentRanges);
+ }
+
+ _coverageRangesObserver() {
+ this.diffBuilder.updateCoverageRanges(this.coverageRanges);
}
/**
@@ -483,7 +489,7 @@
/** Cancel any remaining diff builder rendering work. */
cancel() {
- this.$.diffBuilder.cancel();
+ this.diffBuilder.cancel();
this.renderDiffTableTask?.cancel();
}
@@ -492,7 +498,7 @@
// Get rendered stops.
const stops: Array<HTMLElement | AbortStop> =
- this.$.diffBuilder.getLineNumberRows();
+ this.diffBuilder.getLineNumberRows();
// If we are still loading this diff, abort after the rendered stops to
// avoid skipping over to e.g. the next file.
@@ -512,7 +518,7 @@
_blameChanged(newValue?: BlameInfo[] | null) {
if (newValue === undefined) return;
- this.$.diffBuilder.setBlame(newValue);
+ this.diffBuilder.setBlame(newValue);
if (newValue) {
this.classList.add('showBlame');
} else {
@@ -603,7 +609,7 @@
_createCommentForSelection(side: Side, range: CommentRange) {
const lineNum = range.end_line;
- const lineEl = this.$.diffBuilder.getLineElByNumber(lineNum, side);
+ const lineEl = this.diffBuilder.getLineElByNumber(lineNum, side);
if (lineEl) {
this._createComment(lineEl, lineNum, side, range);
}
@@ -621,7 +627,7 @@
side?: Side,
range?: CommentRange
) {
- const contentEl = this.$.diffBuilder.getContentTdByLineEl(lineEl);
+ const contentEl = this.diffBuilder.getContentTdByLineEl(lineEl);
if (!contentEl) throw new Error('content el not found for line el');
side = side ?? this._getCommentSideByLineAndContent(lineEl, contentEl);
assertIsDefined(this.path, 'path');
@@ -699,7 +705,7 @@
if (!this.lineOfInterest) return;
const lineNum = this.lineOfInterest.lineNum;
if (typeof lineNum !== 'number') return;
- this.$.diffBuilder.unhideLine(lineNum, this.lineOfInterest.side);
+ this.diffBuilder.unhideLine(lineNum, this.lineOfInterest.side);
}
_cleanup() {
@@ -808,7 +814,7 @@
if (this.prefs) {
this._updatePreferenceStyles(this.prefs, renderPrefs);
}
- this.$.diffBuilder.updateRenderPrefs(renderPrefs);
+ this.diffBuilder.updateRenderPrefs(renderPrefs);
}
_diffChanged(newValue?: DiffInfo) {
@@ -820,7 +826,7 @@
}
if (this.diff) {
this.diffSelection.init(this.diff, this.$.diffTable);
- this.highlights.init(this.$.diffTable, this.$.diffBuilder);
+ this.highlights.init(this.$.diffTable, this.diffBuilder);
}
}
@@ -866,9 +872,24 @@
this._showWarning = false;
const keyLocations = this._computeKeyLocations();
- this.$.diffBuilder.prefs = this._getBypassPrefs(this.prefs);
- this.$.diffBuilder.renderPrefs = this.renderPrefs;
- this.$.diffBuilder.render(keyLocations);
+
+ // TODO: Setting tons of public properties like this is obviously a code
+ // smell. We are planning to introduce a diff model for managing all this
+ // data. Then diff builder will only need access to that model.
+ this.diffBuilder.prefs = this._getBypassPrefs(this.prefs);
+ this.diffBuilder.renderPrefs = this.renderPrefs;
+ this.diffBuilder.diff = this.diff;
+ this.diffBuilder.path = this.path;
+ this.diffBuilder.viewMode = this.viewMode;
+ this.diffBuilder.layers = this.layers ?? [];
+ this.diffBuilder.isImageDiff = this.isImageDiff;
+ this.diffBuilder.baseImage = this.baseImage ?? null;
+ this.diffBuilder.revisionImage = this.revisionImage ?? null;
+ this.diffBuilder.useNewImageDiffUi = this.useNewImageDiffUi;
+ this.diffBuilder.diffElement = this.$.diffTable;
+ this.diffBuilder.updateCommentRanges(this._commentRanges);
+ this.diffBuilder.updateCoverageRanges(this.coverageRanges);
+ this.diffBuilder.render(keyLocations);
}
_handleRenderContent() {
@@ -895,10 +916,7 @@
const commentSide = getSide(threadEl);
const range = getRange(threadEl);
if (!commentSide) continue;
- const lineEl = this.$.diffBuilder.getLineElByNumber(
- lineNum,
- commentSide
- );
+ const lineEl = this.diffBuilder.getLineElByNumber(lineNum, commentSide);
// When the line the comment refers to does not exist, log an error
// but don't crash. This can happen e.g. if the API does not fully
// validate e.g. (robot) comments
@@ -911,7 +929,7 @@
);
continue;
}
- const contentEl = this.$.diffBuilder.getContentTdByLineEl(lineEl);
+ const contentEl = this.diffBuilder.getContentTdByLineEl(lineEl);
if (!contentEl) continue;
if (lineNum === 'LOST' && !contentEl.hasChildNodes()) {
contentEl.appendChild(this._portedCommentsWithoutRangeMessage());
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_html.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_html.ts
index 6d36b89..40d4e7f 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_html.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_html.ts
@@ -698,36 +698,22 @@
class$="[[_computeContainerClass(loggedIn, viewMode, displayLine)]]"
on-click="_handleTap"
>
- <gr-diff-builder
- id="diffBuilder"
- comment-ranges="[[_commentRanges]]"
- coverage-ranges="[[coverageRanges]]"
- diff="[[diff]]"
- path="[[path]]"
- view-mode="[[viewMode]]"
- is-image-diff="[[isImageDiff]]"
- base-image="[[baseImage]]"
- layers="[[layers]]"
- revision-image="[[revisionImage]]"
- use-new-image-diff-ui="[[useNewImageDiffUi]]"
- >
- <table
- id="diffTable"
- class$="[[_diffTableClass]]"
- role="presentation"
- contenteditable$="[[isContentEditable]]"
- ></table>
+ <table
+ id="diffTable"
+ class$="[[_diffTableClass]]"
+ role="presentation"
+ contenteditable$="[[isContentEditable]]"
+ ></table>
- <template
- is="dom-if"
- if="[[showNoChangeMessage(_loading, prefs, _diffLength, diff)]]"
- >
- <div class="whitespace-change-only-message">
- This file only contains whitespace changes. Modify the whitespace
- setting to see the changes.
- </div>
- </template>
- </gr-diff-builder>
+ <template
+ is="dom-if"
+ if="[[showNoChangeMessage(_loading, prefs, _diffLength, diff)]]"
+ >
+ <div class="whitespace-change-only-message">
+ This file only contains whitespace changes. Modify the whitespace
+ setting to see the changes.
+ </div>
+ </template>
</div>
<div class$="[[_computeNewlineWarningClass(_newlineWarning, _loading)]]">
[[_newlineWarning]]
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.js b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.js
index c8b643d..130dab8 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.js
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.js
@@ -25,7 +25,6 @@
import {Side} from '../../../api/diff.js';
import {mockPromise, stubRestApi} from '../../../test/test-utils.js';
import {AbortStop} from '../../../api/core.js';
-import {afterNextRender} from '@polymer/polymer/lib/utils/render-status';
import {waitForEventOnce} from '../../../utils/event-util.js';
const basicFixture = fixtureFromElement('gr-diff');
@@ -72,7 +71,7 @@
test('cancel', () => {
element = basicFixture.instantiate();
- const cancelStub = sinon.stub(element.$.diffBuilder, 'cancel');
+ const cancelStub = sinon.stub(element.diffBuilder, 'cancel');
element.cancel();
assert.isTrue(cancelStub.calledOnce);
});
@@ -192,9 +191,6 @@
element.changeNum = 123;
element.patchRange = {basePatchNum: 1, patchNum: 2};
element.path = 'file.txt';
- element.$.diffBuilder.diff = createDiff();
- element.$.diffBuilder.prefs = {...MINIMAL_PREFS};
- element.$.diffBuilder._builder = element.$.diffBuilder._getDiffBuilder();
// No thread groups.
assert.equal(contentEl.querySelectorAll('.thread-group').length, 0);
@@ -263,7 +259,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.diffBuilder.builder, GrDiffBuilderImage);
// Left image rendered with the parent commit's version of the file.
const leftImage = element.$.diffTable.querySelector('td.left img');
@@ -321,7 +317,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.diffBuilder.builder, GrDiffBuilderImage);
// Left image rendered with the parent commit's version of the file.
const leftImage = element.$.diffTable.querySelector('td.left img');
@@ -381,7 +377,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.diffBuilder.builder, GrDiffBuilderImage);
const leftImage = element.$.diffTable.querySelector('td.left img');
const rightImage = element.$.diffTable.querySelector('td.right img');
@@ -417,7 +413,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.diffBuilder.builder, GrDiffBuilderImage);
const leftImage = element.$.diffTable.querySelector('td.left img');
const rightImage = element.$.diffTable.querySelector('td.right img');
@@ -455,7 +451,7 @@
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
- element.$.diffBuilder._builder, GrDiffBuilderImage);
+ element.diffBuilder.builder, GrDiffBuilderImage);
const leftImage = element.$.diffTable.querySelector('td.left img');
assert.isNotOk(leftImage);
});
@@ -602,7 +598,7 @@
ab: Array(13).fill('text'),
}];
setupSampleDiff({content});
- await new Promise(resolve => afterNextRender(element, resolve));
+ await waitForEventOnce(element, 'render');
element.appendChild(threadEl);
await flush();
@@ -775,9 +771,9 @@
setup(() => {
element = basicFixture.instantiate();
- renderStub = sinon.stub(element.$.diffBuilder, 'render').callsFake(
+ renderStub = sinon.stub(element.diffBuilder, 'render').callsFake(
() => {
- element.$.diffBuilder.dispatchEvent(
+ element.$.diffTable.dispatchEvent(
new CustomEvent('render', {bubbles: true, composed: true}));
return Promise.resolve({});
});
@@ -838,7 +834,7 @@
assert.equal(element.prefs.context, 3);
assert.equal(element._safetyBypass, -1);
- assert.equal(element.$.diffBuilder.prefs.context, -1);
+ assert.equal(element.diffBuilder.prefs.context, -1);
});
test('toggles collapse context from bypass', async () => {
@@ -851,7 +847,7 @@
assert.equal(element.prefs.context, 3);
assert.isNull(element._safetyBypass);
- assert.equal(element.$.diffBuilder.prefs.context, 3);
+ assert.equal(element.diffBuilder.prefs.context, 3);
});
test('toggles collapse context from pref using default', async () => {
@@ -863,7 +859,7 @@
assert.equal(element.prefs.context, -1);
assert.equal(element._safetyBypass, 10);
- assert.equal(element.$.diffBuilder.prefs.context, 10);
+ assert.equal(element.diffBuilder.prefs.context, 10);
});
});
@@ -874,7 +870,7 @@
test('unsetting', () => {
element.blame = [];
- const setBlameSpy = sinon.spy(element.$.diffBuilder, 'setBlame');
+ const setBlameSpy = sinon.spy(element.diffBuilder, 'setBlame');
element.classList.add('showBlame');
element.blame = null;
assert.isTrue(setBlameSpy.calledWithExactly(null));
@@ -976,7 +972,7 @@
setup(() => {
element = basicFixture.instantiate();
element.prefs = {};
- renderStub = sinon.stub(element.$.diffBuilder, 'render')
+ renderStub = sinon.stub(element.diffBuilder, 'render')
.returns(new Promise(() => {}));
});
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index 985bec1..ea7865e 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 {Key, Modifier} from '../utils/dom-util';
+import {afterNextRender, 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,6 +224,10 @@
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 16e0586..f2e0994 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -505,3 +505,14 @@
});
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);
+ });
+}
diff --git a/polygerrit-ui/app/utils/event-util.ts b/polygerrit-ui/app/utils/event-util.ts
index 418adbd..e624cef 100644
--- a/polygerrit-ui/app/utils/event-util.ts
+++ b/polygerrit-ui/app/utils/event-util.ts
@@ -32,7 +32,7 @@
);
}
-type HTMLElementEventDetailType<K extends keyof HTMLElementEventMap> =
+export type HTMLElementEventDetailType<K extends keyof HTMLElementEventMap> =
HTMLElementEventMap[K] extends CustomEvent<infer DT>
? unknown extends DT
? never
