Merge "Move computation of key locations into diff model"
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 {
