Replace diff-builder rendering by plain Lit rendering
This is a vast simplification of how GrDiff is being rendered. There is
no need to explicitly request rendering of the diff. It will just happen
as part of Lit's usual rendering process.
This also means that lots of `init()` and `cleanup()` methods are
obsolete.
With this change the notion of a "diff builder" is gone. Apart from
merging the GrDiffBuilder into GrDiff we are also mergin the image diff
builder and the binary diff builder into GrDiff, see the new
`renderImageDiff()` and `renderBinaryDiff()` methods.
We are adding a `getUpdateComplete` pattern to gr-diff and
gr-diff-section that is already used elsewhere: It allows parents to
`await el.updateComplete` making sure that all children are also
already rendered. This allows us to remove `waitUntilRendered()` from
gr-diff-group.
Release-Notes: skip
Google-Bug-Id: b/280018960
Change-Id: I8dd98ab9ea35dbed6eb05a43071f9cf67594d013
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 771e298..f406215 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
@@ -6,9 +6,7 @@
import {BLANK_LINE, GrDiffLine} from './gr-diff-line';
import {GrDiffLineType, LineNumber, LineRange, Side} from '../../../api/diff';
import {assertIsDefined, assert} from '../../../utils/common-util';
-import {untilRendered} from '../../../utils/dom-util';
import {isDefined} from '../../../types/types';
-import {LitElement} from 'lit';
export enum GrDiffGroupType {
/** Unchanged context. */
@@ -318,11 +316,6 @@
*/
readonly keyLocation: boolean = false;
- /**
- * Once rendered the diff builder sets this to the diff section element.
- */
- element?: HTMLElement;
-
readonly lines: GrDiffLine[] = [];
readonly adds: GrDiffLine[] = [];
@@ -490,22 +483,6 @@
}
}
- async waitUntilRendered() {
- const lineNumber = this.lines[0]?.beforeNumber;
- // The LOST or FILE lines may be hidden and thus never resolve an
- // untilRendered() promise.
- if (
- this.skip !== undefined ||
- typeof lineNumber !== 'number' ||
- this.type === GrDiffGroupType.CONTEXT_CONTROL
- ) {
- return Promise.resolve();
- }
- assertIsDefined(this.element);
- await (this.element as LitElement).updateComplete;
- await untilRendered(this.element.firstElementChild as HTMLElement);
- }
-
/**
* Determines whether the group is either totally an addition or totally
* a removal.
@@ -517,4 +494,10 @@
!(!this.adds.length && !this.removes.length)
);
}
+
+ id() {
+ return `${this.type} ${this.startLine(Side.LEFT)} ${this.startLine(
+ Side.RIGHT
+ )}`;
+ }
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
index 0b688a6..cfb64b9 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
@@ -4,8 +4,9 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {BlameInfo, CommentRange} from '../../../types/common';
-import {Side} from '../../../constants/constants';
+import {Side, SpecialFilePath} from '../../../constants/constants';
import {
+ DiffContextExpandedExternalDetail,
DiffPreferencesInfo,
DiffResponsiveMode,
DisplayLine,
@@ -15,6 +16,7 @@
RenderPreferences,
} from '../../../api/diff';
import {getBaseUrl} from '../../../utils/url-util';
+import {GrDiffGroup} from './gr-diff-group';
/**
* In JS, unicode code points above 0xFFFF occupy two elements of a string.
@@ -227,6 +229,20 @@
return defaultContext;
}
+export function computeLineLength(
+ prefs: DiffPreferencesInfo,
+ path: string | undefined
+): number {
+ if (path === SpecialFilePath.COMMIT_MESSAGE) {
+ return 72;
+ }
+ const lineLength = prefs.line_length;
+ if (Number.isInteger(lineLength) && lineLength > 0) {
+ return lineLength;
+ }
+ return 100;
+}
+
export function computeKeyLocations(
lineOfInterest: DisplayLine | undefined,
comments: GrDiffCommentThread[]
@@ -474,3 +490,11 @@
return blameNode;
}
+
+export interface DiffContextExpandedEventDetail
+ extends DiffContextExpandedExternalDetail {
+ /** The context control group that should be replaced by `groups`. */
+ contextGroup: GrDiffGroup;
+ groups: GrDiffGroup[];
+ numLines: number;
+}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
index 28c6c08..639b1ac 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
@@ -18,8 +18,10 @@
computeContext,
FULL_CONTEXT,
FullContext,
+ computeLineLength,
} from './gr-diff-utils';
import {FILE, LOST, Side} from '../../../api/diff';
+import {createDefaultDiffPrefs} from '../../../constants/constants';
const LINE_BREAK_HTML = '<span class="gr-diff br"></span>';
@@ -207,6 +209,35 @@
});
});
+ suite('computeLineLength', () => {
+ test('computeLineLength(1, ...)', () => {
+ assert.equal(
+ computeLineLength(
+ {...createDefaultDiffPrefs(), line_length: 1},
+ 'a.txt'
+ ),
+ 1
+ );
+ assert.equal(
+ computeLineLength(
+ {...createDefaultDiffPrefs(), line_length: 1},
+ undefined
+ ),
+ 1
+ );
+ });
+
+ test('computeLineLength(1, "/COMMIT_MSG")', () => {
+ assert.equal(
+ computeLineLength(
+ {...createDefaultDiffPrefs(), line_length: 1},
+ '/COMMIT_MSG'
+ ),
+ 72
+ );
+ });
+ });
+
suite('key locations', () => {
test('lineOfInterest is a key location', () => {
const lineOfInterest = {lineNum: 789, side: Side.LEFT};
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 54d578a..cc49b61 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -11,6 +11,9 @@
import '../gr-syntax-themes/gr-syntax-theme';
import '../gr-ranged-comment-themes/gr-ranged-comment-theme';
import '../gr-ranged-comment-hint/gr-ranged-comment-hint';
+import '../gr-diff-builder/gr-diff-builder-image';
+import '../gr-diff-builder/gr-diff-section';
+import '../gr-diff-builder/gr-diff-row';
import {
getLine,
getLineElByChild,
@@ -27,8 +30,9 @@
getSideByLineEl,
compareComments,
toCommentThreadModel,
- KeyLocations,
FullContext,
+ diffClasses,
+ DiffContextExpandedEventDetail,
} from '../gr-diff/gr-diff-utils';
import {BlameInfo, CommentRange, ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
@@ -41,11 +45,11 @@
CommentRangeLayer,
GrRangedCommentLayer,
} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
-import {DiffViewMode, Side} from '../../../constants/constants';
import {
- GrDiffProcessor,
- ProcessingOptions,
-} from '../gr-diff-processor/gr-diff-processor';
+ DiffViewMode,
+ Side,
+ createDefaultDiffPrefs,
+} from '../../../constants/constants';
import {fire, fireAlert} from '../../../utils/event-util';
import {MovedLinkClickedEvent, ValueChangedEvent} from '../../../types/events';
import {getContentEditableRange} from '../../../utils/safari-selection-util';
@@ -56,16 +60,12 @@
DisplayLine,
LineNumber,
LOST,
+ ContentLoadNeededEventDetail,
} from '../../../api/diff';
import {isHtmlElement, isSafari, toggleClass} from '../../../utils/dom-util';
import {assertIsDefined} from '../../../utils/common-util';
-import {
- debounceP,
- DelayedPromise,
- DELAYED_CANCELLATION,
-} from '../../../utils/async-util';
import {GrDiffSelection} from '../gr-diff-selection/gr-diff-selection';
-import {property, query, state} from 'lit/decorators.js';
+import {property, query, queryAll, state} from 'lit/decorators.js';
import {sharedStyles} from '../../../styles/shared-styles';
import {html, LitElement, nothing, PropertyValues} from 'lit';
import {when} from 'lit/directives/when.js';
@@ -79,14 +79,6 @@
import {grDiffStyles} from './gr-diff-styles';
import {getDiffLength} from '../../../utils/diff-util';
import {GrCoverageLayer} from '../gr-coverage-layer/gr-coverage-layer';
-import {
- GrDiffBuilder,
- isImageDiffBuilder,
- isBinaryDiffBuilder,
- DiffContextExpandedEventDetail,
-} from '../gr-diff-builder/gr-diff-builder';
-import {GrDiffBuilderBinary} from '../gr-diff-builder/gr-diff-builder-binary';
-import {GrDiffBuilderImage} from '../gr-diff-builder/gr-diff-builder-image';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {
GrDiffGroup,
@@ -95,6 +87,9 @@
} from './gr-diff-group';
import {GrDiffLine} from './gr-diff-line';
import {subscribe} from '../../../elements/lit/subscription-controller';
+import {GrDiffSection} from '../gr-diff-builder/gr-diff-section';
+import {GrDiffRow} from '../gr-diff-builder/gr-diff-row';
+import {repeat} from 'lit/directives/repeat.js';
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
@@ -151,6 +146,9 @@
@query('#diffTable')
diffTable?: HTMLTableElement;
+ @queryAll('gr-diff-section')
+ diffSections?: NodeListOf<GrDiffSection>;
+
@property({type: Boolean})
noAutoRender = false;
@@ -233,10 +231,6 @@
@property({type: Boolean})
override isContentEditable = isSafari();
- // Private but used in tests.
- @state()
- showWarning?: boolean;
-
@property({type: String})
errorMessage: string | null = null;
@@ -256,18 +250,9 @@
@state()
diffLength?: number;
- /**
- * Observes comment nodes added or removed at any point.
- * Can be used to unregister upon detachment.
- */
+ /** Observes comment nodes added or removed at any point. */
private nodeObserver?: MutationObserver;
- @property({type: Array})
- layers?: DiffLayer[];
-
- // Private but used in tests.
- renderDiffTableTask?: DelayedPromise<void>;
-
// Private but used in tests.
diffSelection = new GrDiffSelection();
@@ -276,44 +261,39 @@
private diffModel = new DiffModel();
- // visible for testing
- builder?: GrDiffBuilder;
+ /**
+ * Just the layers that are passed in from the outside. See `layersAll`
+ * for an array of all layers.
+ */
+ @property({type: Array})
+ layers: DiffLayer[] = [];
/**
- * All layers, both from the outside and the default ones. See `layers` for
- * the property that can be set from the outside.
+ * Just the internal default layers. See `layers` for the property that can
+ * be set from the outside.
*/
- // visible for testing
- layersInternal: DiffLayer[] = [];
+ @state() layersInternal: DiffLayer[] = [];
- // visible for testing
- showTabs?: boolean;
-
- // visible for testing
- showTrailingWhitespace?: boolean;
+ /**
+ * All layers, just combines `layers` and `layersInternal`.
+ */
+ @state() layersAll: DiffLayer[] = [];
private coverageLayerLeft = new GrCoverageLayer(Side.LEFT);
private coverageLayerRight = new GrCoverageLayer(Side.RIGHT);
- private rangeLayer?: GrRangedCommentLayer;
+ private rangeLayer = new GrRangedCommentLayer();
- // TODO: Remove. Let the model instantiate the processor.
- // visible for testing
- processor?: GrDiffProcessor;
+ @state() groups: GrDiffGroup[] = [];
- /**
- * 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.
- */
- private groups: GrDiffGroup[] = [];
+ @state() private context = 3;
- // TODO: Can be removed when GrDiffProcessor is not instantiated anymore.
- private keyLocations: KeyLocations = {left: {}, right: {}};
-
- // TODO: Can be removed when GrDiffProcessor is not instantiated anymore.
- private context = 3;
+ private readonly layerUpdateListener: (
+ start: LineNumber,
+ end: LineNumber,
+ side: Side
+ ) => void;
static override get styles() {
return [
@@ -330,23 +310,32 @@
provide(this, diffModelToken, () => this.diffModel);
subscribe(
this,
- () => this.diffModel.keyLocations$,
- keyLocations => (this.keyLocations = keyLocations)
+ () => this.diffModel.context$,
+ context => (this.context = context)
);
subscribe(
this,
- () => this.diffModel.context$,
- context => (this.context = context)
+ () => this.diffModel.groups$,
+ groups => (this.groups = groups)
);
this.addEventListener(
'create-range-comment',
(e: CustomEvent<CreateRangeCommentEventDetail>) =>
this.handleCreateRangeComment(e)
);
- this.addEventListener('render-content', () => this.handleRenderContent());
this.addEventListener('moved-link-clicked', (e: MovedLinkClickedEvent) => {
this.dispatchSelectedLine(e.detail.lineNum, e.detail.side);
});
+ this.addEventListener(
+ 'diff-context-expanded-internal-new',
+ this.onDiffContextExpanded
+ );
+ this.layerUpdateListener = (
+ start: LineNumber,
+ end: LineNumber,
+ side: Side
+ ) => this.requestRowUpdates(start, end, side);
+ this.layersInternalInit();
}
override connectedCallback() {
@@ -360,15 +349,12 @@
if (this.diffTable) {
this.highlights.init(this.diffTable, this);
}
- this.diffBuilderInit();
}
override disconnectedCallback() {
this.removeSelectionListeners();
- this.renderDiffTableTask?.cancel();
this.diffSelection.cleanup();
this.highlights.cleanup();
- this.diffBuilderCleanup();
super.disconnectedCallback();
}
@@ -377,14 +363,19 @@
changedProperties.has('diff') ||
changedProperties.has('path') ||
changedProperties.has('renderPrefs') ||
+ changedProperties.has('viewMode') ||
changedProperties.has('prefs') ||
changedProperties.has('lineOfInterest')
) {
if (this.diff && this.prefs) {
+ const renderPrefs = {...(this.renderPrefs ?? {})};
+ if (renderPrefs.view_mode === undefined) {
+ renderPrefs.view_mode = this.viewMode;
+ }
this.diffModel.updateState({
diff: this.diff,
path: this.path,
- renderPrefs: this.renderPrefs ?? {},
+ renderPrefs,
diffPrefs: this.prefs,
lineOfInterest: this.lineOfInterest,
isImageDiff: this.isImageDiff,
@@ -400,6 +391,9 @@
) {
this.prefsChanged();
}
+ if (changedProperties.has('layers')) {
+ this.layersChanged();
+ }
if (changedProperties.has('blame')) {
this.blameChanged();
}
@@ -421,18 +415,37 @@
}
}
- protected override updated(changedProperties: PropertyValues<this>): void {
+ private async fireRenderContent() {
+ await this.updateComplete;
+ this.loading = false;
+ this.observeNodes();
+ // TODO: Retire one of these two events.
+ fire(this, 'render-content', {});
+ fire(this, 'render', {});
+ }
+
+ protected override async getUpdateComplete(): Promise<boolean> {
+ const result = await super.getUpdateComplete();
+ const sections = [...(this.diffSections ?? [])];
+ await Promise.all(sections.map(section => section.updateComplete));
+ return result;
+ }
+
+ protected override updated(changedProperties: PropertyValues<this>) {
if (changedProperties.has('diff')) {
- // diffChanged relies on diffTable ahving been rendered.
+ // diffChanged relies on diffTable having been rendered.
this.diffChanged();
}
+ if (changedProperties.has('groups')) {
+ if (this.groups?.length > 0) this.fireRenderContent();
+ }
}
override render() {
+ fire(this.diffTable, 'render-start', {});
return html`
${this.renderHeader()} ${this.renderContainer()}
${this.renderNewlineWarning()} ${this.renderLoadingError()}
- ${this.renderSizeWarning()}
`;
}
@@ -459,7 +472,19 @@
id="diffTable"
class=${this.diffTableClass}
?contenteditable=${this.isContentEditable}
- ></table>
+ >
+ ${this.renderColumns()}
+ ${when(!this.showWarning(), () =>
+ repeat(
+ this.groups,
+ group => group.id(),
+ group => this.renderSectionElement(group)
+ )
+ )}
+ ${when(this.diff?.binary, () =>
+ this.isImageDiff ? this.renderImageDiff() : this.renderBinaryDiff()
+ )}
+ </table>
${when(
this.showNoChangeMessage(),
() => html`
@@ -469,6 +494,7 @@
</div>
`
)}
+ ${when(this.showWarning(), () => this.renderSizeWarning())}
</div>
`;
}
@@ -485,7 +511,7 @@
}
private renderSizeWarning() {
- if (!this.showWarning) return nothing;
+ if (!this.showWarning()) return nothing;
// TODO: Update comment about 'Whole file' as it's not in settings.
return html`
<div id="sizeWarning">
@@ -596,7 +622,7 @@
});
}
- this.updateCommentRanges(this.commentRanges);
+ this.rangeLayer?.updateRanges(this.commentRanges);
}
// Dispatch events that are handled by the gr-diff-highlight.
@@ -612,11 +638,8 @@
});
}
- /** Cancel any remaining diff builder rendering work. */
- cancel() {
- this.diffBuilderCleanup();
- this.renderDiffTableTask?.cancel();
- }
+ /** TODO: Can be removed when diff-old is gone. */
+ cancel() {}
getCursorStops(): Array<HTMLElement | AbortStop> {
if (this.hidden && this.noAutoRender) return [];
@@ -641,7 +664,7 @@
}
private blameChanged() {
- this.setBlame(this.blame);
+ this.setBlame(this.blame ?? []);
if (this.blame) {
this.classList.add('showBlame');
} else {
@@ -759,22 +782,20 @@
this.unhideLine(lineNum, this.lineOfInterest.side);
}
- private cleanup() {
- this.cancel();
- this.blame = null;
- this.diffModel.updateState({showFullContext: FullContext.UNDECIDED});
- this.showWarning = false;
- this.clearDiffContent();
- }
-
private prefsChanged() {
if (!this.prefs) return;
this.blame = null;
this.updatePreferenceStyles();
- if (this.diff && !this.noRenderOnPrefsChange) {
- this.debounceRenderDiffTable();
+ if (!Number.isInteger(this.prefs.tab_size) || this.prefs.tab_size <= 0) {
+ this.handlePreferenceError('tab size');
+ }
+ if (
+ !Number.isInteger(this.prefs.line_length) ||
+ this.prefs.line_length <= 0
+ ) {
+ this.handlePreferenceError('diff width');
}
}
@@ -854,15 +875,12 @@
if (this.prefs) {
this.updatePreferenceStyles();
}
- this.updateRenderPrefs(this.renderPrefs);
}
private diffChanged() {
this.loading = true;
- this.cleanup();
if (this.diff) {
this.diffLength = this.getDiffLength(this.diff);
- this.debounceRenderDiffTable();
assertIsDefined(this.diffTable, 'diffTable');
this.diffSelection.init(this.diff, this.diffTable);
this.highlights.init(this.diffTable, this);
@@ -874,73 +892,23 @@
return getDiffLength(diff);
}
- /**
- * When called multiple times from the same task, will call
- * _renderDiffTable only once, in the next task (scheduled via `setTimeout`).
- *
- * This should be used instead of calling _renderDiffTable directly to
- * render the diff in response to an input change, because there may be
- * multiple inputs changing in the same microtask, but we only want to
- * render once.
- */
- private debounceRenderDiffTable() {
- // at this point gr-diff might be considered as rendered from the outside
- // (client), although it was not actually rendered. Clients need to know
- // when it is safe to perform operations like cursor moves, for example,
- // and if changing an input actually requires a reload of the diff table.
- // Since `fire` is synchronous it allows clients to be aware when an
- // async render is needed and that they can wait for a further `render`
- // event to actually take further action.
- fire(this, 'render-required', {});
- this.renderDiffTableTask = debounceP(
- this.renderDiffTableTask,
- async () => await this.renderDiffTable()
- );
- this.renderDiffTableTask.catch((e: unknown) => {
- if (e === DELAYED_CANCELLATION) return;
- throw e;
- });
- }
-
- // Private but used in tests.
- async renderDiffTable() {
- this.unobserveNodes();
- if (!this.diff || !this.prefs) {
- fire(this, 'render', {});
- return;
- }
- if (
- this.prefs.context === FULL_CONTEXT &&
+ private showWarning() {
+ return (
+ this.prefs?.context === FULL_CONTEXT &&
this.diffModel.getState().showFullContext === FullContext.UNDECIDED &&
this.diffLength &&
this.diffLength >= LARGE_DIFF_THRESHOLD_LINES
- ) {
- this.showWarning = true;
- fire(this, 'render', {});
- return;
- }
-
- this.showWarning = false;
-
- this.updateCommentRanges(this.commentRanges);
- this.updateCoverageRanges(this.coverageRanges);
- await this.legacyRender();
- }
-
- private handleRenderContent() {
- this.querySelectorAll('gr-ranged-comment-hint').forEach(element =>
- element.remove()
);
- this.loading = false;
- this.observeNodes();
- // We are just converting 'render-content' into 'render' here. Maybe we
- // should retire the 'render' event in favor of 'render-content'?
- fire(this, 'render', {});
}
+ /**
+ * This must be called once, but only after diff lines are rendered. Otherwise
+ * `processNodes()` will fail to lookup the HTML elements that it wants to
+ * manipulate.
+ */
private observeNodes() {
+ if (this.nodeObserver) return;
// First stop observing old nodes.
- this.unobserveNodes();
// Then introduce a Mutation observer that watches for children being added
// to gr-diff. If those children are `isThreadEl`, namely then they are
// processed.
@@ -1019,19 +987,6 @@
}
}
- private unobserveNodes() {
- if (this.nodeObserver) {
- this.nodeObserver.disconnect();
- this.nodeObserver = undefined;
- }
- // You only stop observing for comment thread elements when the diff is
- // completely rendered from scratch. And then comment thread elements
- // will be (re-)added *after* rendering is done. That is also when we
- // re-start observing. So it is appropriate to thoroughly clean up
- // everything that the observer is managing.
- this.commentRanges = [];
- }
-
private insertPortedCommentsWithoutRangeMessage(lostCell: Element) {
const existingMessage = lostCell.querySelector('div.lost-message');
if (existingMessage) return;
@@ -1047,11 +1002,8 @@
lostCell.insertBefore(div, lostCell.firstChild);
}
- clearDiffContent() {
- this.unobserveNodes();
- if (!this.diffTable) return;
- this.diffTable.innerHTML = '';
- }
+ /** TODO: Can be removed when diff-old is gone. */
+ clearDiffContent() {}
// Private but used in tests.
computeDiffHeaderItems() {
@@ -1071,13 +1023,10 @@
private handleFullBypass() {
this.diffModel.updateState({showFullContext: FullContext.YES});
- this.debounceRenderDiffTable();
}
private collapseContext() {
this.diffModel.updateState({showFullContext: FullContext.NO});
- // Uses the default context amount if the preference is for the entire file.
- this.debounceRenderDiffTable();
}
// TODO: Migrate callers to just update prefs.context.
@@ -1103,72 +1052,49 @@
return messages.join(' \u2014 '); // \u2014 - '—'
}
- private updateCommentRanges(ranges: CommentRangeLayer[]) {
- this.rangeLayer?.updateRanges(ranges);
- }
-
private updateCoverageRanges(rs: CoverageRange[]) {
this.coverageLayerLeft.setRanges(rs.filter(r => r?.side === Side.LEFT));
this.coverageLayerRight.setRanges(rs.filter(r => r?.side === Side.RIGHT));
}
- legacyRender(): Promise<void> {
- assertIsDefined(this.diff, 'diff');
- assertIsDefined(this.diffTable, 'diff table');
- assertIsDefined(this.prefs, 'prefs');
-
- // Setting up annotation layers must happen after plugins are
- // 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.showTabs = this.prefs.show_tabs;
- this.showTrailingWhitespace = this.prefs.show_whitespace_errors;
-
- this.diffBuilderCleanup();
- this.builder = this.getDiffBuilder();
- this.diffBuilderInit();
-
- this.diffTable.innerHTML = '';
- this.builder.addColumns(this.diffTable, getLineNumberCellWidth(this.prefs));
-
- const options: ProcessingOptions = {
- context: this.context,
- keyLocations: this.keyLocations,
- 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)
- .then(async () => {
- if (isImageDiffBuilder(this.builder)) {
- this.builder.renderImageDiff();
- } else if (isBinaryDiffBuilder(this.builder)) {
- this.builder.renderBinaryDiff();
- }
- await this.untilGroupsRendered();
- fire(this.diffTable, 'render-content', {});
- })
- // Mocha testing does not like uncaught rejections, so we catch
- // the cancels which are expected and should not throw errors in
- // tests.
- .catch(e => {
- if (!e.isCanceled) return Promise.reject(e);
- return;
- })
+ public renderImageDiff() {
+ return when(
+ this.useNewImageDiffUi,
+ () => this.renderImageDiffNew(),
+ () => this.renderImageDiffOld()
);
}
- // visible for testing
- async untilGroupsRendered(groups: readonly GrDiffGroup[] = this.groups) {
- return Promise.all(groups.map(g => g.waitUntilRendered()));
+ private renderImageDiffNew() {
+ const autoBlink = !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
+ return html`
+ <gr-diff-image-new
+ .automaticBlink=${autoBlink}
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-new>
+ `;
+ }
+
+ private renderImageDiffOld() {
+ return html`
+ <gr-diff-image-old
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-old>
+ `;
+ }
+
+ public renderBinaryDiff() {
+ return html`
+ <tbody class="gr-diff binary-diff">
+ <tr class="gr-diff">
+ <td colspan="5" class="gr-diff">
+ <span>Difference in binary files</span>
+ </td>
+ </tr>
+ </tbody>
+ `;
}
private onDiffContextExpanded = (
@@ -1176,14 +1102,19 @@
) => {
// Don't stop propagation. The host may listen for reporting or
// resizing.
- this.replaceGroup(e.detail.contextGroup, e.detail.groups);
+ this.diffModel.replaceGroup(e.detail.contextGroup, e.detail.groups);
};
- // visible for testing
- setupAnnotationLayers() {
- this.rangeLayer = new GrRangedCommentLayer();
+ private layersChanged() {
+ this.layersAll = [...this.layersInternal, ...this.layers];
+ for (const layer of this.layersAll) {
+ layer.removeListener?.(this.layerUpdateListener);
+ layer.addListener?.(this.layerUpdateListener);
+ }
+ }
- const layers: DiffLayer[] = [
+ private layersInternalInit() {
+ this.layersInternal = [
this.createTrailingWhitespaceLayer(),
this.createIntralineLayer(),
this.createTabIndicatorLayer(),
@@ -1192,16 +1123,7 @@
this.coverageLayerLeft,
this.coverageLayerRight,
];
-
- if (this.layers) {
- layers.push(...this.layers);
- }
- this.layersInternal = layers;
- }
-
- getContentTdByLine(lineNumber: LineNumber, side?: Side) {
- if (!this.builder) return undefined;
- return this.builder.getContentTdByLine(lineNumber, side);
+ this.layersChanged();
}
getContentTdByLineEl(lineEl?: Element): Element | undefined {
@@ -1212,21 +1134,6 @@
return this.getContentTdByLine(line, side);
}
- getLineElByNumber(lineNumber: LineNumber, side?: Side) {
- if (!this.builder) return undefined;
- return this.builder.getLineElByNumber(lineNumber, side);
- }
-
- getLineNumberRows() {
- if (!this.builder) return [];
- return this.builder.getLineNumberRows();
- }
-
- getLineNumEls(side: Side) {
- if (!this.builder) return [];
- return this.builder.getLineNumEls(side);
- }
-
/**
* When the line is hidden behind a context expander, expand it.
*
@@ -1237,8 +1144,7 @@
*/
unhideLine(lineNum: number, side: Side) {
assertIsDefined(this.prefs, 'prefs');
- if (!this.builder) return;
- const group = this.builder.findGroup(side, lineNum);
+ const group = this.findGroup(side, lineNum);
// Cannot unhide a line that is not part of the diff.
if (!group) return;
// If it's already visible, great!
@@ -1249,7 +1155,7 @@
const groups = hideInContextControl(
group.contextGroups,
0,
- lineOffset - 1 - this.prefs.context
+ lineOffset - 1 - this.context
);
// If there is a context group, it will be the first group because we
// start hiding from 0 offset
@@ -1259,68 +1165,14 @@
newGroups.push(
...hideInContextControl(
groups,
- lineOffset + 1 + this.prefs.context,
+ lineOffset + 1 + this.context,
// Both ends inclusive, so difference is the offset of the last line.
// But we need to pass the first line not to hide, which is the element
// after.
lineRange.end_line - lineRange.start_line + 1
)
);
- this.replaceGroup(group, newGroups);
- }
-
- /**
- * Replace the group of a context control section by rendering the provided
- * groups instead. This happens in response to expanding a context control
- * group.
- *
- * @param contextGroup The context control group to replace
- * @param newGroups The groups that are replacing the context control group
- */
- private replaceGroup(
- contextGroup: GrDiffGroup,
- newGroups: readonly GrDiffGroup[]
- ) {
- if (!this.builder) return;
- fire(this.diffTable, 'render-start', {});
- this.builder.replaceGroup(contextGroup, newGroups);
- this.groups = this.groups.filter(g => g !== contextGroup);
- this.groups.push(...newGroups);
- this.untilGroupsRendered(newGroups).then(() => {
- fire(this.diffTable, 'render-content', {});
- });
- }
-
- /**
- * This is meant to be called when the gr-diff component re-connects, or when
- * the diff is (re-)rendered.
- *
- * Make sure that this method is symmetric with cleanup(), which is called
- * when gr-diff disconnects.
- */
- private diffBuilderInit() {
- this.cleanup();
- this.diffTable?.addEventListener(
- 'diff-context-expanded-internal-new',
- this.onDiffContextExpanded
- );
- this.builder?.init();
- }
-
- /**
- * This is meant to be called when the gr-diff component disconnects, or when
- * the diff is (re-)rendered.
- *
- * Make sure that this method is symmetric with init(), which is called when
- * gr-diff re-connects.
- */
- private diffBuilderCleanup() {
- this.processor?.cancel();
- this.builder?.cleanup();
- this.diffTable?.removeEventListener(
- 'diff-context-expanded-internal-new',
- this.onDiffContextExpanded
- );
+ this.diffModel.replaceGroup(group, newGroups);
}
// visible for testing
@@ -1328,95 +1180,11 @@
const message =
`The value of the '${pref}' user preference is ` +
'invalid. Fix in diff preferences';
- assertIsDefined(this.diffTable, 'diff table');
- fireAlert(this.diffTable, message);
+ fireAlert(this, message);
throw Error(`Invalid preference value: ${pref}`);
}
// visible for testing
- getDiffBuilder(): GrDiffBuilder {
- assertIsDefined(this.diff, 'diff');
- assertIsDefined(this.diffTable, 'diff table');
- assertIsDefined(this.prefs, 'prefs');
- if (isNaN(this.prefs.tab_size) || this.prefs.tab_size <= 0) {
- this.handlePreferenceError('tab size');
- }
-
- if (isNaN(this.prefs.line_length) || this.prefs.line_length <= 0) {
- this.handlePreferenceError('diff width');
- }
-
- const localPrefs = {...this.prefs};
- if (this.path === COMMIT_MSG_PATH) {
- // override line_length for commit msg the same way as
- // in gr-diff
- localPrefs.line_length = COMMIT_MSG_LINE_LENGTH;
- }
-
- let builder = null;
- if (this.isImageDiff) {
- builder = new GrDiffBuilderImage(
- this.diff,
- localPrefs,
- this.diffTable,
- this.baseImage ?? null,
- this.revisionImage ?? null,
- this.renderPrefs,
- this.useNewImageDiffUi
- );
- } else if (this.diff.binary) {
- return new GrDiffBuilderBinary(this.diff, localPrefs, this.diffTable);
- } else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
- this.renderPrefs = {
- ...this.renderPrefs,
- view_mode: DiffViewMode.SIDE_BY_SIDE,
- };
- builder = new GrDiffBuilder(
- this.diff,
- localPrefs,
- this.diffTable,
- this.layersInternal,
- this.renderPrefs
- );
- } else if (this.viewMode === DiffViewMode.UNIFIED) {
- this.renderPrefs = {
- ...this.renderPrefs,
- view_mode: DiffViewMode.UNIFIED,
- };
- builder = new GrDiffBuilder(
- this.diff,
- localPrefs,
- this.diffTable,
- this.layersInternal,
- this.renderPrefs
- );
- }
- if (!builder) {
- throw Error(`Unsupported diff view mode: ${this.viewMode}`);
- }
- return builder;
- }
-
- /**
- * Called when the processor starts converting the diff information from the
- * server into chunks.
- */
- clearGroups() {
- if (!this.builder) return;
- this.groups = [];
- 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]);
- this.groups.push(group);
- }
-
- // visible for testing
createIntralineLayer(): DiffLayer {
return {
// Take a DIV.contentText element and a line object with intraline
@@ -1451,15 +1219,10 @@
// visible for testing
createTabIndicatorLayer(): DiffLayer {
- const show = () => this.showTabs;
+ const show = () => this.prefs?.show_tabs;
return {
annotate(contentEl: HTMLElement, _: HTMLElement, line: GrDiffLine) {
- // If visible tabs are disabled, do nothing.
- if (!show()) {
- return;
- }
-
- // Find and annotate the locations of tabs.
+ if (!show()) return;
annotateSymbols(contentEl, line, '\t', 'tab-indicator');
},
};
@@ -1483,14 +1246,10 @@
// visible for testing
createTrailingWhitespaceLayer(): DiffLayer {
- const show = () => this.showTrailingWhitespace;
-
+ const show = () => this.prefs?.show_whitespace_errors;
return {
annotate(contentEl: HTMLElement, _: HTMLElement, line: GrDiffLine) {
- if (!show()) {
- return;
- }
-
+ if (!show()) return;
const match = line.text.match(TRAILING_WHITESPACE_PATTERN);
if (match) {
// Normalize string positions in case there is unicode before or
@@ -1510,13 +1269,167 @@
};
}
- setBlame(blame: BlameInfo[] | null) {
- if (!this.builder) return;
- this.builder.setBlame(blame ?? []);
+ getContentTdByLine(
+ lineNumber: LineNumber,
+ side?: Side
+ ): HTMLTableCellElement | undefined {
+ if (!side) return undefined;
+ const row = this.findRow(side, lineNumber);
+ return row?.getContentCell(side);
}
- updateRenderPrefs(renderPrefs: RenderPreferences) {
- this.builder?.updateRenderPrefs(renderPrefs);
+ getLineElByNumber(
+ lineNumber: LineNumber,
+ side?: Side
+ ): HTMLTableCellElement | undefined {
+ if (!side) return undefined;
+ const row = this.findRow(side, lineNumber);
+ return row?.getLineNumberCell(side);
+ }
+
+ private findRow(side: Side, lineNumber: LineNumber): GrDiffRow | undefined {
+ const group = this.findGroup(side, lineNumber);
+ if (!group) return undefined;
+ const section = this.findSection(group);
+ if (!section) return undefined;
+ return section.findRow(side, lineNumber);
+ }
+
+ private getDiffRows() {
+ assertIsDefined(this.diffTable, 'diffTable');
+ const sections = [
+ ...this.diffTable.querySelectorAll<GrDiffSection>('gr-diff-section'),
+ ];
+ return sections.map(s => s.getDiffRows()).flat();
+ }
+
+ getLineNumberRows(): HTMLTableRowElement[] {
+ const rows = this.getDiffRows();
+ return rows.map(r => r.getTableRow()).filter(isDefined);
+ }
+
+ getLineNumEls(side: Side): HTMLTableCellElement[] {
+ const rows = this.getDiffRows();
+ return rows.map(r => r.getLineNumberCell(side)).filter(isDefined);
+ }
+
+ /** This is used when layers initiate an update. */
+ private requestRowUpdates(start: LineNumber, end: LineNumber, side: Side) {
+ const groups = this.getGroupsByLineRange(start, end, side);
+ for (const group of groups) {
+ const section = this.findSection(group);
+ for (const row of section?.getDiffRows() ?? []) {
+ row.requestUpdate();
+ }
+ }
+ }
+
+ private findSection(group: GrDiffGroup): GrDiffSection | undefined {
+ assertIsDefined(this.diffTable, 'diffTable');
+ const leftClass = `left-${group.startLine(Side.LEFT)}`;
+ const rightClass = `right-${group.startLine(Side.RIGHT)}`;
+ return (
+ this.diffTable.querySelector<GrDiffSection>(
+ `gr-diff-section.${leftClass}.${rightClass}`
+ ) ?? undefined
+ );
+ }
+
+ renderSectionElement(group: GrDiffGroup) {
+ const leftClass = `left-${group.startLine(Side.LEFT)}`;
+ const rightClass = `right-${group.startLine(Side.RIGHT)}`;
+ if (this.diff?.binary && group.startLine(Side.LEFT) === LOST) {
+ return nothing;
+ }
+ return html`
+ <gr-diff-section
+ class="${leftClass} ${rightClass}"
+ .group=${group}
+ .diff=${this.diff}
+ .layers=${this.layersAll}
+ .diffPrefs=${this.prefs}
+ .renderPrefs=${this.renderPrefs}
+ ></gr-diff-section>
+ `;
+ }
+
+ renderColumns() {
+ const lineNumberWidth = getLineNumberCellWidth(
+ this.prefs ?? createDefaultDiffPrefs()
+ );
+ return html`
+ <colgroup>
+ <col class=${diffClasses('blame')}></col>
+ ${when(
+ (this.renderPrefs?.view_mode ?? this.viewMode) ===
+ DiffViewMode.UNIFIED,
+ () => html` ${this.renderUnifiedColumns(lineNumberWidth)} `,
+ () => html`
+ ${this.renderSideBySideColumns(Side.LEFT, lineNumberWidth)}
+ ${this.renderSideBySideColumns(Side.RIGHT, lineNumberWidth)}
+ `
+ )}
+ </colgroup>
+ `;
+ }
+
+ private renderUnifiedColumns(lineNumberWidth: number) {
+ return html`
+ <col class=${diffClasses()} width=${lineNumberWidth}></col>
+ <col class=${diffClasses()} width=${lineNumberWidth}></col>
+ <col class=${diffClasses()}></col>
+ `;
+ }
+
+ private renderSideBySideColumns(side: Side, lineNumberWidth: number) {
+ return html`
+ <col class=${diffClasses(side)} width=${lineNumberWidth}></col>
+ <col class=${diffClasses(side, 'sign')}></col>
+ <col class=${diffClasses(side)}></col>
+ `;
+ }
+
+ findGroup(side: Side, line: LineNumber) {
+ return this.groups.find(group => group.containsLine(side, line));
+ }
+
+ // visible for testing
+ getGroupsByLineRange(
+ startLine: LineNumber,
+ endLine: LineNumber,
+ side: Side
+ ): GrDiffGroup[] {
+ const startIndex = this.groups.findIndex(group =>
+ group.containsLine(side, startLine)
+ );
+ if (startIndex === -1) return [];
+ let endIndex = this.groups.findIndex(group =>
+ group.containsLine(side, endLine)
+ );
+ // Not all groups may have been processed yet (i.e. this.groups is still
+ // incomplete). In that case let's just return *all* groups until the end
+ // of the array.
+ if (endIndex === -1) endIndex = this.groups.length - 1;
+ // The filter preserves the legacy behavior to only return non-context
+ // groups
+ return this.groups
+ .slice(startIndex, endIndex + 1)
+ .filter(group => group.lines.length > 0);
+ }
+
+ /**
+ * Set the blame information for the diff. For any already-rendered line,
+ * re-render its blame cell content.
+ */
+ setBlame(blame: BlameInfo[]) {
+ for (const blameInfo of blame) {
+ for (const range of blameInfo.ranges) {
+ for (let line = range.start; line <= range.end; line++) {
+ const row = this.findRow(Side.LEFT, line);
+ if (row) row.blameInfo = blameInfo;
+ }
+ }
+ }
}
}
@@ -1577,5 +1490,7 @@
* renders and for partial rerenders.
*/
'render-content': CustomEvent<{}>;
+ 'diff-context-expanded-internal-new': CustomEvent<DiffContextExpandedEventDetail>;
+ 'content-load-needed': CustomEvent<ContentLoadNeededEventDetail>;
}
}
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 5603edb..8806dbf 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
@@ -30,7 +30,6 @@
queryAndAssert,
stubBaseUrl,
stubRestApi,
- waitEventLoop,
waitQueryAndAssert,
waitUntil,
} from '../../../test/test-utils';
@@ -39,10 +38,8 @@
import {GrDiff} from './gr-diff';
import {ImageInfo} from '../../../types/common';
import {GrRangedCommentHint} from '../gr-ranged-comment-hint/gr-ranged-comment-hint';
-import {assertIsDefined} from '../../../utils/common-util';
import {fixture, html, assert} from '@open-wc/testing';
import {createDefaultDiffPrefs} from '../../../constants/constants';
-import {GrDiffBuilder} from '../gr-diff-builder/gr-diff-builder';
import {GrDiffRow} from '../gr-diff-builder/gr-diff-row';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {GrDiffLine} from './gr-diff-line';
@@ -77,7 +74,17 @@
element,
/* HTML */ `
<div class="diffContainer sideBySide">
- <table id="diffTable"></table>
+ <table id="diffTable">
+ <colgroup>
+ <col class="blame gr-diff" />
+ <col class="gr-diff left" width="48" />
+ <col class="gr-diff left sign" />
+ <col class="gr-diff left" />
+ <col class="gr-diff right" width="48" />
+ <col class="gr-diff right sign" />
+ <col class="gr-diff right" />
+ </colgroup>
+ </table>
</div>
`
);
@@ -3163,7 +3170,6 @@
<col class="gr-diff right sign" />
<col class="gr-diff right" />
</colgroup>
- <tbody class="binary-diff gr-diff"></tbody>
<tbody class="both gr-diff section">
<tr
aria-labelledby="left-button-FILE left-content-FILE right-button-FILE right-content-FILE"
@@ -3305,13 +3311,13 @@
<td class="blank gr-diff left lineNum"></td>
<td class="gr-diff left">
<label class="gr-diff">
- <span class="gr-diff label"> image/bmp </span>
+ <span class="gr-diff label"> 1×1 image/bmp </span>
</label>
</td>
<td class="blank gr-diff lineNum right"></td>
<td class="gr-diff right">
<label class="gr-diff">
- <span class="gr-diff label"> image/bmp </span>
+ <span class="gr-diff label"> 1×1 image/bmp </span>
</label>
</td>
</tr>
@@ -3369,7 +3375,7 @@
<label class="gr-diff">
<span class="gr-diff name"> carrot.jpg </span>
<br class="gr-diff" />
- <span class="gr-diff label"> image/bmp </span>
+ <span class="gr-diff label"> 1×1 image/bmp </span>
</label>
`
);
@@ -3379,7 +3385,7 @@
<label class="gr-diff">
<span class="gr-diff name"> carrot2.jpg </span>
<br class="gr-diff" />
- <span class="gr-diff label"> image/bmp </span>
+ <span class="gr-diff label"> 1×1 image/bmp </span>
</label>
`
);
@@ -3538,7 +3544,6 @@
ignore_whitespace: 'IGNORE_NONE',
};
await element.updateComplete;
- element.renderDiffTable();
}
test('returns [] when hidden and noAutoRender', async () => {
@@ -3554,6 +3559,7 @@
test('returns one stop per line and one for the file row', async () => {
await setupDiff();
element.loading = false;
+ await waitUntil(() => element.groups.length > 2);
await element.updateComplete;
const ROWS = 48;
const FILE_ROW = 1;
@@ -3567,10 +3573,12 @@
test('returns an additional AbortStop when still loading', async () => {
await setupDiff();
element.loading = true;
+ await waitUntil(() => element.groups.length > 2);
await element.updateComplete;
const ROWS = 48;
const FILE_ROW = 1;
const LOST_ROW = 1;
+ element.loading = true;
const actual = element.getCursorStops();
assert.equal(actual.length, ROWS + FILE_ROW + LOST_ROW + 1);
assert.isTrue(actual[actual.length - 1] instanceof AbortStop);
@@ -3694,67 +3702,6 @@
assert.isEmpty(element.querySelectorAll('gr-ranged-comment-hint'));
});
-
- suite('change in preferences', () => {
- setup(async () => {
- element.diff = {
- meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
- meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 560},
- diff_header: [],
- intraline_status: 'OK',
- change_type: 'MODIFIED',
- content: [{skip: 66}],
- };
- await element.updateComplete;
- await element.renderDiffTableTask?.flush();
- });
-
- test('change in preferences re-renders diff', async () => {
- const stub = sinon.stub(element, 'renderDiffTable');
- element.prefs = {
- ...MINIMAL_PREFS,
- };
- await element.updateComplete;
- await element.renderDiffTableTask?.flush();
- assert.isTrue(stub.called);
- });
-
- test('adding/removing property in preferences re-renders diff', async () => {
- const stub = sinon.stub(element, 'renderDiffTable');
- const newPrefs1: DiffPreferencesInfo = {
- ...MINIMAL_PREFS,
- line_wrapping: true,
- };
- element.prefs = newPrefs1;
- await element.updateComplete;
- await element.renderDiffTableTask?.flush();
- assert.isTrue(stub.called);
- stub.reset();
-
- const newPrefs2 = {...newPrefs1};
- delete newPrefs2.line_wrapping;
- element.prefs = newPrefs2;
- await element.updateComplete;
- await element.renderDiffTableTask?.flush();
- assert.isTrue(stub.called);
- });
-
- test(
- 'change in preferences does not re-renders diff with ' +
- 'noRenderOnPrefsChange',
- async () => {
- const stub = sinon.stub(element, 'renderDiffTable');
- element.noRenderOnPrefsChange = true;
- element.prefs = {
- ...MINIMAL_PREFS,
- context: 12,
- };
- await element.updateComplete;
- await element.renderDiffTableTask?.flush();
- assert.isFalse(stub.called);
- }
- );
- });
});
suite('diff header', () => {
@@ -3808,7 +3755,7 @@
element.classList.add('showBlame');
element.blame = null;
await element.updateComplete;
- assert.isTrue(setBlameSpy.calledWithExactly(null));
+ assert.isTrue(setBlameSpy.calledWithExactly([]));
assert.isFalse(element.classList.contains('showBlame'));
});
@@ -3916,36 +3863,10 @@
content,
binary,
};
+ await waitUntil(() => element.groups.length > 1);
await element.updateComplete;
- await element.renderDiffTableTask;
};
- test('clear diff table content as soon as diff changes', async () => {
- const content = [
- {
- a: ['all work and no play make andybons a dull boy'],
- },
- {
- b: ['Non eram nescius, Brute, cum, quae summis ingeniis '],
- },
- ];
- function diffTableHasContent() {
- assertIsDefined(element.diffTable);
- return element.diffTable.innerText.includes(content[0].a?.[0] ?? '');
- }
- await setupSampleDiff({content});
- await waitUntil(diffTableHasContent);
- element.diff = {...element.diff!};
- await element.updateComplete;
- // immediately cleaned up
- assertIsDefined(element.diffTable);
- assert.equal(element.diffTable.innerHTML, '');
- element.renderDiffTable();
- await element.updateComplete;
- // rendered again
- await waitUntil(diffTableHasContent);
- });
-
suite('selection test', () => {
test('user-select set correctly on side-by-side view', async () => {
const content = [
@@ -3961,8 +3882,9 @@
},
];
await setupSampleDiff({content});
- await waitEventLoop();
+ // We are selecting "Non eram nescius..." on the left side.
+ // The default is `selected-right`, so we will have to click.
const diffLine = queryAll<HTMLElement>(element, '.contentText')[2];
assert.equal(getComputedStyle(diffLine).userSelect, 'none');
mouseDown(diffLine);
@@ -3982,10 +3904,12 @@
],
},
];
- await setupSampleDiff({content});
element.viewMode = DiffViewMode.UNIFIED;
- await element.updateComplete;
- const diffLine = queryAll<HTMLElement>(element, '.contentText')[2];
+ await setupSampleDiff({content});
+
+ // We are selecting "all work and no play..." on the left side.
+ // The default is `selected-right`, so we will have to click.
+ const diffLine = queryAll<HTMLElement>(element, '.contentText')[0];
assert.equal(getComputedStyle(diffLine).userSelect, 'none');
mouseDown(diffLine);
assert.equal(getComputedStyle(diffLine).userSelect, 'text');
@@ -4057,16 +3981,6 @@
suite('former gr-diff-builder tests', () => {
let element: GrDiff;
- let builder: GrDiffBuilder;
- let diffTable: HTMLTableElement;
-
- const setBuilderPrefs = (prefs: Partial<DiffPreferencesInfo>) => {
- builder = new GrDiffBuilder(
- createEmptyDiff(),
- {...createDefaultDiffPrefs(), ...prefs},
- diffTable
- );
- };
const line = (text: string) => {
const line = new GrDiffLine(GrDiffLineType.BOTH);
@@ -4081,51 +3995,6 @@
stubRestApi('getLoggedIn').returns(Promise.resolve(false));
stubRestApi('getProjectConfig').returns(Promise.resolve(createConfig()));
stubBaseUrl('/r');
- setBuilderPrefs({});
- });
-
- [DiffViewMode.UNIFIED, DiffViewMode.SIDE_BY_SIDE].forEach(mode => {
- test(`line_length used for regular files under ${mode}`, () => {
- element.path = '/a.txt';
- element.viewMode = mode;
- element.diff = createEmptyDiff();
- element.prefs = {
- ...createDefaultDiffPrefs(),
- tab_size: 4,
- line_length: 50,
- };
- builder = element.getDiffBuilder();
- assert.equal(builder.prefs.line_length, 50);
- });
-
- test(`line_length ignored for commit msg under ${mode}`, () => {
- element.path = '/COMMIT_MSG';
- element.viewMode = mode;
- element.diff = createEmptyDiff();
- element.prefs = {
- ...createDefaultDiffPrefs(),
- tab_size: 4,
- line_length: 50,
- };
- builder = element.getDiffBuilder();
- assert.equal(builder.prefs.line_length, 72);
- });
- });
-
- test('_handlePreferenceError throws with invalid preference', () => {
- element.prefs = {...createDefaultDiffPrefs(), tab_size: 0};
- assert.throws(() => element.getDiffBuilder());
- });
-
- test('_handlePreferenceError triggers alert and javascript error', () => {
- const errorStub = sinon.stub();
- element.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. " +
- 'Fix in diff preferences'
- );
});
suite('intraline differences', () => {
@@ -4276,7 +4145,7 @@
const lineNumberEl = document.createElement('td');
setup(() => {
- element.showTabs = true;
+ element.prefs = {...DEFAULT_PREFS, show_tabs: true};
layer = element.createTabIndicatorLayer();
});
@@ -4320,7 +4189,7 @@
});
test('does not annotate when disabled', () => {
- element.showTabs = false;
+ element.prefs = {...DEFAULT_PREFS, show_tabs: false};
const str = '\tlorem upsum';
const l = line(str);
@@ -4375,44 +4244,15 @@
});
});
- suite('layers', () => {
- let initialLayersCount = 0;
- let withLayerCount = 0;
- setup(() => {
- const layers: DiffLayer[] = [];
- element.layers = layers;
- element.showTrailingWhitespace = true;
- element.setupAnnotationLayers();
- initialLayersCount = element.layersInternal.length;
- });
-
- test('no layers', () => {
- element.setupAnnotationLayers();
- assert.equal(element.layersInternal.length, initialLayersCount);
- });
-
- suite('with layers', () => {
- const layers: DiffLayer[] = [{annotate: () => {}}, {annotate: () => {}}];
- setup(() => {
- element.layers = layers;
- element.showTrailingWhitespace = true;
- element.setupAnnotationLayers();
- withLayerCount = element.layersInternal.length;
- });
- test('with layers', () => {
- element.setupAnnotationLayers();
- assert.equal(element.layersInternal.length, withLayerCount);
- assert.equal(initialLayersCount + layers.length, withLayerCount);
- });
- });
- });
-
suite('trailing whitespace', () => {
let layer: DiffLayer;
const lineNumberEl = document.createElement('td');
setup(() => {
- element.showTrailingWhitespace = true;
+ element.prefs = {
+ ...createDefaultDiffPrefs(),
+ show_whitespace_errors: true,
+ };
layer = element.createTrailingWhitespaceLayer();
});
@@ -4483,7 +4323,10 @@
});
test('does not annotate when disabled', () => {
- element.showTrailingWhitespace = false;
+ element.prefs = {
+ ...createDefaultDiffPrefs(),
+ show_whitespace_errors: false,
+ };
const str = 'lorem upsum\t \t ';
const l = line(str);
const el = document.createElement('div');
@@ -4520,29 +4363,47 @@
test('text', async () => {
element.diff = {...createEmptyDiff(), content};
- await waitForEventOnce(element.diffTable!, 'render-content');
- assert.equal(querySelectorAll(element.diffTable!, 'tbody')?.length, 4);
+ await waitUntil(() => element.groups.length > 2);
+ await element.updateComplete;
+ const bodies = [...(querySelectorAll(element.diffTable!, 'tbody') ?? [])];
+ assert.equal(bodies.length, 4);
+ assert.isTrue(bodies[0].innerHTML.includes('LOST'));
+ assert.isTrue(bodies[1].innerHTML.includes('FILE'));
+ assert.isTrue(bodies[2].innerHTML.includes('andybons a dull boy'));
+ assert.isTrue(bodies[3].innerHTML.includes('Non eram nescius'));
});
test('image', async () => {
element.diff = {...createEmptyDiff(), content, binary: true};
element.isImageDiff = true;
- await waitForEventOnce(element.diffTable!, 'render-content');
- assert.equal(querySelectorAll(element.diffTable!, 'tbody')?.length, 4);
+ await element.updateComplete;
+ const body = queryAndAssert(element, 'tbody.image-diff');
+ assert.lightDom.equal(
+ body,
+ /* HTML */ `
+ <label class="gr-diff">
+ <span class="gr-diff label"> No image </span>
+ </label>
+ <label class="gr-diff">
+ <span class="gr-diff label"> No image </span>
+ </label>
+ `
+ );
});
test('binary', async () => {
element.diff = {...createEmptyDiff(), content, binary: true};
- await waitForEventOnce(element.diffTable!, 'render-content');
- assert.equal(querySelectorAll(element.diffTable!, 'tbody')?.length, 3);
+ await element.updateComplete;
+ const body = queryAndAssert(element, 'tbody.binary-diff');
+ assert.lightDom.equal(
+ body,
+ /* HTML */ '<span>Difference in binary files</span>'
+ );
});
});
suite('context hiding and expanding', () => {
- let dispatchStub: sinon.SinonStub;
-
setup(async () => {
- dispatchStub = sinon.stub(element.diffTable!, 'dispatchEvent');
element.diff = {
...createEmptyDiff(),
content: [
@@ -4552,15 +4413,12 @@
],
};
element.viewMode = DiffViewMode.SIDE_BY_SIDE;
-
element.prefs = {
...DEFAULT_PREFS,
context: 1,
};
+ await waitUntil(() => element.groups.length > 2);
await element.updateComplete;
- element.legacyRender();
- // Make sure all listeners are installed.
- await element.untilGroupsRendered();
});
test('hides lines behind two context controls', () => {
@@ -4617,7 +4475,6 @@
});
test('unhideLine shows the line with context', async () => {
- dispatchStub.reset();
element.unhideLine(4, Side.LEFT);
await waitUntil(() => {
@@ -4642,10 +4499,6 @@
assert.include(diffRows[8].textContent, 'before');
assert.include(diffRows[8].textContent, 'after');
assert.include(diffRows[9].textContent, 'unchanged 11');
-
- await element.untilGroupsRendered();
- const firedEventTypes = dispatchStub.getCalls().map(c => c.args[0].type);
- assert.include(firedEventTypes, 'render-content');
});
});
});
