Remove a TODO about checking `comment-side` in thread elements
All users of gr-diff are using `diff-side` attribute.
Release-Notes: skip
Change-Id: I4d836ebba6eebfb45a2b20ebb874f8743968951b
diff --git a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
index d9f5a4d..92cf968 100644
--- a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
+++ b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
@@ -86,8 +86,6 @@
@property({type: Object}) diff?: DiffInfo;
- @property({type: Object}) section?: HTMLElement;
-
@property({type: Object}) group?: GrDiffGroup;
@property({type: String, reflect: true})
@@ -361,10 +359,9 @@
lineRange: this.group.lineRange,
});
} else {
- assertIsDefined(this.section, 'section');
fire(this, 'diff-context-expanded', {
+ contextGroup: this.group,
groups,
- section: this.section!,
numLines: this.numLines(),
buttonType: type,
expandedLines: linesToExpand,
@@ -491,7 +488,7 @@
}
private hasValidProperties() {
- return !!(this.diff && this.section && this.group?.contextGroups?.length);
+ return !!(this.diff && this.group?.contextGroups?.length);
}
override render() {
diff --git a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts
index 92debda..7af40ed 100644
--- a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts
@@ -34,7 +34,6 @@
element = document.createElement('gr-context-controls');
element.diff = {content: []} as any as DiffInfo;
element.renderPreferences = {};
- element.section = document.createElement('div');
blankFixture.instantiate().appendChild(element);
await flush();
});
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 14277f8..6854ef34 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
@@ -146,8 +146,15 @@
@property({type: Object})
_builder?: DiffBuilder;
- // This is written to only from the processor via property notify
- // And then passed to the builder via a property observer.
+ /**
+ * 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[] = [];
@@ -206,7 +213,7 @@
(e: CustomEvent<DiffContextExpandedEventDetail>) => {
// Don't stop propagation. The host may listen for reporting or
// resizing.
- this.rerenderSection(e.detail.groups, e.detail.section);
+ this.replaceGroup(e.detail.contextGroup, e.detail.groups);
}
);
});
@@ -353,12 +360,9 @@
*/
unhideLine(lineNum: number, side: Side) {
if (!this._builder) return;
- const groupIndex = this.$.processor.groups.findIndex(group =>
- group.containsLine(side, lineNum)
- );
+ const group = this._builder.findGroup(side, lineNum);
// Cannot unhide a line that is not part of the diff.
- if (groupIndex < 0) return;
- const group = this._groups[groupIndex];
+ if (!group) return;
// If it's already visible, great!
if (group.type !== GrDiffGroupType.CONTEXT_CONTROL) return;
const lineRange = group.lineRange[side];
@@ -384,25 +388,24 @@
lineRange.end_line - lineRange.start_line + 1
)
);
- this._builder.spliceGroups(groupIndex, 1, ...newGroups);
+ this._builder.replaceGroup(group, newGroups);
setTimeout(() => fireEvent(this, 'render-content'), 1);
}
/**
- * Replace the provided section by rendering the provided groups.
+ * 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 newGroups The groups to be rendered in the place of the section.
- * @param sectionEl The context section that should be expanded from.
+ * @param contextGroup The context control group to replace
+ * @param newGroups The groups that are replacing the context control group
*/
- private rerenderSection(
- newGroups: readonly GrDiffGroup[],
- sectionEl: HTMLElement
+ private replaceGroup(
+ contextGroup: GrDiffGroup,
+ newGroups: readonly GrDiffGroup[]
) {
if (!this._builder) return;
-
- const contextIndex = this._builder.getIndexOfSection(sectionEl);
- this._builder.spliceGroups(contextIndex, 1, ...newGroups);
-
+ this._builder.replaceGroup(contextGroup, newGroups);
setTimeout(() => fireEvent(this, 'render-content'), 1);
}
@@ -482,18 +485,28 @@
this.diffElement.innerHTML = '';
}
+ /**
+ * Forward groups added by the processor to the builder for rendering.
+ */
@observe('_groups.splices')
_groupsChanged(changeRecord: PolymerSpliceChange<GrDiffGroup[]>) {
- if (!changeRecord || !this._builder) {
+ if (!changeRecord || !this._builder) return;
+
+ // The processor either removes all groups or adds new ones to the end,
+ // so let's simplify the Polymer splices.
+ const isRemoval = changeRecord.indexSplices.find(
+ splice => splice.removed.length > 0
+ );
+ if (isRemoval) {
+ this._builder.clearGroups();
return;
}
- // Forward any splices to the builder
for (const splice of changeRecord.indexSplices) {
const added = splice.object.slice(
splice.index,
splice.index + splice.addedCount
);
- this._builder.spliceGroups(splice.index, splice.removed.length, ...added);
+ this._builder.addGroups(added);
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
index a4fd7d2..7758aa0 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
@@ -197,13 +197,7 @@
section.appendChild(paddingRow);
}
section.appendChild(
- this.createContextControlRow(
- section,
- group,
- showAbove,
- showBelow,
- viewMode
- )
+ this.createContextControlRow(group, showAbove, showBelow, viewMode)
);
if (showBelow) {
const paddingRow = this.createContextControlPaddingRow(viewMode);
@@ -218,7 +212,6 @@
* method up or down into the area of code that they affect.
*/
private createContextControlRow(
- section: HTMLElement,
group: GrDiffGroup,
showAbove: boolean,
showBelow: boolean,
@@ -252,7 +245,6 @@
) as GrContextControls;
contextControls.diff = this._diff;
contextControls.renderPreferences = this.renderPrefs;
- contextControls.section = section;
contextControls.group = group;
contextControls.showConfig = showConfig;
cell.appendChild(contextControls);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
index 930731c..8b7a308 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
@@ -30,8 +30,9 @@
export interface DiffContextExpandedEventDetail
extends DiffContextExpandedExternalDetail {
+ /** The context control group that should be replaced by `groups`. */
+ contextGroup: GrDiffGroup;
groups: GrDiffGroup[];
- section: HTMLElement;
numLines: number;
}
@@ -49,12 +50,13 @@
*/
export interface DiffBuilder {
clear(): void;
- spliceGroups(
- start: number,
- deleteCount: number,
- ...addedGroups: GrDiffGroup[]
- ): GrDiffGroup[];
- getIndexOfSection(sectionEl: HTMLElement): number;
+ addGroups(groups: readonly GrDiffGroup[]): void;
+ clearGroups(): void;
+ replaceGroup(
+ contextControl: GrDiffGroup,
+ groups: readonly GrDiffGroup[]
+ ): void;
+ findGroup(side: Side, line: LineNumber): GrDiffGroup | undefined;
addColumns(outputEl: HTMLElement, fontSize: number): void;
// TODO: Change `null` to `undefined`.
getContentTdByLine(
@@ -77,9 +79,8 @@
*
* The builder takes GrDiffGroups, and builds the corresponding DOM elements,
* called sections. Only the builder should add or remove sections from the
- * DOM. Callers can use the spliceGroups method to add groups that
- * will then be rendered - or remove groups whose sections will then be
- * removed from the DOM.
+ * DOM. Callers can use the ...group() methods to modify groups and thus cause
+ * rendering changes.
*
* TODO: Do not subclass `GrDiffBuilder`. Use composition and interfaces.
*/
@@ -94,7 +95,7 @@
protected readonly outputEl: HTMLElement;
- protected readonly groups: GrDiffGroup[];
+ protected groups: GrDiffGroup[];
private blameInfo: BlameInfo[] = [];
@@ -155,41 +156,41 @@
protected abstract buildSectionElement(group: GrDiffGroup): HTMLElement;
- getIndexOfSection(sectionEl: HTMLElement) {
- return this.groups.findIndex(group => group.element === sectionEl);
+ addGroups(groups: readonly GrDiffGroup[]) {
+ for (const group of groups) {
+ this.groups.push(group);
+ this.emitGroup(group);
+ }
}
- spliceGroups(
- start: number,
- deleteCount: number,
- ...addedGroups: GrDiffGroup[]
- ) {
- // TODO: Change `null` to `undefined`.
- const sectionBeforeWhichToInsert =
- start < this.groups.length ? this.groups[start].element ?? null : null;
- // Update the groups array
- const deletedGroups = this.groups.splice(
- start,
- deleteCount,
- ...addedGroups
- );
-
- // Add new sections for the new groups
- for (const addedGroup of addedGroups) {
- this.emitGroup(addedGroup, sectionBeforeWhichToInsert);
+ clearGroups() {
+ for (const deletedGroup of this.groups) {
+ deletedGroup.element?.remove();
}
- // Remove sections corresponding to deleted groups from the DOM
- for (const deletedGroup of deletedGroups) {
- const section = deletedGroup.element;
- section?.parentNode?.removeChild(section);
- }
- return deletedGroups;
+ this.groups = [];
}
- // TODO: Change `null` to `undefined`.
- private emitGroup(group: GrDiffGroup, beforeSection: HTMLElement | null) {
+ replaceGroup(contextControl: GrDiffGroup, groups: readonly GrDiffGroup[]) {
+ const i = this.groups.indexOf(contextControl);
+ if (i === -1) throw new Error('cannot find context control group');
+
+ const contextControlSection = this.groups[i].element;
+ if (!contextControlSection) throw new Error('diff group element not set');
+
+ this.groups.splice(i, 1, ...groups);
+ for (const group of groups) {
+ this.emitGroup(group, contextControlSection);
+ }
+ contextControlSection?.remove();
+ }
+
+ findGroup(side: Side, line: LineNumber) {
+ return this.groups.find(group => group.containsLine(side, line));
+ }
+
+ private emitGroup(group: GrDiffGroup, beforeSection?: HTMLElement) {
const element = this.buildSectionElement(group);
- this.outputEl.insertBefore(element, beforeSection);
+ this.outputEl.insertBefore(element, beforeSection ?? null);
group.element = element;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 4d8efed..b588bc7 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -97,6 +97,13 @@
@property({type: Number})
context = 3;
+ /**
+ * The builder elements watches this (two-way data binding and @observe) and
+ * thus passes each added group on to the renderer (i.e. gr-diff-builder).
+ * You must only add to this array and not modify it later (only when
+ * resetting). The source of truth is then held by gr-diff-builder, which also
+ * reflects expanding and collapsing of groups.
+ */
@property({type: Array, notify: true})
groups: GrDiffGroup[] = [];
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 56c5d79..b8262e0 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
@@ -320,6 +320,9 @@
*/
readonly keyLocation: boolean = false;
+ /**
+ * Once rendered the diff builder sets this to the diff section element.
+ */
element?: HTMLElement;
readonly lines: GrDiffLine[] = [];
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 94a7a84..182d48e 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
@@ -146,10 +146,7 @@
}
export function getSide(threadEl: HTMLElement): Side | undefined {
- // TODO(dhruvsri): Remove check for comment-side once all users of gr-diff
- // start setting diff-side
- const sideAtt =
- threadEl.getAttribute('diff-side') || threadEl.getAttribute('comment-side');
+ const sideAtt = threadEl.getAttribute('diff-side');
if (!sideAtt) {
console.warn('comment thread without side');
return undefined;
