Move computation of key locations into diff model
This is a preparation of change 373057. We want to the model to be
responsible for diff processing. So it needs to control the key
locations, which are one of the processing inputs.
We are also moving the information about currently attached comment
widgets into the model, which will be valuable in other changes. So
you have one place to look up information about potentially existing
comments. See change 373406 for how this will be used later.
Google-Bug-Id: b/280018663
Release-Notes: skip
Change-Id: I877a732ac508bc49d559177d4dc62385930306b1
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-model/gr-diff-model.ts b/polygerrit-ui/app/embed/diff/gr-diff-model/gr-diff-model.ts
index 8fbda14..d2e997c 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-model/gr-diff-model.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-model/gr-diff-model.ts
@@ -8,18 +8,26 @@
import {
DiffInfo,
DiffPreferencesInfo,
+ DisplayLine,
RenderPreferences,
} from '../../../api/diff';
import {define} from '../../../models/dependency';
import {Model} from '../../../models/model';
import {isDefined} from '../../../types/types';
import {select} from '../../../utils/observable-util';
+import {
+ GrDiffCommentThread,
+ KeyLocations,
+ computeKeyLocations,
+} from '../gr-diff/gr-diff-utils';
export interface DiffState {
diff: DiffInfo;
path?: string;
renderPrefs: RenderPreferences;
diffPrefs: DiffPreferencesInfo;
+ lineOfInterest?: DisplayLine;
+ comments: GrDiffCommentThread[];
}
export const diffModelToken = define<DiffModel>('diff-model');
@@ -44,4 +52,10 @@
this.state$.pipe(filter(isDefined)),
diffState => diffState.diffPrefs
);
+
+ readonly keyLocations$: Observable<KeyLocations> = select(
+ this.state$.pipe(filter(isDefined)),
+ diffState =>
+ computeKeyLocations(diffState.lineOfInterest, diffState.comments ?? [])
+ );
}
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 6346573..483a4da 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
@@ -15,6 +15,7 @@
import {assert} from '../../../utils/common-util';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {FILE, GrDiffLineType, LineNumber} from '../../../api/diff';
+import {KeyLocations} from '../gr-diff/gr-diff-utils';
const WHOLE_FILE = -1;
@@ -32,11 +33,6 @@
keyLocation: boolean;
}
-export interface KeyLocations {
- left: {[key: string]: boolean};
- right: {[key: string]: boolean};
-}
-
/**
* The maximum size for an addition or removal chunk before it is broken down
* into a series of chunks that are this size at most.
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 e1de348..d309556 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
@@ -8,6 +8,7 @@
import {
DiffPreferencesInfo,
DiffResponsiveMode,
+ DisplayLine,
FILE,
LOST,
LineNumber,
@@ -159,6 +160,87 @@
return range;
}
+/**
+ * This is all the data that gr-diff extracts from comment thread elements.
+ * Otherwise gr-diff treats such elements as a black box.
+ */
+export interface GrDiffCommentThread {
+ side: Side;
+ line: LineNumber;
+ range?: CommentRange;
+ rootId?: string;
+}
+
+export function toCommentThreadModel(
+ threadEl: HTMLElement
+): GrDiffCommentThread | undefined {
+ if (!isThreadEl(threadEl)) return undefined;
+ const side = getSide(threadEl);
+ const line = getLine(threadEl);
+ const range = getRange(threadEl);
+ if (!side) return undefined;
+ if (!line) return undefined;
+ return {side, line, range, rootId: threadEl.rootId};
+}
+
+export interface KeyLocations {
+ left: {[key: string]: boolean};
+ right: {[key: string]: boolean};
+}
+
+export function computeKeyLocations(
+ lineOfInterest: DisplayLine | undefined,
+ comments: GrDiffCommentThread[]
+) {
+ const keyLocations: KeyLocations = {left: {}, right: {}};
+
+ if (lineOfInterest) {
+ keyLocations[lineOfInterest.side][lineOfInterest.lineNum] = true;
+ }
+
+ for (const comment of comments) {
+ keyLocations[comment.side][comment.line] = true;
+ if (comment.range?.start_line) {
+ keyLocations[comment.side][comment.range.start_line] = true;
+ }
+ }
+
+ return keyLocations;
+}
+
+export function compareComments(
+ c1: GrDiffCommentThread,
+ c2: GrDiffCommentThread
+): number {
+ if (c1.side !== c2.side) {
+ return c1.side === Side.RIGHT ? 1 : -1;
+ }
+
+ if (c1.line !== c2.line) {
+ if (c1.line === FILE && c2.line !== FILE) return -1;
+ if (c1.line !== FILE && c2.line === FILE) return 1;
+ if (c1.line === LOST && c2.line !== LOST) return -1;
+ if (c1.line !== LOST && c2.line === LOST) return 1;
+ return (c1.line as number) - (c2.line as number);
+ }
+
+ if (c1.rootId !== c2.rootId) {
+ if (!c1.rootId) return -1;
+ if (!c2.rootId) return 1;
+ return c1.rootId > c2.rootId ? 1 : -1;
+ }
+
+ if (c1.range && c2.range) {
+ const r1 = JSON.stringify(c1.range);
+ const r2 = JSON.stringify(c2.range);
+ return r1 > r2 ? 1 : -1;
+ }
+ if (c1.range) return 1;
+ if (c2.range) return -1;
+
+ return 0;
+}
+
// TODO: This type should be exposed to gr-diff clients in a separate type file.
// For Gerrit these are instances of GrCommentThread, but other gr-diff users
// have different HTML elements in use for comment threads.
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 6549230..7e6e7fc 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
@@ -10,7 +10,13 @@
formatText,
createTabWrapper,
getRange,
+ computeKeyLocations,
+ GrDiffCommentThread,
+ toCommentThreadModel,
+ compareComments,
+ GrDiffThreadElement,
} from './gr-diff-utils';
+import {FILE, LOST, Side} from '../../../api/diff';
const LINE_BREAK_HTML = '<span class="gr-diff br"></span>';
@@ -177,4 +183,151 @@
threadEl.setAttribute('slot', 'right-1');
assert.isUndefined(getRange(threadEl));
});
+
+ suite('key locations', () => {
+ test('lineOfInterest is a key location', () => {
+ const lineOfInterest = {lineNum: 789, side: Side.LEFT};
+ assert.deepEqual(computeKeyLocations(lineOfInterest, []), {
+ left: {789: true},
+ right: {},
+ });
+ });
+
+ test('line comments are key locations', async () => {
+ const comments: GrDiffCommentThread[] = [{side: Side.RIGHT, line: 3}];
+ assert.deepEqual(computeKeyLocations(undefined, comments), {
+ left: {},
+ right: {3: true},
+ });
+ });
+
+ test('file comments are key locations', async () => {
+ const comments: GrDiffCommentThread[] = [{side: Side.LEFT, line: FILE}];
+ assert.deepEqual(computeKeyLocations(undefined, comments), {
+ left: {FILE: true},
+ right: {},
+ });
+ });
+
+ test('lots of key locations', () => {
+ const lineOfInterest = {lineNum: 789, side: Side.LEFT};
+ const comments: GrDiffCommentThread[] = [
+ {side: Side.LEFT, line: FILE},
+ {side: Side.LEFT, line: 2},
+ {side: Side.LEFT, line: 111},
+ {side: Side.RIGHT, line: LOST},
+ {side: Side.RIGHT, line: 13},
+ {side: Side.RIGHT, line: 19},
+ ];
+ assert.deepEqual(computeKeyLocations(lineOfInterest, comments), {
+ left: {FILE: true, 2: true, 111: true, 789: true},
+ right: {LOST: true, 13: true, 19: true},
+ });
+ });
+ });
+
+ suite('toCommentThreadModel', () => {
+ test('simple example', () => {
+ const el = document.createElement(
+ 'div'
+ ) as unknown as GrDiffThreadElement;
+ el.className = 'comment-thread';
+ el.setAttribute('diff-side', 'left');
+ el.setAttribute('line-num', '3');
+ el.rootId = 'ab12';
+
+ assert.deepEqual(toCommentThreadModel(el), {
+ line: 3,
+ side: Side.LEFT,
+ range: undefined,
+ rootId: 'ab12',
+ });
+ });
+
+ test('FILE default', () => {
+ const el = document.createElement(
+ 'div'
+ ) as unknown as GrDiffThreadElement;
+ el.className = 'comment-thread';
+ el.setAttribute('diff-side', 'left');
+ el.rootId = 'ab12';
+
+ assert.deepEqual(toCommentThreadModel(el), {
+ line: FILE,
+ side: Side.LEFT,
+ range: undefined,
+ rootId: 'ab12',
+ });
+ });
+
+ test('undefined', () => {
+ const el = document.createElement(
+ 'div'
+ ) as unknown as GrDiffThreadElement;
+ assert.isUndefined(toCommentThreadModel(el));
+ el.className = 'comment-thread';
+ assert.isUndefined(toCommentThreadModel(el));
+ el.setAttribute('line-num', '3');
+ assert.isUndefined(toCommentThreadModel(el));
+ });
+ });
+
+ suite('compare comments', () => {
+ test('sort array of comments', () => {
+ const comments: GrDiffCommentThread[] = [
+ {side: Side.RIGHT, line: 3},
+ {side: Side.RIGHT, line: 2},
+ {side: Side.RIGHT, line: 1},
+ {side: Side.RIGHT, line: LOST},
+ {side: Side.RIGHT, line: FILE},
+ {side: Side.LEFT, line: 3},
+ {side: Side.LEFT, line: 2},
+ {
+ side: Side.LEFT,
+ line: 1,
+ rootId: 'b',
+ range: {
+ start_line: 1,
+ start_character: 0,
+ end_line: 5,
+ end_character: 14,
+ },
+ },
+ {
+ side: Side.LEFT,
+ line: 1,
+ rootId: 'b',
+ range: {
+ start_line: 1,
+ start_character: 0,
+ end_line: 2,
+ end_character: 4,
+ },
+ },
+ {side: Side.LEFT, line: 1, rootId: 'b'},
+ {side: Side.LEFT, line: 1, rootId: 'a'},
+ {side: Side.LEFT, line: 1},
+ {side: Side.LEFT, line: LOST},
+ ];
+ const commentsOrdered: GrDiffCommentThread[] = [
+ comments[12],
+ comments[11],
+ comments[10],
+ comments[9],
+ comments[8],
+ comments[7],
+ comments[6],
+ comments[5],
+ comments[4],
+ comments[3],
+ comments[2],
+ comments[1],
+ comments[0],
+ ];
+ assert.sameOrderedMembers(
+ comments.sort(compareComments),
+ commentsOrdered
+ );
+ });
+ });
});
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 32ffc73..1f212b0 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -25,6 +25,9 @@
isResponsive,
isNewDiff,
getSideByLineEl,
+ compareComments,
+ toCommentThreadModel,
+ KeyLocations,
} from '../gr-diff/gr-diff-utils';
import {BlameInfo, CommentRange, ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
@@ -32,7 +35,7 @@
CreateRangeCommentEventDetail,
GrDiffHighlight,
} from '../gr-diff-highlight/gr-diff-highlight';
-import {CoverageRange, DiffLayer} from '../../../types/types';
+import {CoverageRange, DiffLayer, isDefined} from '../../../types/types';
import {
CommentRangeLayer,
GrRangedCommentLayer,
@@ -44,7 +47,6 @@
} from '../../../constants/constants';
import {
GrDiffProcessor,
- KeyLocations,
ProcessingOptions,
} from '../gr-diff-processor/gr-diff-processor';
import {fire, fireAlert} from '../../../utils/event-util';
@@ -58,7 +60,7 @@
LineNumber,
LOST,
} from '../../../api/diff';
-import {isSafari, toggleClass} from '../../../utils/dom-util';
+import {isHtmlElement, isSafari, toggleClass} from '../../../utils/dom-util';
import {assertIsDefined} from '../../../utils/common-util';
import {
debounceP,
@@ -95,6 +97,7 @@
hideInContextControl,
} from './gr-diff-group';
import {GrDiffLine} from './gr-diff-line';
+import {subscribe} from '../../../elements/lit/subscription-controller';
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
@@ -319,6 +322,8 @@
*/
private groups: GrDiffGroup[] = [];
+ private keyLocations: KeyLocations = {left: {}, right: {}};
+
static override get styles() {
return [
iconStyles,
@@ -332,6 +337,11 @@
constructor() {
super();
provide(this, diffModelToken, () => this.diffModel);
+ subscribe(
+ this,
+ () => this.diffModel.keyLocations$,
+ keyLocations => (this.keyLocations = keyLocations)
+ );
this.addEventListener(
'create-range-comment',
(e: CustomEvent<CreateRangeCommentEventDetail>) =>
@@ -368,6 +378,21 @@
protected override willUpdate(changedProperties: PropertyValues<this>): void {
if (
+ changedProperties.has('diff') ||
+ changedProperties.has('path') ||
+ changedProperties.has('renderPrefs') ||
+ changedProperties.has('prefs') ||
+ changedProperties.has('lineOfInterest')
+ ) {
+ this.diffModel.updateState({
+ diff: this.diff,
+ path: this.path,
+ renderPrefs: this.renderPrefs,
+ diffPrefs: this.prefs,
+ lineOfInterest: this.lineOfInterest,
+ });
+ }
+ if (
changedProperties.has('path') ||
changedProperties.has('lineWrapping') ||
changedProperties.has('viewMode') ||
@@ -575,35 +600,6 @@
this.updateCommentRanges(this.commentRanges);
}
- /**
- * The key locations based on the comments and line of interests,
- * where lines should not be collapsed.
- *
- */
- // visible for testing
- computeKeyLocations() {
- const keyLocations: KeyLocations = {left: {}, right: {}};
- if (this.lineOfInterest) {
- const side = this.lineOfInterest.side;
- keyLocations[side][this.lineOfInterest.lineNum] = true;
- }
- const threadEls = [...this.childNodes].filter(isThreadEl);
-
- for (const threadEl of threadEls) {
- const side = getSide(threadEl);
- if (!side) continue;
- const lineNum = getLine(threadEl);
- const commentRange = getRange(threadEl);
- keyLocations[side][lineNum] = true;
- // Add start_line as well if exists,
- // the being and end of the range should not be collapsed.
- if (commentRange?.start_line) {
- keyLocations[side][commentRange.start_line] = true;
- }
- }
- return keyLocations;
- }
-
// Dispatch events that are handled by the gr-diff-highlight.
private redispatchHoverEvents(
hoverEl: HTMLElement,
@@ -774,7 +770,6 @@
private prefsChanged() {
if (!this.prefs) return;
- this.diffModel.updateState({diffPrefs: this.prefs});
this.blame = null;
this.updatePreferenceStyles();
@@ -845,7 +840,6 @@
}
private renderPrefsChanged() {
- this.diffModel.updateState({renderPrefs: this.renderPrefs});
if (this.renderPrefs.hide_left_side) {
this.classList.add('no-left');
}
@@ -929,13 +923,6 @@
this.showWarning = false;
- this.diffModel.setState({
- diff: this.diff,
- path: this.path,
- renderPrefs: this.renderPrefs,
- diffPrefs: this.prefs,
- });
-
this.updateCommentRanges(this.commentRanges);
this.updateCoverageRanges(this.coverageRanges);
await this.legacyRender();
@@ -973,6 +960,13 @@
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)
@@ -1159,7 +1153,7 @@
const options: ProcessingOptions = {
context: this.getBypassPrefs().context,
- keyLocations: this.computeKeyLocations(),
+ keyLocations: this.keyLocations,
isBinary: !!(this.isImageDiff || this.diff.binary),
};
if (this.renderPrefs?.num_lines_rendered_at_once) {
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 d542633..99db4e9 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
@@ -3964,48 +3964,6 @@
});
});
- suite('key locations', () => {
- setup(async () => {
- element.prefs = {...MINIMAL_PREFS};
- element.diff = createDiff();
- await element.updateComplete;
- });
-
- test('lineOfInterest is a key location', () => {
- element.lineOfInterest = {lineNum: 789, side: Side.LEFT};
- assert.deepEqual(element.computeKeyLocations(), {
- left: {789: true},
- right: {},
- });
- });
-
- test('line comments are key locations', async () => {
- const threadEl = document.createElement('div');
- threadEl.className = 'comment-thread';
- threadEl.setAttribute('diff-side', 'right');
- threadEl.setAttribute('line-num', '3');
- element.appendChild(threadEl);
- await element.updateComplete;
-
- assert.deepEqual(element.computeKeyLocations(), {
- left: {},
- right: {3: true},
- });
- });
-
- test('file comments are key locations', async () => {
- const threadEl = document.createElement('div');
- threadEl.className = 'comment-thread';
- threadEl.setAttribute('diff-side', 'left');
- element.appendChild(threadEl);
- await element.updateComplete;
-
- assert.deepEqual(element.computeKeyLocations(), {
- left: {FILE: true},
- right: {},
- });
- });
- });
const setupSampleDiff = async function (params: {
content: DiffContent[];
ignore_whitespace?: IgnoreWhitespaceType;
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index 056238a..67c78cd 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -20,6 +20,10 @@
return node.nodeType === 1;
}
+export function isHtmlElement(node: Node): node is HTMLElement {
+ return isElement(node) && node instanceof HTMLElement;
+}
+
export function isElementTarget(
target: EventTarget | null | undefined
): target is Element {
