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;