Use the same comment data for the ranged comment layer as diff model
And further reduce the node observer complexity.
We are removing the `commentRanges` property from gr-diff.ts and the
rather complicated way of computing it in `updateRanges()`. Instead
in `processNodes()` we are calling `this.rangeLayer.updateRanges()`
with the same comment objects that are fed into the diff model.
We are also adding the comment metadata as event detail to
`comment-thread-mouseenter` and `comment-thread-mouseleave` events.
That allows us to get rid of `getThreadEl()` in gr-diff-highlight.
Release-Notes: skip
Change-Id: I3314a7a691f7500ffa5023c6b7279acc9e6f338d
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 cfb64b9..c41dc91 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
@@ -163,8 +163,9 @@
}
/**
- * This is all the data that gr-diff extracts from comment thread elements.
- * Otherwise gr-diff treats such elements as a black box.
+ * This is all the data that gr-diff extracts from comment thread elements,
+ * see `GrDiffThreadElement`. Otherwise gr-diff treats such elements as a black
+ * box.
*/
export interface GrDiffCommentThread {
side: Side;
@@ -173,8 +174,12 @@
rootId?: string;
}
-export function toCommentThreadModel(
- threadEl: HTMLElement
+/**
+ * Retrieves all the data from a comment thread element that the gr-diff API
+ * contract defines for such elements.
+ */
+export function getDataFromCommentThreadEl(
+ threadEl?: EventTarget | null
): GrDiffCommentThread | undefined {
if (!isThreadEl(threadEl)) return undefined;
const side = getSide(threadEl);
@@ -300,14 +305,19 @@
// For Gerrit these are instances of GrCommentThread, but other gr-diff users
// have different HTML elements in use for comment threads.
// TODO: Also document the required HTML attributes that thread elements must
-// have, e.g. 'diff-side', 'range', 'line-num'.
+// have, e.g. 'diff-side', 'range' (optional), 'line-num'.
+// Comment widgets are also required to have `comment-thread` in their css
+// class list.
export interface GrDiffThreadElement extends HTMLElement {
rootId: string;
}
-export function isThreadEl(node: Node): node is GrDiffThreadElement {
+export function isThreadEl(
+ node?: Node | EventTarget | null
+): node is GrDiffThreadElement {
return (
- node.nodeType === Node.ELEMENT_NODE &&
+ !!node &&
+ (node as Node).nodeType === Node.ELEMENT_NODE &&
(node as Element).classList.contains('comment-thread')
);
}
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 639b1ac..44f4f60 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
@@ -12,7 +12,7 @@
getRange,
computeKeyLocations,
GrDiffCommentThread,
- toCommentThreadModel,
+ getDataFromCommentThreadEl,
compareComments,
GrDiffThreadElement,
computeContext,
@@ -290,7 +290,7 @@
el.setAttribute('line-num', '3');
el.rootId = 'ab12';
- assert.deepEqual(toCommentThreadModel(el), {
+ assert.deepEqual(getDataFromCommentThreadEl(el), {
line: 3,
side: Side.LEFT,
range: undefined,
@@ -306,7 +306,7 @@
el.setAttribute('diff-side', 'left');
el.rootId = 'ab12';
- assert.deepEqual(toCommentThreadModel(el), {
+ assert.deepEqual(getDataFromCommentThreadEl(el), {
line: FILE,
side: Side.LEFT,
range: undefined,
@@ -318,11 +318,11 @@
const el = document.createElement(
'div'
) as unknown as GrDiffThreadElement;
- assert.isUndefined(toCommentThreadModel(el));
+ assert.isUndefined(getDataFromCommentThreadEl(el));
el.className = 'comment-thread';
- assert.isUndefined(toCommentThreadModel(el));
+ assert.isUndefined(getDataFromCommentThreadEl(el));
el.setAttribute('line-num', '3');
- assert.isUndefined(toCommentThreadModel(el));
+ assert.isUndefined(getDataFromCommentThreadEl(el));
});
});
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 0b37bd4..09dab26 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -17,19 +17,16 @@
import '../gr-diff-builder/gr-diff-row';
import {
getLineNumber,
- getRange,
- getSide,
- GrDiffThreadElement,
isThreadEl,
- rangesEqual,
getResponsiveMode,
isResponsive,
isNewDiff,
getSideByLineEl,
compareComments,
- toCommentThreadModel,
+ getDataFromCommentThreadEl,
FullContext,
DiffContextExpandedEventDetail,
+ GrDiffCommentThread,
} from '../gr-diff/gr-diff-utils';
import {BlameInfo, CommentRange, ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
@@ -38,10 +35,7 @@
GrDiffHighlight,
} from '../gr-diff-highlight/gr-diff-highlight';
import {CoverageRange, DiffLayer, isDefined} from '../../../types/types';
-import {
- CommentRangeLayer,
- GrRangedCommentLayer,
-} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
+import {GrRangedCommentLayer} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
import {DiffViewMode, Side} from '../../../constants/constants';
import {fire, fireAlert} from '../../../utils/event-util';
import {MovedLinkClickedEvent, ValueChangedEvent} from '../../../types/events';
@@ -54,7 +48,7 @@
LineNumber,
ContentLoadNeededEventDetail,
} from '../../../api/diff';
-import {isHtmlElement, isSafari, toggleClass} from '../../../utils/dom-util';
+import {isSafari, toggleClass} from '../../../utils/dom-util';
import {assertIsDefined} from '../../../utils/common-util';
import {GrDiffSelection} from '../gr-diff-selection/gr-diff-selection';
import {property, query, state} from 'lit/decorators.js';
@@ -161,10 +155,6 @@
@property({type: Boolean})
noRenderOnPrefsChange?: boolean;
- // Private but used in tests.
- @state()
- commentRanges: CommentRangeLayer[] = [];
-
// explicitly highlight a range if it is not associated with any comment
@property({type: Object})
highlightRange?: CommentRange;
@@ -468,63 +458,22 @@
: document.getSelection();
}
- private updateRanges(
- addedThreadEls: GrDiffThreadElement[],
- removedThreadEls: GrDiffThreadElement[]
- ) {
- function commentRangeFromThreadEl(
- threadEl: GrDiffThreadElement
- ): CommentRangeLayer | undefined {
- const side = getSide(threadEl);
- if (!side) return undefined;
- const range = getRange(threadEl);
- if (!range) return undefined;
+ private commentThreadRedispatcher = (
+ target: EventTarget | null,
+ eventName: 'comment-thread-mouseenter' | 'comment-thread-mouseleave'
+ ) => {
+ if (!isThreadEl(target)) return;
+ const data = getDataFromCommentThreadEl(target);
+ if (data) fire(target, eventName, data);
+ };
- return {side, range, rootId: threadEl.rootId};
- }
+ private commentThreadEnterRedispatcher = (e: Event) => {
+ this.commentThreadRedispatcher(e.target, 'comment-thread-mouseenter');
+ };
- // TODO(brohlfs): Rewrite `.map().filter() as ...` with `.reduce()` instead.
- const addedCommentRanges = addedThreadEls
- .map(commentRangeFromThreadEl)
- .filter(range => !!range) as CommentRangeLayer[];
- const removedCommentRanges = removedThreadEls
- .map(commentRangeFromThreadEl)
- .filter(range => !!range) as CommentRangeLayer[];
- for (const removedCommentRange of removedCommentRanges) {
- const i = this.commentRanges.findIndex(
- cr =>
- cr.side === removedCommentRange.side &&
- rangesEqual(cr.range, removedCommentRange.range)
- );
- this.commentRanges.splice(i, 1);
- }
-
- if (addedCommentRanges?.length) {
- this.commentRanges.push(...addedCommentRanges);
- }
- if (this.highlightRange) {
- this.commentRanges.push({
- side: Side.RIGHT,
- range: this.highlightRange,
- rootId: '',
- });
- }
-
- this.rangeLayer?.updateRanges(this.commentRanges);
- }
-
- // Dispatch events that are handled by the gr-diff-highlight.
- private redispatchHoverEvents(
- hoverEl: HTMLElement,
- threadEl: GrDiffThreadElement
- ) {
- hoverEl.addEventListener('mouseenter', () => {
- fire(threadEl, 'comment-thread-mouseenter', {});
- });
- hoverEl.addEventListener('mouseleave', () => {
- fire(threadEl, 'comment-thread-mouseleave', {});
- });
- }
+ private commentThreadLeaveRedispatcher = (e: Event) => {
+ this.commentThreadRedispatcher(e.target, 'comment-thread-mouseleave');
+ };
/** TODO: Can be removed when diff-old is gone. */
cancel() {}
@@ -739,39 +688,33 @@
* 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.
+ *
+ * TODO: Validate whether the above comment is still true. We don't look up
+ * elements anymore, and processing the nodes earlier might be beneficial
+ * performance wise.
*/
private observeNodes() {
if (this.nodeObserver) return;
- // First stop observing old nodes.
- // Then introduce a Mutation observer that watches for children being added
- // to gr-diff. If those children are `isThreadEl`, namely then they are
- // processed.
- this.nodeObserver = new MutationObserver(mutations => {
- const addedThreadEls = extractAddedNodes(mutations).filter(isThreadEl);
- const removedThreadEls =
- extractRemovedNodes(mutations).filter(isThreadEl);
- this.processNodes(addedThreadEls, removedThreadEls);
- });
+ // Watches children being added to gr-diff. We are expecting only comment
+ // widgets to be direct children.
+ this.nodeObserver = new MutationObserver(() => this.processNodes());
this.nodeObserver.observe(this, {childList: true});
- // Make sure to process existing gr-comment-threads that already exist.
- this.processNodes([...this.childNodes].filter(isThreadEl), []);
+ // Process existing comment widgets before the first observed change.
+ this.processNodes();
}
- private processNodes(
- addedThreadEls: GrDiffThreadElement[],
- removedThreadEls: GrDiffThreadElement[]
- ) {
- this.diffModel.updateState({
- comments: [...this.childNodes]
- .filter(isHtmlElement)
- .map(toCommentThreadModel)
- .filter(isDefined)
- .sort(compareComments),
- });
- this.updateRanges(addedThreadEls, removedThreadEls);
- addedThreadEls.forEach(threadEl =>
- this.redispatchHoverEvents(threadEl, threadEl)
- );
+ private processNodes() {
+ const threadEls = [...this.childNodes].filter(isThreadEl);
+ const comments = threadEls
+ .map(getDataFromCommentThreadEl)
+ .filter(isDefined)
+ .sort(compareComments);
+ this.diffModel.updateState({comments});
+ this.rangeLayer.updateRanges(comments);
+ for (const el of threadEls) {
+ el.addEventListener('mouseenter', this.commentThreadEnterRedispatcher);
+ el.addEventListener('mouseleave', this.commentThreadLeaveRedispatcher);
+ }
}
/** TODO: Can be removed when diff-old is gone. */
@@ -1070,14 +1013,6 @@
}
}
-function extractAddedNodes(mutations: MutationRecord[]) {
- return mutations.flatMap(mutation => [...mutation.addedNodes]);
-}
-
-function extractRemovedNodes(mutations: MutationRecord[]) {
- return mutations.flatMap(mutation => [...mutation.removedNodes]);
-}
-
function getLineNumberCellWidth(prefs: DiffPreferencesInfo) {
return prefs.font_size * 4;
}
@@ -1113,8 +1048,8 @@
'gr-diff': LitElement;
}
interface HTMLElementEventMap {
- 'comment-thread-mouseenter': CustomEvent<{}>;
- 'comment-thread-mouseleave': CustomEvent<{}>;
+ 'comment-thread-mouseenter': CustomEvent<GrDiffCommentThread>;
+ 'comment-thread-mouseleave': CustomEvent<GrDiffCommentThread>;
'loading-changed': ValueChangedEvent<boolean>;
'render-required': CustomEvent<{}>;
/**
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 d65aba8..917decc 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
@@ -319,35 +319,6 @@
1
);
});
-
- test('removes long range comment hint when comment is discarded', async () => {
- const range = {
- start_line: 1,
- end_line: 7,
- start_character: 0,
- end_character: 0,
- };
- const threadEl = document.createElement('div');
- threadEl.className = 'comment-thread';
- threadEl.setAttribute('diff-side', 'right');
- threadEl.setAttribute('line-num', '1');
- threadEl.setAttribute('range', JSON.stringify(range));
- threadEl.setAttribute('slot', 'right-1');
- const content = [
- {
- ab: Array(8).fill('text'),
- },
- ];
- await setupSampleDiff({content});
-
- element.appendChild(threadEl);
- await waitUntil(() => element.commentRanges.length === 1);
-
- threadEl.remove();
- await waitUntil(() => element.commentRanges.length === 0);
-
- assert.isEmpty(element.querySelectorAll('gr-ranged-comment-hint'));
- });
});
suite('blame', () => {
