Refactor usage of thread elements in gr-diff
Unify all calls to threadEl.getAttribute() in gr-diff-utils.
Change-Id: Iea805a8885d2fa222329ec841fd931d4b5369010
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts
index 2f85a0b..912d59e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -306,7 +306,7 @@
return this.getContentTdByLine(line, side, row);
}
- getLineElByNumber(lineNumber: string | number, side?: Side) {
+ getLineElByNumber(lineNumber: LineNumber, side?: Side) {
const sideSelector = side ? '.' + side : '';
return this.diffElement.querySelector(
`.lineNum[data-value="${lineNumber}"]${sideSelector}`
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
index 08a567b..c788d76 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
@@ -30,6 +30,7 @@
import {GrSelectionActionBox} from '../gr-selection-action-box/gr-selection-action-box';
import {GrDiffBuilderElement} from '../gr-diff-builder/gr-diff-builder-element';
import {FILE} from '../gr-diff/gr-diff-line';
+import {getRange, getSide} from '../gr-diff/gr-diff-utils';
interface SidedRange {
side: Side;
@@ -199,13 +200,9 @@
}
_indexForThreadEl(threadEl: HTMLElement) {
- const side = threadEl.getAttribute('comment-side') as Side;
- const rangeString = threadEl.getAttribute('range');
- if (!rangeString) return undefined;
- const range = JSON.parse(rangeString) as CommentRange;
-
- if (!range) return undefined;
-
+ const side = getSide(threadEl);
+ const range = getRange(threadEl);
+ if (!side || !range) return undefined;
return this._indexOfCommentRange(side, range);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 4b4f429..2223734 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -24,7 +24,12 @@
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-diff-host_html';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
-import {rangesEqual} from '../gr-diff/gr-diff-utils';
+import {
+ getLine,
+ getRange,
+ getSide,
+ rangesEqual,
+} from '../gr-diff/gr-diff-utils';
import {appContext} from '../../../services/app-context';
import {
getParentIndex,
@@ -808,11 +813,7 @@
throw new Error(`Unknown side: ${commentSide}`);
}
function matchesRange(threadEl: GrCommentThread) {
- const rangeAtt = threadEl.getAttribute('range');
- const threadRange = rangeAtt
- ? (JSON.parse(rangeAtt) as CommentRange)
- : undefined;
- return rangesEqual(threadRange, range);
+ return rangesEqual(getRange(threadEl), range);
}
const filteredThreadEls = this._filterThreadElsForLocation(
@@ -830,21 +831,18 @@
) {
function matchesLeftLine(threadEl: GrCommentThread) {
return (
- threadEl.getAttribute('comment-side') === Side.LEFT &&
- threadEl.getAttribute('line-num') === String(lineInfo.beforeNumber)
+ getSide(threadEl) === Side.LEFT &&
+ getLine(threadEl) === lineInfo.beforeNumber
);
}
function matchesRightLine(threadEl: GrCommentThread) {
return (
- threadEl.getAttribute('comment-side') === Side.RIGHT &&
- threadEl.getAttribute('line-num') === String(lineInfo.afterNumber)
+ getSide(threadEl) === Side.RIGHT &&
+ getLine(threadEl) === lineInfo.afterNumber
);
}
function matchesFileComment(threadEl: GrCommentThread) {
- return (
- threadEl.getAttribute('comment-side') === side &&
- threadEl.getAttribute('line-num') === FILE
- );
+ return getSide(threadEl) === side && getLine(threadEl) === FILE;
}
// Select the appropriate matchers for the desired side and line
@@ -855,7 +853,7 @@
if (side === Side.RIGHT) {
matchers.push(matchesRightLine);
}
- if (lineInfo.afterNumber === 'FILE' || lineInfo.beforeNumber === 'FILE') {
+ if (lineInfo.afterNumber === FILE || lineInfo.beforeNumber === FILE) {
matchers.push(matchesFileComment);
}
return threadEls.filter(threadEl =>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.ts b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.ts
index 03740ba..08c8226 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.ts
@@ -30,6 +30,7 @@
import {DiffInfo} from '../../../types/diff';
import {Side} from '../../../constants/constants';
import {GrDiffBuilderElement} from '../gr-diff-builder/gr-diff-builder-element';
+import {getSide, isThreadEl} from '../gr-diff/gr-diff-utils';
/**
* Possible CSS classes indicating the state of selection. Dynamically added/
@@ -96,10 +97,10 @@
}
_handleDownOnRangeComment(node: Element) {
- if (node?.nodeName?.toLowerCase() === 'gr-comment-thread') {
+ if (isThreadEl(node)) {
this._setClasses([
SelectionClass.COMMENT,
- node.getAttribute('comment-side') === Side.LEFT
+ getSide(node) === Side.LEFT
? SelectionClass.LEFT
: SelectionClass.RIGHT,
]);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts
index 8984dc8..b13b3c5 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts
@@ -17,6 +17,7 @@
import {CommentRange} from '../../../types/common';
import {FILE, LineNumber} from './gr-diff-line';
+import {Side} from '../../../constants/constants';
/**
* Compare two ranges. Either argument may be falsy, but will only return
@@ -46,3 +47,47 @@
const lineNumber = Number(lineNumberStr);
return Number.isInteger(lineNumber) ? lineNumber : null;
}
+
+export function getLine(threadEl: HTMLElement): LineNumber {
+ const lineAtt = threadEl.getAttribute('line-num');
+ if (!lineAtt || lineAtt === 'FILE') return FILE;
+ const line = Number(lineAtt);
+ if (isNaN(line)) throw new Error(`cannot parse line number: ${lineAtt}`);
+ if (line < 1) throw new Error(`line number smaller than 1: ${line}`);
+ return line;
+}
+
+export function getSide(threadEl: HTMLElement): Side | undefined {
+ const sideAtt = threadEl.getAttribute('comment-side');
+ if (!sideAtt) {
+ console.warn('comment thread without side');
+ return undefined;
+ }
+ if (sideAtt !== Side.LEFT && sideAtt !== Side.RIGHT)
+ throw Error(`unexpected value for side: ${sideAtt}`);
+ return sideAtt as Side;
+}
+
+export function getRange(threadEl: HTMLElement): CommentRange | undefined {
+ const rangeAtt = threadEl.getAttribute('range');
+ if (!rangeAtt) return undefined;
+ const range = JSON.parse(rangeAtt) as CommentRange;
+ if (!range.start_line) throw new Error(`invalid range: ${rangeAtt}`);
+ return range;
+}
+
+// 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.
+// TODO: Also document the required HTML attritbutes that thread elements must
+// have, e.g. 'comment-side', 'range', 'line-num', 'data-value'.
+export interface GrDiffThreadElement extends HTMLElement {
+ rootId: string;
+}
+
+export function isThreadEl(node: Node): node is GrDiffThreadElement {
+ return (
+ node.nodeType === Node.ELEMENT_NODE &&
+ (node as Element).classList.contains('comment-thread')
+ );
+}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index d8d6115..472a6bd 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -27,8 +27,16 @@
import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners';
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
import {htmlTemplate} from './gr-diff_html';
-import {FILE, LineNumber} from './gr-diff-line';
-import {getLineNumber, rangesEqual} from './gr-diff-utils';
+import {LineNumber} from './gr-diff-line';
+import {
+ getLine,
+ getLineNumber,
+ getRange,
+ getSide,
+ GrDiffThreadElement,
+ isThreadEl,
+ rangesEqual,
+} from './gr-diff-utils';
import {getHiddenScroll} from '../../../scripts/hiddenscroll';
import {isMergeParent, patchNumEquals} from '../../../utils/patch-set-util';
import {customElement, observe, property} from '@polymer/decorators';
@@ -53,7 +61,7 @@
PolymerDomWrapper,
} from '../../../types/types';
import {CommentRangeLayer} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
-import {DiffViewMode, Side, CommentSide} from '../../../constants/constants';
+import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
import {KeyLocations} from '../gr-diff-processor/gr-diff-processor';
import {FlattenedNodesObserver} from '@polymer/polymer/lib/utils/flattened-nodes-observer';
import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces';
@@ -67,30 +75,6 @@
const FULL_CONTEXT = -1;
const LIMITED_CONTEXT = 10;
-function getSide(threadEl: GrCommentThread): Side | undefined {
- const sideAtt = threadEl.getAttribute('comment-side');
- if (!sideAtt) {
- console.warn('comment thread without side');
- return undefined;
- }
- if (sideAtt !== 'left' && sideAtt !== 'right')
- throw Error(`unexpected value for side: ${sideAtt}`);
- return sideAtt as Side;
-}
-
-function isThreadEl(node: Node): node is GrCommentThread {
- return (
- node.nodeType === Node.ELEMENT_NODE &&
- (node as Element).classList.contains('comment-thread')
- );
-}
-
-// TODO(TS): Replace by proper GrCommentThread once converted.
-type GrCommentThread = PolymerElement & {
- rootId: string;
- range: CommentRange;
-};
-
const COMMIT_MSG_PATH = '/COMMIT_MSG';
/**
* 72 is the unofficial length standard for git commit messages.
@@ -383,17 +367,16 @@
// TODO(brohlfs): Rewrite gr-diff to be agnostic of GrCommentThread, because
// other users of gr-diff may use different comment widgets.
_updateRanges(
- addedThreadEls: GrCommentThread[],
- removedThreadEls: GrCommentThread[]
+ addedThreadEls: GrDiffThreadElement[],
+ removedThreadEls: GrDiffThreadElement[]
) {
function commentRangeFromThreadEl(
- threadEl: GrCommentThread
+ threadEl: GrDiffThreadElement
): CommentRangeLayer | undefined {
const side = getSide(threadEl);
if (!side) return undefined;
- const rangeAtt = threadEl.getAttribute('range');
- if (!rangeAtt) return undefined;
- const range = JSON.parse(rangeAtt) as CommentRange;
+ const range = getRange(threadEl);
+ if (!range) return undefined;
return {side, range, hovering: false, rootId: threadEl.rootId};
}
@@ -437,12 +420,12 @@
for (const threadEl of threadEls) {
const side = getSide(threadEl);
if (!side) continue;
- const lineNum = threadEl.getAttribute('line-num') || FILE;
- const commentRange = threadEl.range || {};
+ 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) {
+ if (commentRange?.start_line) {
keyLocations[side][commentRange.start_line] = true;
}
}
@@ -450,7 +433,7 @@
}
// Dispatch events that are handled by the gr-diff-highlight.
- _redispatchHoverEvents(addedThreadEls: GrCommentThread[]) {
+ _redispatchHoverEvents(addedThreadEls: HTMLElement[]) {
for (const threadEl of addedThreadEls) {
threadEl.addEventListener('mouseenter', () => {
threadEl.dispatchEvent(
@@ -895,11 +878,11 @@
// for each line from the start.
let lastEl;
for (const threadEl of addedThreadEls) {
- const lineNumString = threadEl.getAttribute('line-num') || 'FILE';
+ const lineNum = getLine(threadEl);
const commentSide = getSide(threadEl);
if (!commentSide) continue;
const lineEl = this.$.diffBuilder.getLineElByNumber(
- lineNumString,
+ lineNum,
commentSide
);
// When the line the comment refers to does not exist, log an error
@@ -909,7 +892,7 @@
console.error(
'thread attached to line ',
commentSide,
- lineNumString,
+ lineNum,
' which does not exist.'
);
continue;
