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/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;
