Convert gr-diff-host to typescript The change converts the following files to typescript: * elements/diff/gr-diff-host/gr-diff-host.ts Change-Id: Ie565ab753fea6909790729b8e42410d39c437612
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js index 126c330..8aebf9b 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -27,6 +27,7 @@ import {html} from '@polymer/polymer/lib/utils/html-tag.js'; import {TestKeyboardShortcutBinder} from '../../../test/test-utils.js'; import {Shortcut} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.js'; +import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api.js'; const commentApiMock = createCommentApiMockWithTemplateElement( 'gr-file-list-comment-api-mock', html` @@ -351,7 +352,7 @@ }); test('comment filtering', () => { - element.changeComments._comments = { + const comments = { '/COMMIT_MSG': [ {patch_set: 1, message: 'Done', updated: '2017-02-08 16:40:49'}, {patch_set: 1, message: 'oh hay', updated: '2017-02-09 16:40:49'}, @@ -387,7 +388,7 @@ }, ], }; - element.changeComments._drafts = { + const drafts = { '/COMMIT_MSG': [ { patch_set: 1, @@ -414,6 +415,7 @@ }, ], }; + element.changeComments = new ChangeComments(comments, {}, drafts, 123); const parentTo1 = { basePatchNum: 'PARENT',
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts index 208e766..de56e9d 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -97,11 +97,8 @@ PolymerSplice, PolymerSpliceChange, } from '@polymer/polymer/interfaces'; -import {assertNever, hasOwnProperty} from '../../../utils/common-util'; -import { - CommentThread, - HumanCommentInfoWithPath, -} from '../../diff/gr-comment-api/gr-comment-api'; +import {assertNever} from '../../../utils/common-util'; +import {CommentThread, isDraft} from '../../diff/gr-comment-api/gr-comment-api'; import {GrTextarea} from '../../shared/gr-textarea/gr-textarea'; import {GrAccountChip} from '../../shared/gr-account-chip/gr-account-chip'; import {GrOverlay} from '../../shared/gr-overlay/gr-overlay'; @@ -1029,10 +1026,7 @@ _containsNewCommentThread(threads: CommentThread[]) { return threads.some( thread => - !!thread.comments && - !!thread.comments[0] && - hasOwnProperty(thread.comments[0], '__draft') && - !!(thread.comments[0] as HumanCommentInfoWithPath).__draft + !!thread.comments && !!thread.comments[0] && isDraft(thread.comments[0]) ); }
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts index 52ee722..4b02075 100644 --- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts +++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -26,9 +26,12 @@ import {NO_THREADS_MSG} from '../../../constants/messages'; import {CommentSide, SpecialFilePath} from '../../../constants/constants'; -import {customElement, property, observe} from '@polymer/decorators'; -import {CommentThread} from '../../diff/gr-comment-api/gr-comment-api'; -import {Comment, RobotComment} from '../../shared/gr-comment/gr-comment'; +import {customElement, observe, property} from '@polymer/decorators'; +import { + CommentThread, + isDraft, + UIRobot, +} from '../../diff/gr-comment-api/gr-comment-api'; import {PolymerSpliceChange} from '@polymer/polymer/interfaces'; import {ChangeInfo} from '../../../types/common'; @@ -303,27 +306,32 @@ _getThreadWithStatusInfo(thread: CommentThread): CommentThreadWithInfo { const comments = thread.comments; - const lastComment = (comments[comments.length - 1] || {}) as Comment; + const lastComment = comments.length + ? comments[comments.length - 1] + : undefined; let hasRobotComment = false; let hasHumanReplyToRobotComment = false; comments.forEach(comment => { - if ((comment as RobotComment).robot_id) { + if ((comment as UIRobot).robot_id) { hasRobotComment = true; } else if (hasRobotComment) { hasHumanReplyToRobotComment = true; } }); + let updated = undefined; + if (lastComment) { + if (isDraft(lastComment)) updated = lastComment.__date; + if (lastComment.updated) updated = parseDate(lastComment.updated); + } return { thread, hasRobotComment, hasHumanReplyToRobotComment, - unresolved: !!lastComment.unresolved, - isEditing: !!lastComment.__editing, - hasDraft: !!lastComment.__draft, - updated: lastComment.updated - ? parseDate(lastComment.updated) - : lastComment.__date, + unresolved: !!lastComment && !!lastComment.unresolved, + isEditing: !!lastComment && !!lastComment.__editing, + hasDraft: !!lastComment && isDraft(lastComment), + updated, }; }
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js index 92010d2..bad3a99 100644 --- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js +++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
@@ -154,7 +154,7 @@ message: 'resolved draft', unresolved: false, __draft: true, - __draftID: '0.m683trwff68', + __draftID: '0.m683trwff69', __editing: false, patch_set: '2', },
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts index d27590a..675a048 100644 --- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts +++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -38,11 +38,9 @@ RobotId, } from '../../../types/common'; import {GrOverlay} from '../../shared/gr-overlay/gr-overlay'; -import { - CommentEventDetail, - isRobotComment, -} from '../../shared/gr-comment/gr-comment'; +import {CommentEventDetail} from '../../shared/gr-comment/gr-comment'; import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api'; +import {isRobot} from '../gr-comment-api/gr-comment-api'; export interface GrApplyFixDialog { $: { @@ -115,7 +113,7 @@ open(e: CustomEvent<CommentEventDetail>) { const detail = e.detail; const comment = detail.comment; - if (!detail.patchNum || !comment || !isRobotComment(comment)) { + if (!detail.patchNum || !comment || !isRobot(comment)) { return Promise.resolve(); } this._patchNum = detail.patchNum;
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts index 0830b83..deecdbf 100644 --- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts +++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -27,12 +27,12 @@ } from '../../../utils/patch-set-util'; import {customElement, property} from '@polymer/decorators'; import { + CommentBasics, CommentInfo, ConfigInfo, ParentPatchSetNum, PatchRange, PatchSetNum, - PathToCommentsInfoMap, PathToRobotCommentsInfoMap, RobotCommentInfo, Timestamp, @@ -40,36 +40,61 @@ NumericChangeId, } from '../../../types/common'; import {hasOwnProperty} from '../../../utils/common-util'; -import {CommentSide} from '../../../constants/constants'; +import {CommentSide, Side} from '../../../constants/constants'; import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api'; -export interface HumanCommentInfoWithPath extends CommentInfo { - path: string; +export interface DraftCommentProps { __draft?: boolean; + __draftID?: string; __date?: Date; } -export interface RobotCommentInfoWithPath extends RobotCommentInfo { - path: string; +export type DraftInfo = CommentBasics & DraftCommentProps; + +/** + * Each of the type implements or extends CommentBasics. + */ +export type Comment = DraftInfo | CommentInfo | RobotCommentInfo; + +export interface UIStateCommentProps { + // The `side` of the comment is PARENT or REVISION, but this is LEFT or RIGHT. + // TODO(TS): Remove the naming confusion of commentSide being of type of Side, + // but side being of type CommentSide. :-) + __commentSide?: Side; + // TODO(TS): Remove this. Seems to be exactly the same as `path`?? + __path?: string; + collapsed?: boolean; + // TODO(TS): Consider allowing this only for drafts. + __editing?: boolean; + __otherEditing?: boolean; } -export type CommentInfoWithPath = - | HumanCommentInfoWithPath - | RobotCommentInfoWithPath; +export type UIDraft = DraftInfo & UIStateCommentProps; -// TODO(TS): Can be removed, CommentInfoWithTwoPaths already has a path -export type CommentInfoWithTwoPaths = CommentInfoWithPath & {__path: string}; +export type UIHuman = CommentInfo & UIStateCommentProps; -export type PathToCommentsInfoWithPathMap = { - [path: string]: CommentInfoWithPath[]; -}; +export type UIRobot = RobotCommentInfo & UIStateCommentProps; + +export type UIComment = UIHuman | UIRobot | UIDraft; export type CommentMap = {[path: string]: boolean}; +export function isRobot<T extends CommentInfo>( + x: T | DraftInfo | RobotCommentInfo | undefined +): x is RobotCommentInfo { + return !!x && !!(x as RobotCommentInfo).robot_id; +} + +export function isDraft<T extends CommentInfo>( + x: T | UIDraft | undefined +): x is UIDraft { + return !!x && !!(x as UIDraft).__draft; +} + export interface PatchSetFile { path: string; basePath?: string; - patchNum: PatchSetNum; + patchNum?: PatchSetNum; } export interface PatchNumOnly { @@ -107,9 +132,13 @@ } export interface CommentThread { - comments: CommentInfoWithTwoPaths[]; + comments: UIComment[]; patchNum?: PatchSetNum; path: string; + // TODO(TS): It would be nice to use LineNumber here, but the comment thread + // element actually relies on line to be undefined for file comments. Be + // aware of element attribute getters and setters, if you try to refactor + // this. :-) Still worthwhile to do ... line?: number; rootId: UrlEncodedCommentId; commentSide?: CommentSide; @@ -119,7 +148,7 @@ [urlEncodedCommentId: string]: CommentThread; }; -interface TwoSidesComments { +export interface TwoSidesComments { // TODO(TS): remove meta - it is not used anywhere meta: { changeNum: NumericChangeId; @@ -127,16 +156,16 @@ patchRange: PatchRange; projectConfig?: ConfigInfo; }; - left: CommentInfoWithPath[]; - right: CommentInfoWithPath[]; + left: UIComment[]; + right: UIComment[]; } export class ChangeComments { - private readonly _comments: PathToCommentsInfoWithPathMap; + private readonly _comments: {[path: string]: UIHuman[]}; - private readonly _robotComments: PathToCommentsInfoWithPathMap; + private readonly _robotComments: {[path: string]: UIRobot[]}; - private readonly _drafts: PathToCommentsInfoWithPathMap; + private readonly _drafts: {[path: string]: UIDraft[]}; private readonly _changeNum: NumericChangeId; @@ -145,9 +174,9 @@ * elements of that which uses the gr-comment-api. */ constructor( - comments: PathToCommentsInfoMap | undefined, - robotComments: PathToRobotCommentsInfoMap | undefined, - drafts: PathToCommentsInfoMap | undefined, + comments: {[path: string]: UIHuman[]} | undefined, + robotComments: {[path: string]: UIRobot[]} | undefined, + drafts: {[path: string]: UIDraft[]} | undefined, changeNum: NumericChangeId ) { this._comments = this._addPath(comments); @@ -164,10 +193,10 @@ * TODO(taoalpha): should consider changing BE to send path * back within CommentInfo */ - _addPath( - comments: PathToCommentsInfoMap = {} - ): PathToCommentsInfoWithPathMap { - const updatedComments: PathToCommentsInfoWithPathMap = {}; + _addPath<T>( + comments: {[path: string]: T[]} = {} + ): {[path: string]: Array<T & {path: string}>} { + const updatedComments: {[path: string]: Array<T & {path: string}>} = {}; for (const filePath of Object.keys(comments)) { const allCommentsForPath = comments[filePath] || []; if (allCommentsForPath.length) { @@ -191,10 +220,8 @@ return this._robotComments; } - findCommentById( - commentId: UrlEncodedCommentId - ): HumanCommentInfoWithPath | RobotCommentInfoWithPath | undefined { - const findComment = (comments: PathToCommentsInfoWithPathMap) => { + findCommentById(commentId: UrlEncodedCommentId): Comment | undefined { + const findComment = (comments: {[path: string]: CommentBasics[]}) => { let comment; for (const path of Object.keys(comments)) { comment = comment || comments[path].find(c => c.id === commentId); @@ -216,7 +243,11 @@ * patchNum and basePatchNum properties to represent the range. */ getPaths(patchRange?: PatchRange): CommentMap { - const responses = [this.comments, this.drafts, this.robotComments]; + const responses: {[path: string]: UIComment[]}[] = [ + this.comments, + this.drafts, + this.robotComments, + ]; const commentMap: CommentMap = {}; for (const response of responses) { for (const path in response) { @@ -240,9 +271,7 @@ /** * Gets all the comments and robot comments for the given change. */ - getAllPublishedComments( - patchNum?: PatchSetNum - ): PathToCommentsInfoWithPathMap { + getAllPublishedComments(patchNum?: PatchSetNum) { return this.getAllComments(false, patchNum); } @@ -263,12 +292,9 @@ /** * Gets all the comments and robot comments for the given change. */ - getAllComments( - includeDrafts?: boolean, - patchNum?: PatchSetNum - ): PathToCommentsInfoWithPathMap { + getAllComments(includeDrafts?: boolean, patchNum?: PatchSetNum) { const paths = this.getPaths(); - const publishedComments: PathToCommentsInfoWithPathMap = {}; + const publishedComments: {[path: string]: CommentBasics[]} = {}; for (const path of Object.keys(paths)) { publishedComments[path] = this.getAllCommentsForPath( path, @@ -282,9 +308,9 @@ /** * Gets all the drafts for the given change. */ - getAllDrafts(patchNum?: PatchSetNum): PathToCommentsInfoWithPathMap { + getAllDrafts(patchNum?: PatchSetNum) { const paths = this.getPaths(); - const drafts: PathToCommentsInfoWithPathMap = {}; + const drafts: {[path: string]: UIDraft[]} = {}; for (const path of Object.keys(paths)) { drafts[path] = this.getAllDraftsForPath(path, patchNum); } @@ -302,8 +328,8 @@ path: string, patchNum?: PatchSetNum, includeDrafts?: boolean - ): CommentInfoWithPath[] { - const comments = this._comments[path] || []; + ): Comment[] { + const comments: Comment[] = this._comments[path] || []; const robotComments = this._robotComments[path] || []; let allComments = comments.concat(robotComments); if (includeDrafts) { @@ -347,10 +373,7 @@ * This will return a shallow copy of all drafts every time, * so changes on any copy will not affect other copies. */ - getAllDraftsForPath( - path: string, - patchNum?: PatchSetNum - ): CommentInfoWithPath[] { + getAllDraftsForPath(path: string, patchNum?: PatchSetNum): Comment[] { let comments = this._drafts[path] || []; if (patchNum) { comments = comments.filter(c => patchNumEquals(c.patch_set, patchNum)); @@ -365,7 +388,7 @@ * * // TODO(taoalpha): maybe merge in *ForPath */ - getAllDraftsForFile(file: PatchSetFile): CommentInfoWithPath[] { + getAllDraftsForFile(file: PatchSetFile): Comment[] { let allDrafts = this.getAllDraftsForPath(file.path, file.patchNum); if (file.basePath) { allDrafts = allDrafts.concat( @@ -390,9 +413,9 @@ patchRange: PatchRange, projectConfig?: ConfigInfo ): TwoSidesComments { - let comments: CommentInfoWithPath[] = []; - let drafts: CommentInfoWithPath[] = []; - let robotComments: CommentInfoWithPath[] = []; + let comments: Comment[] = []; + let drafts: DraftInfo[] = []; + let robotComments: RobotCommentInfo[] = []; if (this.comments && this.comments[path]) { comments = this.comments[path]; } @@ -404,11 +427,10 @@ } drafts.forEach(d => { - // drafts don't include robot comments - (d as HumanCommentInfoWithPath).__draft = true; + d.__draft = true; }); - const all = comments + const all: Comment[] = comments .concat(drafts) .concat(robotComments) .map(c => { @@ -450,7 +472,7 @@ file: PatchSetFile, patchRange: PatchRange, projectConfig?: ConfigInfo - ) { + ): TwoSidesComments { const comments = this.getCommentsBySideForPath( file.path, patchRange, @@ -476,24 +498,22 @@ * also includes the file that it was left on, which was the key of the * originall object. */ - _commentObjToArrayWithFile( - comments: PathToCommentsInfoWithPathMap - ): CommentInfoWithTwoPaths[] { - let commentArr: CommentInfoWithTwoPaths[] = []; + _commentObjToArrayWithFile<T>(comments: { + [path: string]: T[]; + }): Array<T & {__path: string}> { + let commentArr: Array<T & {__path: string}> = []; for (const file of Object.keys(comments)) { - const commentsForFile = []; + const commentsForFile: Array<T & {__path: string}> = []; for (const comment of comments[file]) { - commentsForFile.push({__path: file, ...comment}); + commentsForFile.push({...comment, __path: file}); } commentArr = commentArr.concat(commentsForFile); } return commentArr; } - _commentObjToArray( - comments: PathToCommentsInfoWithPathMap - ): CommentInfoWithPath[] { - let commentArr: CommentInfoWithPath[] = []; + _commentObjToArray<T>(comments: {[path: string]: T[]}): T[] { + let commentArr: T[] = []; for (const file of Object.keys(comments)) { commentArr = commentArr.concat(comments[file]); } @@ -527,8 +547,8 @@ * Computes a number of unresolved comment threads in a given file and path. */ computeUnresolvedNum(file: PatchSetFile | PatchNumOnly) { - let comments: CommentInfoWithPath[] = []; - let drafts: CommentInfoWithPath[] = []; + let comments: Comment[] = []; + let drafts: Comment[] = []; if (isPatchSetFile(file)) { comments = this.getAllCommentsForFile(file); @@ -541,12 +561,7 @@ comments = comments.concat(drafts); - // TODO(TS): the 'as CommentInfoWithTwoPaths[]' is completely wrong below - // However, this doesn't affect the final result of computeUnresolvedNum - // This should be fixed by removing CommentInfoWithTwoPaths later - const threads = this.getCommentThreads( - sortComments(comments) as CommentInfoWithTwoPaths[] - ); + const threads = this.getCommentThreads(sortComments(comments)); const unresolvedThreads = threads.filter( thread => @@ -568,7 +583,7 @@ * * @param comments sorted by updated timestamp. */ - getCommentThreads(comments: CommentInfoWithTwoPaths[]) { + getCommentThreads(comments: UIComment[]) { const threads: CommentThread[] = []; const idThreadMap: CommentIdToCommentThreadMap = {}; for (const comment of comments) { @@ -585,10 +600,13 @@ } // Otherwise, this comment starts its own thread. + if (!comment.__path && !comment.path) { + throw new Error('Comment missing required "path".'); + } const newThread: CommentThread = { comments: [comment], patchNum: comment.patch_set, - path: comment.__path, + path: comment.__path || comment.path!, line: comment.line, rootId: comment.id, }; @@ -605,7 +623,7 @@ * Whether the given comment should be included in the base side of the * given patch range. */ - _isInBaseOfPatchRange(comment: CommentInfo, range: PatchRange) { + _isInBaseOfPatchRange(comment: CommentBasics, range: PatchRange) { // If the base of the patch range is a parent of a merge, and the comment // appears on a specific parent then only show the comment if the parent // index of the comment matches that of the range. @@ -636,7 +654,7 @@ * Whether the given comment should be included in the revision side of the * given patch range. */ - _isInRevisionOfPatchRange(comment: CommentInfo, range: PatchRange) { + _isInRevisionOfPatchRange(comment: CommentBasics, range: PatchRange) { return ( comment.side !== CommentSide.PARENT && patchNumEquals(comment.patch_set, range.patchNum) @@ -646,7 +664,7 @@ /** * Whether the given comment should be included in the given patch range. */ - _isInPatchRange(comment: CommentInfo, range: PatchRange): boolean { + _isInPatchRange(comment: CommentBasics, range: PatchRange): boolean { return ( this._isInBaseOfPatchRange(comment, range) || this._isInRevisionOfPatchRange(comment, range)
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js index bfe74c2..2cf9bc1 100644 --- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js +++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
@@ -17,6 +17,7 @@ import '../../../test/common-test-setup-karma.js'; import './gr-comment-api.js'; +import {ChangeComments} from './gr-comment-api.js'; const basicFixture = fixtureFromElement('gr-comment-api'); @@ -216,7 +217,7 @@ } setup(() => { - element._changeComments._drafts = { + const drafts = { 'file/one': [ { id: '11', @@ -244,7 +245,7 @@ }, ], }; - element._changeComments._robotComments = { + const robotComments = { 'file/one': [ { id: '01', @@ -268,7 +269,7 @@ }, ], }; - element._changeComments._comments = { + const comments = { 'file/one': [ { id: '03', @@ -305,6 +306,8 @@ {id: '10', patch_set: 5, line: 1, updated: makeTime(1)}, ], }; + element._changeComments = + new ChangeComments(comments, robotComments, drafts, 1234); }); test('getPaths', () => { @@ -405,9 +408,7 @@ }); test('computeUnresolvedNum w/ non-linear thread', () => { - element._changeComments._drafts = {}; - element._changeComments._robotComments = {}; - element._changeComments._comments = { + const comments = { path: [{ id: '9c6ba3c6_28b7d467', patch_set: 1, @@ -433,6 +434,7 @@ unresolved: false, }], }; + element._changeComments = new ChangeComments(comments, {}, {}, 1234); assert.equal( element._changeComments.computeUnresolvedNum(1, 'path'), 0); }); @@ -523,6 +525,7 @@ end_line: 2, end_character: 2, }, + path: 'file/one', __path: 'file/one', }, ], @@ -538,6 +541,7 @@ patch_set: 2, side: 'PARENT', line: 2, + path: 'file/one', __path: 'file/one', updated: '2013-02-26 15:01:43.986000000', }, @@ -553,6 +557,7 @@ id: '04', patch_set: 2, line: 1, + path: 'file/one', __path: 'file/one', updated: '2013-02-26 15:01:43.986000000', }, @@ -562,6 +567,7 @@ patch_set: 2, unresolved: true, line: 1, + path: 'file/one', __path: 'file/one', updated: '2013-02-26 15:03:43.986000000', }, @@ -570,6 +576,7 @@ in_reply_to: '04', patch_set: 2, line: 1, + path: 'file/one', __path: 'file/one', __draft: true, updated: '2013-02-26 15:02:43.986000000', @@ -585,6 +592,7 @@ id: '05', patch_set: 2, line: 2, + path: 'file/two', __path: 'file/two', updated: '2013-02-26 15:01:43.986000000', }, @@ -599,6 +607,7 @@ id: '06', patch_set: 3, line: 2, + path: 'file/two', __path: 'file/two', updated: '2013-02-26 15:01:43.986000000', }, @@ -615,6 +624,7 @@ side: 'PARENT', unresolved: true, line: 1, + path: 'file/three', __path: 'file/three', updated: '2013-02-26 15:01:43.986000000', }, @@ -630,6 +640,7 @@ id: '08', patch_set: 3, line: 1, + path: 'file/three', __path: 'file/three', updated: '2013-02-26 15:01:43.986000000', }, @@ -645,6 +656,7 @@ patch_set: 5, side: 'PARENT', line: 1, + path: 'file/four', __path: 'file/four', updated: '2013-02-26 15:01:43.986000000', }, @@ -660,6 +672,7 @@ id: '10', patch_set: 5, line: 1, + path: 'file/four', __path: 'file/four', updated: '2013-02-26 15:01:43.986000000', }, @@ -674,6 +687,7 @@ id: '05', patch_set: 3, line: 1, + path: 'file/two', __path: 'file/two', __draft: true, updated: '2013-02-26 15:03:43.986000000', @@ -690,6 +704,7 @@ patch_set: 2, side: 'PARENT', line: 1, + path: 'file/one', __path: 'file/one', __draft: true, updated: '2013-02-26 15:03:43.986000000', @@ -710,6 +725,7 @@ let expectedComments = [ { __path: 'file/one', + path: 'file/one', id: '04', patch_set: 2, line: 1, @@ -717,6 +733,7 @@ }, { __path: 'file/one', + path: 'file/one', id: '02', in_reply_to: '04', patch_set: 2, @@ -726,6 +743,7 @@ }, { __path: 'file/one', + path: 'file/one', __draft: true, id: '12', in_reply_to: '04', @@ -742,6 +760,7 @@ patch_set: 2, side: 'PARENT', line: 1, + path: 'file/one', __path: 'file/one', __draft: true, updated: '2013-02-26 15:03:43.986000000',
diff --git a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.ts b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.ts index 70629f4..2167641 100644 --- a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.ts +++ b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.ts
@@ -18,7 +18,7 @@ import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin'; import {PolymerElement} from '@polymer/polymer/polymer-element'; import {htmlTemplate} from './gr-coverage-layer_html'; -import {CoverageType, DiffLayer} from '../../../types/types'; +import {CoverageRange, CoverageType, DiffLayer} from '../../../types/types'; import {customElement, property} from '@polymer/decorators'; declare global { @@ -45,11 +45,9 @@ /** * Must be sorted by code_range.start_line. * Must only contain ranges that match the side. - * */ - // TODO(TS): convert into AnnotationLayer once gr-diff-is converted @property({type: Array}) - coverageRanges: any[] = []; + coverageRanges: CoverageRange[] = []; @property({type: String}) side?: string;
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 aee6ab9..5256564 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
@@ -317,9 +317,7 @@ } getSideByLineEl(lineEl: Element) { - return lineEl.classList.contains(GrDiffBuilder.Side.RIGHT) - ? Side.RIGHT - : Side.LEFT; + return lineEl.classList.contains(Side.RIGHT) ? Side.RIGHT : Side.LEFT; } emitGroup(group: GrDiffGroup, sectionEl: HTMLElement) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts index 5c9b998..d1f52ae 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
@@ -24,6 +24,7 @@ } from '../gr-diff/gr-diff-group'; import {BlameInfo, DiffInfo, DiffPreferencesInfo} from '../../../types/common'; import {Side} from '../../../constants/constants'; +import {DiffLayer} from '../../../types/types'; /** * In JS, unicode code points above 0xFFFF occupy two elements of a string. @@ -86,8 +87,7 @@ diff: DiffInfo, prefs: DiffPreferencesInfo, outputEl: HTMLElement, - // TODO(TS): Replace any by a layer interface. - readonly layers: any[] = [] + readonly layers: DiffLayer[] = [] ) { this._diff = diff; this._numLinesLeft = this._diff.content @@ -142,12 +142,6 @@ REMOVED: 'edit_a', }; - // TODO(TS): Convert to enum. - static readonly Side = { - LEFT: 'left', - RIGHT: 'right', - }; - // TODO(TS): Replace usages with ContextButtonType enum. static readonly ContextButtonType = { ABOVE: 'above', @@ -480,10 +474,14 @@ contentText.setAttribute('data-side', side); } - for (const layer of this.layers) { - if (typeof layer.annotate === 'function') { - layer.annotate(contentText, lineNumberEl, line); + if (lineNumberEl) { + for (const layer of this.layers) { + if (typeof layer.annotate === 'function') { + layer.annotate(contentText, lineNumberEl, line); + } } + } else { + console.error('The lineNumberEl is null, skipping layer annotations.'); } td.appendChild(contentText);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts index 9937ebb..1896eff 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -26,9 +26,8 @@ import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin'; import {PolymerElement} from '@polymer/polymer/polymer-element'; import {htmlTemplate} from './gr-diff-cursor_html'; -import {ScrollMode} from '../../../constants/constants'; +import {ScrollMode, Side} from '../../../constants/constants'; import {customElement, property, observe} from '@polymer/decorators'; -import {DiffSide} from '../gr-diff/gr-diff-utils'; import {GrDiffLineType} from '../gr-diff/gr-diff-line'; import {PolymerSpliceChange} from '@polymer/polymer/interfaces'; import {PolymerDomWrapper} from '../../../types/types'; @@ -83,7 +82,7 @@ private _lastDisplayedNavigateToNextFileToast: number | null = null; @property({type: String}) - side = DiffSide.RIGHT; + side = Side.RIGHT; @property({type: Object, notify: true, observer: '_rowChanged'}) diffRow?: HTMLElement; @@ -162,14 +161,14 @@ } moveLeft() { - this.side = DiffSide.LEFT; + this.side = Side.LEFT; if (this._isTargetBlank()) { this.moveUp(); } } moveRight() { - this.side = DiffSide.RIGHT; + this.side = Side.RIGHT; if (this._isTargetBlank()) { this.moveUp(); } @@ -272,7 +271,7 @@ this._fixSide(); } - moveToLineNumber(number: number, side: DiffSide, path?: string) { + moveToLineNumber(number: number, side: Side, path?: string) { const row = this._findRowByNumberAndFile(number, side, path); if (row) { this.side = side; @@ -291,7 +290,7 @@ } if (this._getViewMode() === DiffViewMode.SIDE_BY_SIDE) { - lineElSelector += this.side === DiffSide.LEFT ? '.left' : '.right'; + lineElSelector += this.side === Side.LEFT ? '.left' : '.right'; } return this.diffRow.querySelector(lineElSelector); @@ -452,7 +451,7 @@ _rowHasSide(row: Element) { const selector = - (this.side === DiffSide.LEFT ? '.left' : '.right') + ' + .content'; + (this.side === Side.LEFT ? '.left' : '.right') + ' + .content'; return !!row.querySelector(selector); } @@ -478,7 +477,7 @@ this._getViewMode() === DiffViewMode.SIDE_BY_SIDE && this._isTargetBlank() ) { - this.side = this.side === DiffSide.LEFT ? DiffSide.RIGHT : DiffSide.LEFT; + this.side = this.side === Side.LEFT ? Side.RIGHT : Side.LEFT; } } @@ -489,8 +488,8 @@ const actions = this._getActionsForRow(); return ( - (this.side === DiffSide.LEFT && !actions.left) || - (this.side === DiffSide.RIGHT && !actions.right) + (this.side === Side.LEFT && !actions.left) || + (this.side === Side.RIGHT && !actions.right) ); } @@ -506,16 +505,8 @@ if (!this.diffRow) { return; } - this.toggleClass( - LEFT_SIDE_CLASS, - this.side === DiffSide.LEFT, - this.diffRow - ); - this.toggleClass( - RIGHT_SIDE_CLASS, - this.side === DiffSide.RIGHT, - this.diffRow - ); + this.toggleClass(LEFT_SIDE_CLASS, this.side === Side.LEFT, this.diffRow); + this.toggleClass(RIGHT_SIDE_CLASS, this.side === Side.RIGHT, this.diffRow); } _isActionType(type: GrDiffRowType) { @@ -601,7 +592,7 @@ _findRowByNumberAndFile( targetNumber: number, - side: DiffSide, + side: Side, path?: string ): HTMLElement | undefined { let stops;
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 02d8cfa..49826bb 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
@@ -14,21 +14,62 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import '../../shared/gr-rest-api-interface/gr-rest-api-interface.js'; -import '../../shared/gr-comment-thread/gr-comment-thread.js'; -import '../../shared/gr-js-api-interface/gr-js-api-interface.js'; -import '../gr-diff/gr-diff.js'; -import '../gr-syntax-layer/gr-syntax-layer.js'; -import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners.js'; -import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin.js'; -import {PolymerElement} from '@polymer/polymer/polymer-element.js'; -import {htmlTemplate} from './gr-diff-host_html.js'; -import {GrDiffBuilder} from '../gr-diff-builder/gr-diff-builder.js'; -import {GerritNav} from '../../core/gr-navigation/gr-navigation.js'; -import {DiffSide, rangesEqual} from '../gr-diff/gr-diff-utils.js'; -import {appContext} from '../../../services/app-context.js'; -import {getParentIndex, isMergeParent} from '../../../utils/patch-set-util.js'; -import {sortComments} from '../gr-comment-api/gr-comment-api.js'; +import '../../shared/gr-rest-api-interface/gr-rest-api-interface'; +import '../../shared/gr-comment-thread/gr-comment-thread'; +import '../../shared/gr-js-api-interface/gr-js-api-interface'; +import '../gr-diff/gr-diff'; +import '../gr-syntax-layer/gr-syntax-layer'; +import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners'; +import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin'; +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 {appContext} from '../../../services/app-context'; +import { + getParentIndex, + isMergeParent, + isNumber, +} from '../../../utils/patch-set-util'; +import { + Comment, + isDraft, + PatchSetFile, + sortComments, + TwoSidesComments, + UIComment, +} from '../gr-comment-api/gr-comment-api'; +import {customElement, observe, property} from '@polymer/decorators'; +import { + CommitRange, + CoverageRange, + DiffLayer, + DiffLayerListener, +} from '../../../types/types'; +import { + Base64ImageFile, + BlameInfo, + CommentRange, + DiffInfo, + DiffPreferencesInfo, + NumericChangeId, + PatchRange, + PatchSetNum, + RepoName, +} from '../../../types/common'; +import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api'; +import {JsApiService} from '../../shared/gr-js-api-interface/gr-js-api-types'; +import {GrDiff, LineOfInterest} from '../gr-diff/gr-diff'; +import {GrSyntaxLayer} from '../gr-syntax-layer/gr-syntax-layer'; +import { + DiffViewMode, + IgnoreWhitespaceType, + Side, +} from '../../../constants/constants'; +import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces'; +import {FilesWebLinks} from '../gr-patch-range-select/gr-patch-range-select'; +import {LineNumber} from '../gr-diff/gr-diff-line'; +import {GrCommentThread} from '../../shared/gr-comment-thread/gr-comment-thread'; const MSG_EMPTY_BLAME = 'No blame information for this diff.'; @@ -36,12 +77,6 @@ const EVENT_ZERO_REBASE = 'rebase-percent-zero'; const EVENT_NONZERO_REBASE = 'rebase-percent-nonzero'; -const DiffViewMode = { - SIDE_BY_SIDE: 'SIDE_BY_SIDE', - UNIFIED: 'UNIFIED_DIFF', -}; - -/** @enum {string} */ const TimingLabel = { TOTAL: 'Diff Total Render', CONTENT: 'Diff Content Render', @@ -58,37 +93,58 @@ // 120 lines is good enough threshold for full-sized window viewport const NUM_OF_LINES_THRESHOLD_FOR_VIEWPORT = 120; -const WHITESPACE_IGNORE_NONE = 'IGNORE_NONE'; +function isImageDiff(diff?: DiffInfo) { + if (!diff) return false; -/** - * @param {Object} diff - * @return {boolean} - */ -function isImageDiff(diff) { - if (!diff) { return false; } - - const isA = diff.meta_a && - diff.meta_a.content_type.startsWith('image/'); - const isB = diff.meta_b && - diff.meta_b.content_type.startsWith('image/'); + const isA = diff.meta_a && diff.meta_a.content_type.startsWith('image/'); + const isB = diff.meta_b && diff.meta_b.content_type.startsWith('image/'); return !!(diff.binary && (isA || isB)); } +interface LineInfo { + beforeNumber?: LineNumber; + afterNumber?: LineNumber; +} + +// TODO(TS): Consolidate this with the CommentThread interface of comment-api. +// What is being used here is just a local object for collecting all the data +// that is needed to create a GrCommentThread component, see +// _createThreadElement(). +interface CommentThread { + comments: UIComment[]; + // In the context of a diff each thread must have a side! + commentSide: Side; + patchNum?: PatchSetNum; + lineNum?: LineNumber; + isOnParent?: boolean; + range?: CommentRange; +} + +export interface GrDiffHost { + $: { + restAPI: RestApiService & Element; + jsAPI: JsApiService & Element; + syntaxLayer: GrSyntaxLayer & Element; + diff: GrDiff; + }; +} + /** * Wrapper around gr-diff. * * Webcomponent fetching diffs and related data from restAPI and passing them - * to the presentational gr-diff for rendering. - * - * @extends PolymerElement + * to the presentational gr-diff for rendering. <gr-diff-host> is a Gerrit + * specific component, while <gr-diff> is a re-usable component. */ -class GrDiffHost extends GestureEventListeners( - LegacyElementMixin( - PolymerElement)) { - static get template() { return htmlTemplate; } +@customElement('gr-diff-host') +export class GrDiffHost extends GestureEventListeners( + LegacyElementMixin(PolymerElement) +) { + static get template() { + return htmlTemplate; + } - static get is() { return 'gr-diff-host'; } /** * Fired when the user selects a line. * @@ -107,192 +163,141 @@ * @event diff-comments-modified */ - static get properties() { - return { - changeNum: String, - noAutoRender: { - type: Boolean, - value: false, - }, - /** @type {?} */ - patchRange: Object, - /** @type {!Gerrit.FileRange} */ - file: Object, - // TODO: deprecate path since that info is included in file - path: String, - prefs: { - type: Object, - }, - projectName: String, - displayLine: { - type: Boolean, - value: false, - }, - isImageDiff: { - type: Boolean, - computed: '_computeIsImageDiff(diff)', - notify: true, - }, - commitRange: Object, - // The return type is FilesWebLinks from gr-patch-range-select. - filesWeblinks: { - type: Object, - value() { - return {}; - }, - notify: true, - }, - hidden: { - type: Boolean, - reflectToAttribute: true, - }, - noRenderOnPrefsChange: { - type: Boolean, - value: false, - }, - comments: { - type: Object, - observer: '_commentsChanged', - }, - lineWrapping: { - type: Boolean, - value: false, - }, - viewMode: { - type: String, - value: DiffViewMode.SIDE_BY_SIDE, - }, + @property({type: Number}) + changeNum?: NumericChangeId; - /** - * Special line number which should not be collapsed into a shared region. - * - * @type {{ - * number: number, - * leftSide: {boolean} - * }|null} - */ - lineOfInterest: Object, + @property({type: Boolean}) + noAutoRender = false; - /** - * If the diff fails to load, show the failure message in the diff rather - * than bubbling the error up to the whole page. This is useful for when - * loading inline diffs because one diff failing need not mark the whole - * page with a failure. - */ - showLoadFailure: Boolean, + @property({type: Object}) + patchRange?: PatchRange; - isBlameLoaded: { - type: Boolean, - notify: true, - computed: '_computeIsBlameLoaded(_blame)', - }, + @property({type: Object}) + file?: PatchSetFile; - _loggedIn: { - type: Boolean, - value: false, - }, + @property({type: String}) + path?: string; - _loading: { - type: Boolean, - value: false, - }, + @property({type: Object}) + prefs?: DiffPreferencesInfo; - /** @type {?string} */ - _errorMessage: { - type: String, - value: null, - }, + @property({type: String}) + projectName?: RepoName; - /** @type {?Object} */ - _baseImage: Object, - /** @type {?Object} */ - _revisionImage: Object, - /** - * This is a DiffInfo object. - */ - diff: { - type: Object, - notify: true, - }, + @property({type: Boolean}) + displayLine = false; - _fetchDiffPromise: { - type: Object, - value: null, - }, + @property({ + type: Boolean, + computed: '_computeIsImageDiff(diff)', + notify: true, + }) + isImageDiff?: boolean; - /** @type {?Object} */ - _blame: { - type: Object, - value: null, - }, + @property({type: Object}) + commitRange?: CommitRange; - /** - * @type {!Array<!Gerrit.CoverageRange>} - */ - _coverageRanges: { - type: Array, - value: () => [], - }, + @property({type: Object, notify: true}) + filesWeblinks: FilesWebLinks | {} = {}; - _loadedWhitespaceLevel: String, + @property({type: Boolean, reflectToAttribute: true}) + hidden = false; - _parentIndex: { - type: Number, - computed: '_computeParentIndex(patchRange.*)', - }, + @property({type: Boolean}) + noRenderOnPrefsChange = false; - _syntaxHighlightingEnabled: { - type: Boolean, - computed: - '_isSyntaxHighlightingEnabled(prefs.*, diff)', - }, + @property({type: Object, observer: '_commentsChanged'}) + comments?: TwoSidesComments; - _layers: { - type: Array, - value: [], - }, - }; - } + @property({type: Boolean}) + lineWrapping = false; - static get observers() { - return [ - '_whitespaceChanged(prefs.ignore_whitespace, _loadedWhitespaceLevel,' + - ' noRenderOnPrefsChange)', - '_syntaxHighlightingChanged(noRenderOnPrefsChange, prefs.*)', - ]; - } + @property({type: String}) + viewMode = DiffViewMode.SIDE_BY_SIDE; - constructor() { - super(); - this.reporting = appContext.reportingService; - } + @property({type: Object}) + lineOfInterest?: LineOfInterest; + + @property({type: Boolean}) + showLoadFailure?: boolean; + + @property({ + type: Boolean, + notify: true, + computed: '_computeIsBlameLoaded(_blame)', + }) + isBlameLoaded?: boolean; + + @property({type: Boolean}) + _loggedIn = false; + + @property({type: Boolean}) + _loading = false; + + @property({type: String}) + _errorMessage: string | null = null; + + @property({type: Object}) + _baseImage: Base64ImageFile | null = null; + + @property({type: Object}) + _revisionImage: Base64ImageFile | null = null; + + @property({type: Object, notify: true}) + diff?: DiffInfo; + + @property({type: Object}) + _fetchDiffPromise: Promise<DiffInfo> | null = null; + + @property({type: Object}) + _blame: BlameInfo[] | null = null; + + @property({type: Array}) + _coverageRanges: CoverageRange[] = []; + + @property({type: String}) + _loadedWhitespaceLevel?: IgnoreWhitespaceType; + + @property({type: Number, computed: '_computeParentIndex(patchRange.*)'}) + _parentIndex: number | null = null; + + @property({ + type: Boolean, + computed: '_isSyntaxHighlightingEnabled(prefs.*, diff)', + }) + _syntaxHighlightingEnabled?: boolean; + + @property({type: Array}) + _layers: DiffLayer[] = []; + + private readonly reporting = appContext.reportingService; /** @override */ created() { super.created(); this.addEventListener( - // These are named inconsistently for a reason: - // The create-comment event is fired to indicate that we should - // create a comment. - // The comment-* events are just notifying that the comments did already - // change in some way, and that we should update any models we may want - // to keep in sync. - 'create-comment', - e => this._handleCreateComment(e)); - this.addEventListener('comment-discard', - e => this._handleCommentDiscard(e)); - this.addEventListener('comment-update', - e => this._handleCommentUpdate(e)); - this.addEventListener('comment-save', - e => this._handleCommentSave(e)); - this.addEventListener('render-start', - () => this._handleRenderStart()); - this.addEventListener('render-content', - () => this._handleRenderContent()); - this.addEventListener('normalize-range', - event => this._handleNormalizeRange(event)); - this.addEventListener('diff-context-expanded', - event => this._handleDiffContextExpanded(event)); + // These are named inconsistently for a reason: + // The create-comment event is fired to indicate that we should + // create a comment. + // The comment-* events are just notifying that the comments did already + // change in some way, and that we should update any models we may want + // to keep in sync. + 'create-comment', + e => this._handleCreateComment(e) + ); + this.addEventListener('comment-discard', e => + this._handleCommentDiscard(e) + ); + this.addEventListener('comment-update', e => this._handleCommentUpdate(e)); + this.addEventListener('comment-save', e => this._handleCommentSave(e)); + this.addEventListener('render-start', () => this._handleRenderStart()); + this.addEventListener('render-content', () => this._handleRenderContent()); + this.addEventListener('normalize-range', event => + this._handleNormalizeRange(event) + ); + this.addEventListener('diff-context-expanded', event => + this._handleDiffContextExpanded(event) + ); } /** @override */ @@ -318,20 +323,24 @@ } /** - * @param {boolean=} shouldReportMetric indicate a new Diff Page. This is a + * @param shouldReportMetric indicate a new Diff Page. This is a * signal to report metrics event that started on location change. - * @return {!Promise} - **/ - reload(shouldReportMetric) { + * @return + */ + reload(shouldReportMetric?: boolean) { this.clear(); + if (!this.path) throw new Error('Missing required "path" property.'); + if (!this.changeNum) throw new Error('Missing required "changeNum" prop.'); this._loading = true; this._errorMessage = null; const whitespaceLevel = this._getIgnoreWhitespace(); - const layers = [this.$.syntaxLayer]; + const layers: DiffLayer[] = [this.$.syntaxLayer]; // Get layers from plugins (if any). for (const pluginLayer of this.$.jsAPI.getDiffLayers( - this.path, this.changeNum)) { + this.path, + this.changeNum + )) { layers.push(pluginLayer); } this._layers = layers; @@ -344,19 +353,21 @@ this._coverageRanges = []; this._getCoverageData(); const diffRequest = this._getDiff() - .then(diff => { - this._loadedWhitespaceLevel = whitespaceLevel; - this._reportDiff(diff); - return diff; - }) - .catch(e => { - this._handleGetDiffError(e); - return null; - }); + .then(diff => { + this._loadedWhitespaceLevel = whitespaceLevel; + this._reportDiff(diff); + return diff; + }) + .catch(e => { + this._handleGetDiffError(e); + return null; + }); const assetRequest = diffRequest.then(diff => { // If the diff is null, then it's failed to load. - if (!diff) { return null; } + if (!diff) { + return null; + } return this._loadDiffAssets(diff); }); @@ -364,105 +375,129 @@ // Not waiting for coverage ranges intentionally as // plugin loading should not block the content rendering return Promise.all([diffRequest, assetRequest]) - .then(results => { - const diff = results[0]; - if (!diff) { - return Promise.resolve(); - } - this.filesWeblinks = this._getFilesWeblinks(diff); - return new Promise(resolve => { - const callback = event => { - const needsSyntaxHighlighting = event.detail && - event.detail.contentRendered; - if (needsSyntaxHighlighting) { - this.reporting.time(TimingLabel.SYNTAX); - this.$.syntaxLayer.process().finally(() => { - this.reporting.timeEnd(TimingLabel.SYNTAX); - this.reporting.timeEnd(TimingLabel.TOTAL); - resolve(); - }); - } else { + .then(results => { + const diff = results[0]; + if (!diff) { + return Promise.resolve(); + } + this.filesWeblinks = this._getFilesWeblinks(diff); + return new Promise(resolve => { + const callback = (event: CustomEvent) => { + const needsSyntaxHighlighting = + event.detail && event.detail.contentRendered; + if (needsSyntaxHighlighting) { + this.reporting.time(TimingLabel.SYNTAX); + this.$.syntaxLayer.process().finally(() => { + this.reporting.timeEnd(TimingLabel.SYNTAX); this.reporting.timeEnd(TimingLabel.TOTAL); resolve(); - } - this.removeEventListener('render', callback); - if (shouldReportMetric) { - // We report diffViewContentDisplayed only on reload caused - // by params changed - expected only on Diff Page. - this.reporting.diffViewContentDisplayed(); - } - }; - this.addEventListener('render', callback); - this.diff = diff; - }); - }) - .catch(err => { - console.warn('Error encountered loading diff:', err); - }) - .then(() => { this._loading = false; }); + }); + } else { + this.reporting.timeEnd(TimingLabel.TOTAL); + resolve(); + } + this.removeEventListener('render', callback); + if (shouldReportMetric) { + // We report diffViewContentDisplayed only on reload caused + // by params changed - expected only on Diff Page. + this.reporting.diffViewContentDisplayed(); + } + }; + this.addEventListener('render', callback); + this.diff = diff; + }); + }) + .catch(err => { + console.warn('Error encountered loading diff:', err); + }) + .then(() => { + this._loading = false; + }); } clear() { - this.$.jsAPI.disposeDiffLayers(this.path); + if (this.path) this.$.jsAPI.disposeDiffLayers(this.path); this._layers = []; } _getCoverageData() { - const {changeNum, path, patchRange: {basePatchNum, patchNum}} = this; - this.$.jsAPI.getCoverageAnnotationApi(). - then(coverageAnnotationApi => { - if (!coverageAnnotationApi) return; - const provider = coverageAnnotationApi.getCoverageProvider(); - return provider(changeNum, path, basePatchNum, patchNum) - .then(coverageRanges => { - if (!coverageRanges || - changeNum !== this.changeNum || - path !== this.path || - basePatchNum !== this.patchRange.basePatchNum || - patchNum !== this.patchRange.patchNum) { - return; - } + if (!this.changeNum) throw new Error('Missing required "changeNum" prop.'); + if (!this.path) throw new Error('Missing required "path" prop.'); + if (!this.patchRange) throw new Error('Missing required "patchRange".'); + const changeNum = this.changeNum; + const path = this.path; + // Coverage providers do not provide data for EDIT and PARENT patch sets. + const basePatchNum = isNumber(this.patchRange.basePatchNum) + ? this.patchRange.basePatchNum + : undefined; + const patchNum = isNumber(this.patchRange.patchNum) + ? this.patchRange.patchNum + : undefined; + this.$.jsAPI + .getCoverageAnnotationApi() + .then(coverageAnnotationApi => { + if (!coverageAnnotationApi) return; + const provider = coverageAnnotationApi.getCoverageProvider(); + if (!provider) return; + return provider(changeNum, path, basePatchNum, patchNum).then( + coverageRanges => { + if (!this.patchRange) throw new Error('Missing "patchRange".'); + if ( + !coverageRanges || + changeNum !== this.changeNum || + path !== this.path || + basePatchNum !== this.patchRange.basePatchNum || + patchNum !== this.patchRange.patchNum + ) { + return; + } - const existingCoverageRanges = this._coverageRanges; - this._coverageRanges = coverageRanges; + const existingCoverageRanges = this._coverageRanges; + this._coverageRanges = coverageRanges; - // Notify with existing coverage ranges - // in case there is some existing coverage data that needs to be removed - existingCoverageRanges.forEach(range => { - coverageAnnotationApi.notify( - path, - range.code_range.start_line, - range.code_range.end_line, - range.side); - }); + // Notify with existing coverage ranges + // in case there is some existing coverage data that needs to be removed + existingCoverageRanges.forEach(range => { + coverageAnnotationApi.notify( + path, + range.code_range.start_line, + range.code_range.end_line, + range.side + ); + }); - // Notify with new coverage data - coverageRanges.forEach(range => { - coverageAnnotationApi.notify( - path, - range.code_range.start_line, - range.code_range.end_line, - range.side); - }); - }); - }) - .catch(err => { - console.warn('Loading coverage ranges failed: ', err); - }); + // Notify with new coverage data + coverageRanges.forEach(range => { + coverageAnnotationApi.notify( + path, + range.code_range.start_line, + range.code_range.end_line, + range.side + ); + }); + } + ); + }) + .catch(err => { + console.warn('Loading coverage ranges failed: ', err); + }); } - _getFilesWeblinks(diff) { - // The return type is FilesWebLinks from gr-patch-range-select. - if (!this.commitRange) { - return {}; - } + _getFilesWeblinks(diff: DiffInfo) { + if (!this.projectName || !this.commitRange || !this.path) return {}; return { meta_a: GerritNav.getFileWebLinks( - this.projectName, this.commitRange.baseCommit, this.path, - {weblinks: diff && diff.meta_a && diff.meta_a.web_links}), + this.projectName, + this.commitRange.baseCommit, + this.path, + {weblinks: diff && diff.meta_a && diff.meta_a.web_links} + ), meta_b: GerritNav.getFileWebLinks( - this.projectName, this.commitRange.commit, this.path, - {weblinks: diff && diff.meta_b && diff.meta_b.web_links}), + this.projectName, + this.commitRange.commit, + this.path, + {weblinks: diff && diff.meta_b && diff.meta_b.web_links} + ), }; } @@ -472,12 +507,10 @@ this.$.syntaxLayer.cancel(); } - /** @return {!Array<!HTMLElement>} */ getCursorStops() { return this.$.diff.getCursorStops(); } - /** @return {boolean} */ isRangeSelected() { return this.$.diff.isRangeSelected(); } @@ -492,42 +525,39 @@ /** * Load and display blame information for the base of the diff. - * - * @return {Promise} A promise that resolves when blame finishes rendering. */ - loadBlame() { - return this.$.restAPI.getBlame(this.changeNum, this.patchRange.patchNum, - this.path, true) - .then(blame => { - if (!blame.length) { - this.dispatchEvent(new CustomEvent('show-alert', { + loadBlame(): Promise<BlameInfo[]> { + if (!this.changeNum) throw new Error('Missing required "changeNum" prop.'); + if (!this.patchRange) throw new Error('Missing required "patchRange".'); + if (!this.path) throw new Error('Missing required "path" property.'); + return this.$.restAPI + .getBlame(this.changeNum, this.patchRange.patchNum, this.path, true) + .then(blame => { + if (!blame || !blame.length) { + this.dispatchEvent( + new CustomEvent('show-alert', { detail: {message: MSG_EMPTY_BLAME}, - composed: true, bubbles: true, - })); - return Promise.reject(MSG_EMPTY_BLAME); - } + composed: true, + bubbles: true, + }) + ); + return Promise.reject(MSG_EMPTY_BLAME); + } - this._blame = blame; - }); + this._blame = blame; + return blame; + }); } - /** Unload blame information for the diff. */ clearBlame() { this._blame = null; } - /** - * The thread elements in this diff, in no particular order. - * - * @return {!Array<!HTMLElement>} - */ - getThreadEls() { - return Array.from( - this.$.diff.querySelectorAll('.comment-thread')); + getThreadEls(): GrCommentThread[] { + return Array.from(this.$.diff.querySelectorAll('.comment-thread')); } - /** @param {HTMLElement} el */ - addDraftAtLine(el) { + addDraftAtLine(el: Element) { this.$.diff.addDraftAtLine(el); } @@ -539,27 +569,29 @@ this.$.diff.expandAllContext(); } - /** @return {!Promise} */ _getLoggedIn() { return this.$.restAPI.getLoggedIn(); } - /** @return {boolean}} */ _canReload() { - return !!this.changeNum && !!this.patchRange && !!this.path && - !this.noAutoRender; + return ( + !!this.changeNum && !!this.patchRange && !!this.path && !this.noAutoRender + ); } // TODO(milutin): Use rest-api with fetchCacheURL instead of this. prefetchDiff() { - if (!!this.changeNum && !!this.patchRange && !!this.path - && this._fetchDiffPromise === null) { + if ( + !!this.changeNum && + !!this.patchRange && + !!this.path && + this._fetchDiffPromise === null + ) { this._fetchDiffPromise = this._getDiff(); } } - /** @return {!Promise<!Object>} */ - _getDiff() { + _getDiff(): Promise<DiffInfo> { if (this._fetchDiffPromise !== null) { const fetchDiffPromise = this._fetchDiffPromise; this._fetchDiffPromise = null; @@ -568,25 +600,33 @@ // Wrap the diff request in a new promise so that the error handler // rejects the promise, allowing the error to be handled in the .catch. return new Promise((resolve, reject) => { - this.$.restAPI.getDiff( + if (!this.changeNum) throw new Error('Missing required "changeNum".'); + if (!this.patchRange) throw new Error('Missing required "patchRange".'); + if (!this.path) throw new Error('Missing required "path" property.'); + this.$.restAPI + .getDiff( this.changeNum, this.patchRange.basePatchNum, this.patchRange.patchNum, this.path, this._getIgnoreWhitespace(), - reject) - .then(resolve); + reject + ) + .then(resolve); }); } - _handleGetDiffError(response) { + _handleGetDiffError(response: Response) { // Loading the diff may respond with 409 if the file is too large. In this // case, use a toast error.. if (response.status === 409) { - this.dispatchEvent(new CustomEvent('server-error', { - detail: {response}, - composed: true, bubbles: true, - })); + this.dispatchEvent( + new CustomEvent('server-error', { + detail: {response}, + composed: true, + bubbles: true, + }) + ); return; } @@ -599,28 +639,33 @@ return; } - this.dispatchEvent(new CustomEvent('page-error', { - detail: {response}, - composed: true, bubbles: true, - })); + this.dispatchEvent( + new CustomEvent('page-error', { + detail: {response}, + composed: true, + bubbles: true, + }) + ); } /** * Report info about the diff response. */ - _reportDiff(diff) { - if (!diff || !diff.content) { - return; - } + _reportDiff(diff?: DiffInfo) { + if (!diff || !diff.content) return; // Count the delta lines stemming from normal deltas, and from // due_to_rebase deltas. let nonRebaseDelta = 0; let rebaseDelta = 0; diff.content.forEach(chunk => { - if (chunk.ab) { return; } + if (chunk.ab) { + return; + } const deltaSize = Math.max( - chunk.a ? chunk.a.length : 0, chunk.b ? chunk.b.length : 0); + chunk.a ? chunk.a.length : 0, + chunk.b ? chunk.b.length : 0 + ); if (chunk.due_to_rebase) { rebaseDelta += deltaSize; } else { @@ -631,28 +676,28 @@ // Find the percent of the delta from due_to_rebase chunks rounded to two // digits. Diffs with no delta are considered 0%. const totalDelta = rebaseDelta + nonRebaseDelta; - const percentRebaseDelta = !totalDelta ? 0 : - Math.round(100 * rebaseDelta / totalDelta); + const percentRebaseDelta = !totalDelta + ? 0 + : Math.round((100 * rebaseDelta) / totalDelta); // Report the due_to_rebase percentage in the "diff" category when // applicable. + if (!this.patchRange) throw new Error('Missing required "patchRange".'); if (this.patchRange.basePatchNum === 'PARENT') { this.reporting.reportInteraction(EVENT_AGAINST_PARENT); } else if (percentRebaseDelta === 0) { this.reporting.reportInteraction(EVENT_ZERO_REBASE); } else { - this.reporting.reportInteraction(EVENT_NONZERO_REBASE, - {percentRebaseDelta}); + this.reporting.reportInteraction(EVENT_NONZERO_REBASE, { + percentRebaseDelta, + }); } } - /** - * @param {Object} diff - * @return {!Promise} - */ - _loadDiffAssets(diff) { + _loadDiffAssets(diff?: DiffInfo) { if (isImageDiff(diff)) { - return this._getImages(diff).then(images => { + // diff! is justified, because isImageDiff() returns false otherwise + return this._getImages(diff!).then(images => { this._baseImage = images.baseImage; this._revisionImage = images.revisionImage; }); @@ -663,17 +708,13 @@ } } - /** - * @param {Object} diff - * @return {boolean} - */ - _computeIsImageDiff(diff) { + _computeIsImageDiff(diff?: DiffInfo) { return isImageDiff(diff); } - _commentsChanged(newComments) { + _commentsChanged(newComments: TwoSidesComments) { const allComments = []; - for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) { + for (const side of [Side.LEFT, Side.RIGHT]) { // This is needed by the threading. for (const comment of newComments[side]) { comment.__commentSide = side; @@ -692,11 +733,7 @@ } } - /** - * @param {!Array<!Object>} comments - * @return {!Array<!Object>} Threads for the given comments. - */ - _createThreads(comments) { + _createThreads(comments: UIComment[]): CommentThread[] { const sortedComments = sortComments(comments); const threads = []; for (const comment of sortedComments) { @@ -704,7 +741,8 @@ // thread and append to it. if (comment.in_reply_to) { const thread = threads.find(thread => - thread.comments.some(c => c.id === comment.in_reply_to)); + thread.comments.some(c => c.id === comment.in_reply_to) + ); if (thread) { thread.comments.push(comment); continue; @@ -712,12 +750,11 @@ } // Otherwise, this comment starts its own thread. - const newThread = { - start_datetime: comment.updated, + if (!comment.__commentSide) throw new Error('Missing "__commentSide".'); + const newThread: CommentThread = { comments: [comment], commentSide: comment.__commentSide, patchNum: comment.patch_set, - rootId: comment.id || comment.__draftID, lineNum: comment.line, isOnParent: comment.side === 'PARENT', }; @@ -729,28 +766,29 @@ return threads; } - /** - * @param {Object} blame - * @return {boolean} - */ - _computeIsBlameLoaded(blame) { + _computeIsBlameLoaded(blame: BlameInfo[] | null) { return !!blame; } - /** - * @param {Object} diff - * @return {!Promise} - */ - _getImages(diff) { - return this.$.restAPI.getImagesForDiff(this.changeNum, diff, - this.patchRange); + _getImages(diff: DiffInfo) { + if (!this.changeNum) throw new Error('Missing required "changeNum" prop.'); + if (!this.patchRange) throw new Error('Missing required "patchRange".'); + return this.$.restAPI.getImagesForDiff( + this.changeNum, + diff, + this.patchRange + ); } - /** @param {CustomEvent} e */ - _handleCreateComment(e) { + _handleCreateComment(e: CustomEvent) { const {lineNum, side, patchNum, isOnParent, range} = e.detail; - const threadEl = this._getOrCreateThread(patchNum, lineNum, side, range, - isOnParent); + const threadEl = this._getOrCreateThread( + patchNum, + lineNum, + side, + range, + isOnParent + ); threadEl.addOrEditDraft(lineNum, range); this.reporting.recordDraftInteraction(); @@ -759,15 +797,14 @@ /** * Gets or creates a comment thread at a given location. * May provide a range, to get/create a range comment. - * - * @param {string} patchNum - * @param {?number} lineNum - * @param {string} commentSide - * @param {Gerrit.Range|undefined} range - * @param {boolean} isOnParent - * @return {!Object} */ - _getOrCreateThread(patchNum, lineNum, commentSide, range, isOnParent) { + _getOrCreateThread( + patchNum: PatchSetNum, + lineNum: LineNumber | undefined, + commentSide: Side, + range?: CommentRange, + isOnParent?: boolean + ): GrCommentThread { let threadEl = this._getThreadEl(lineNum, commentSide, range); if (!threadEl) { threadEl = this._createThreadElement({ @@ -783,18 +820,18 @@ return threadEl; } - _attachThreadElement(threadEl) { + _attachThreadElement(threadEl: Element) { this.$.diff.appendChild(threadEl); } _clearThreads() { for (const threadEl of this.getThreadEls()) { const parent = threadEl.parentNode; - parent.removeChild(threadEl); + if (parent) parent.removeChild(threadEl); } } - _createThreadElement(thread) { + _createThreadElement(thread: CommentThread) { const threadEl = document.createElement('gr-comment-thread'); threadEl.className = 'comment-thread'; threadEl.setAttribute('slot', `${thread.commentSide}-${thread.lineNum}`); @@ -804,9 +841,12 @@ threadEl.parentIndex = this._parentIndex; // Use path before renmaing when comment added on the left when comparing // two patch sets (not against base) - if (this.file && this.file.basePath - && thread.commentSide === GrDiffBuilder.Side.LEFT - && !thread.isOnParent) { + if ( + this.file && + this.file.basePath && + thread.commentSide === Side.LEFT && + !thread.isOnParent + ) { threadEl.path = this.file.basePath; } else { threadEl.path = this.path; @@ -814,20 +854,14 @@ threadEl.changeNum = this.changeNum; threadEl.patchNum = thread.patchNum; threadEl.showPatchset = false; - threadEl.lineNum = thread.lineNum; - const rootIdChangedListener = changeEvent => { - thread.rootId = changeEvent.detail.value; - }; - threadEl.addEventListener('root-id-changed', rootIdChangedListener); + // GrCommentThread does not understand 'FILE', but requires undefined. + threadEl.lineNum = thread.lineNum !== 'FILE' ? thread.lineNum : undefined; threadEl.projectName = this.projectName; threadEl.range = thread.range; - const threadDiscardListener = e => { - const threadEl = /** @type {!Node} */ (e.currentTarget); - + const threadDiscardListener = (e: Event) => { + const threadEl = e.currentTarget as Element; const parent = threadEl.parentNode; - parent.removeChild(threadEl); - - threadEl.removeEventListener('root-id-changed', rootIdChangedListener); + if (parent) parent.removeChild(threadEl); threadEl.removeEventListener('thread-discard', threadDiscardListener); }; threadEl.addEventListener('thread-discard', threadDiscardListener); @@ -837,132 +871,132 @@ /** * Gets a comment thread element at a given location. * May provide a range, to get a range comment. - * - * @param {?number} lineNum - * @param {string} commentSide - * @param {!Gerrit.Range=} range - * @return {?Node} */ - _getThreadEl(lineNum, commentSide, range = undefined) { - let line; - if (commentSide === GrDiffBuilder.Side.LEFT) { + _getThreadEl( + lineNum: LineNumber | undefined, + commentSide: Side, + range?: CommentRange + ): GrCommentThread | null { + let line: LineInfo; + if (commentSide === Side.LEFT) { line = {beforeNumber: lineNum}; - } else if (commentSide === GrDiffBuilder.Side.RIGHT) { + } else if (commentSide === Side.RIGHT) { line = {afterNumber: lineNum}; } else { throw new Error(`Unknown side: ${commentSide}`); } - function matchesRange(threadEl) { - const threadRange = /** @type {!Gerrit.Range} */( - JSON.parse(threadEl.getAttribute('range'))); + function matchesRange(threadEl: GrCommentThread) { + const rangeAtt = threadEl.getAttribute('range'); + const threadRange = rangeAtt ? JSON.parse(rangeAtt) : undefined; return rangesEqual(threadRange, range); } const filteredThreadEls = this._filterThreadElsForLocation( - this.getThreadEls(), line, commentSide).filter(matchesRange); + this.getThreadEls(), + line, + commentSide + ).filter(matchesRange); return filteredThreadEls.length ? filteredThreadEls[0] : null; } - /** - * @param {!Array<!HTMLElement>} threadEls - * @param {!{beforeNumber: (number|string|undefined|null), - * afterNumber: (number|string|undefined|null)}} - * lineInfo - * @param {!DiffSide=} side The side (LEFT, RIGHT) for - * which to return the threads. - * @return {!Array<!HTMLElement>} The thread elements matching the given - * location. - */ - _filterThreadElsForLocation(threadEls, lineInfo, side) { - function matchesLeftLine(threadEl) { - return threadEl.getAttribute('comment-side') == - DiffSide.LEFT && - threadEl.getAttribute('line-num') == lineInfo.beforeNumber; + _filterThreadElsForLocation( + threadEls: GrCommentThread[], + lineInfo: LineInfo, + side: Side + ) { + function matchesLeftLine(threadEl: GrCommentThread) { + return ( + threadEl.getAttribute('comment-side') === Side.LEFT && + threadEl.getAttribute('line-num') === String(lineInfo.beforeNumber) + ); } - function matchesRightLine(threadEl) { - return threadEl.getAttribute('comment-side') == - DiffSide.RIGHT && - threadEl.getAttribute('line-num') == lineInfo.afterNumber; + function matchesRightLine(threadEl: GrCommentThread) { + return ( + threadEl.getAttribute('comment-side') === Side.RIGHT && + threadEl.getAttribute('line-num') === String(lineInfo.afterNumber) + ); } - function matchesFileComment(threadEl) { - return threadEl.getAttribute('comment-side') == side && - // line/range comments have 1-based line set, if line is falsy it's - // a file comment - !threadEl.getAttribute('line-num'); + function matchesFileComment(threadEl: GrCommentThread) { + return ( + threadEl.getAttribute('comment-side') === side && + // line/range comments have 1-based line set, if line is falsy it's + // a file comment + !threadEl.getAttribute('line-num') + ); } // Select the appropriate matchers for the desired side and line // If side is BOTH, we want both the left and right matcher. - const matchers = []; - if (side !== DiffSide.RIGHT) { + const matchers: ((thread: GrCommentThread) => boolean)[] = []; + if (side !== Side.RIGHT) { matchers.push(matchesLeftLine); } - if (side !== DiffSide.LEFT) { + if (side !== Side.LEFT) { matchers.push(matchesRightLine); } - if (lineInfo.afterNumber === 'FILE' || - lineInfo.beforeNumber === 'FILE') { + if (lineInfo.afterNumber === 'FILE' || lineInfo.beforeNumber === 'FILE') { matchers.push(matchesFileComment); } return threadEls.filter(threadEl => - matchers.some(matcher => matcher(threadEl))); + matchers.some(matcher => matcher(threadEl)) + ); } - _getIgnoreWhitespace() { + _getIgnoreWhitespace(): IgnoreWhitespaceType { if (!this.prefs || !this.prefs.ignore_whitespace) { - return WHITESPACE_IGNORE_NONE; + return IgnoreWhitespaceType.IGNORE_NONE; } return this.prefs.ignore_whitespace; } + @observe( + 'prefs.ignore_whitespace', + '_loadedWhitespaceLevel', + 'noRenderOnPrefsChange' + ) _whitespaceChanged( - preferredWhitespaceLevel, loadedWhitespaceLevel, - noRenderOnPrefsChange) { - // Polymer 2: check for undefined - if ([ - preferredWhitespaceLevel, - loadedWhitespaceLevel, - noRenderOnPrefsChange, - ].includes(undefined)) { - return; - } + preferredWhitespaceLevel?: IgnoreWhitespaceType, + loadedWhitespaceLevel?: IgnoreWhitespaceType, + noRenderOnPrefsChange?: boolean + ) { + if (preferredWhitespaceLevel === undefined) return; + if (loadedWhitespaceLevel === undefined) return; + if (noRenderOnPrefsChange === undefined) return; this._fetchDiffPromise = null; - if (preferredWhitespaceLevel !== loadedWhitespaceLevel && - !noRenderOnPrefsChange) { + if ( + preferredWhitespaceLevel !== loadedWhitespaceLevel && + !noRenderOnPrefsChange + ) { this.reload(); } } - _syntaxHighlightingChanged(noRenderOnPrefsChange, prefsChangeRecord) { - // Polymer 2: check for undefined - if ([ - noRenderOnPrefsChange, - prefsChangeRecord, - ].includes(undefined)) { - return; - } + @observe('noRenderOnPrefsChange', 'prefs.*') + _syntaxHighlightingChanged( + noRenderOnPrefsChange?: boolean, + prefsChangeRecord?: PolymerDeepPropertyChange< + DiffPreferencesInfo, + DiffPreferencesInfo + > + ) { + if (noRenderOnPrefsChange === undefined) return; + if (prefsChangeRecord === undefined) return; + if (prefsChangeRecord.path !== 'prefs.syntax_highlighting') return; - if (prefsChangeRecord.path !== 'prefs.syntax_highlighting') { - return; - } - - if (!noRenderOnPrefsChange) { - this.reload(); - } + if (!noRenderOnPrefsChange) this.reload(); } - /** - * @param {Object} patchRangeRecord - * @return {number|null} - */ - _computeParentIndex(patchRangeRecord) { + _computeParentIndex( + patchRangeRecord: PolymerDeepPropertyChange<PatchRange, PatchRange> + ) { if (!patchRangeRecord.base) return null; - return isMergeParent(patchRangeRecord.base.basePatchNum) ? - getParentIndex(patchRangeRecord.base.basePatchNum) : null; + return isMergeParent(patchRangeRecord.base.basePatchNum) + ? getParentIndex(patchRangeRecord.base.basePatchNum) + : null; } - _handleCommentSave(e) { + _handleCommentSave(e: CustomEvent) { const comment = e.detail.comment; const side = e.detail.comment.__commentSide; const idx = this._findDraftIndex(comment, side); @@ -970,44 +1004,41 @@ this._handleCommentSaveOrDiscard(); } - _handleCommentDiscard(e) { + _handleCommentDiscard(e: CustomEvent) { const comment = e.detail.comment; this._removeComment(comment); this._handleCommentSaveOrDiscard(); } - /** - * Closure annotation for Polymer.prototype.push is off. Submitted PR: - * https://github.com/Polymer/polymer/pull/4776 - * but for not suppressing annotations. - * - * @suppress {checkTypes} - */ - _handleCommentUpdate(e) { + _handleCommentUpdate(e: CustomEvent) { const comment = e.detail.comment; const side = e.detail.comment.__commentSide; let idx = this._findCommentIndex(comment, side); if (idx === -1) { idx = this._findDraftIndex(comment, side); } - if (idx !== -1) { // Update draft or comment. + if (idx !== -1) { + // Update draft or comment. this.set(['comments', side, idx], comment); - } else { // Create new draft. + } else { + // Create new draft. this.push(['comments', side], comment); } } _handleCommentSaveOrDiscard() { - this.dispatchEvent(new CustomEvent( - 'diff-comments-modified', {bubbles: true, composed: true})); + this.dispatchEvent( + new CustomEvent('diff-comments-modified', {bubbles: true, composed: true}) + ); } - _removeComment(comment) { + _removeComment(comment: UIComment) { const side = comment.__commentSide; + if (!side) throw new Error('Missing required "side" in comment.'); this._removeCommentFromSide(comment, side); } - _removeCommentFromSide(comment, side) { + _removeCommentFromSide(comment: Comment, side: Side) { let idx = this._findCommentIndex(comment, side); if (idx === -1) { idx = this._findDraftIndex(comment, side); @@ -1017,50 +1048,64 @@ } } - /** @return {number} */ - _findCommentIndex(comment, side) { - if (!comment.id || !this.comments[side]) { + _findCommentIndex(comment: Comment, side: Side) { + if (!comment.id || !this.comments || !this.comments[side]) { return -1; } return this.comments[side].findIndex(item => item.id === comment.id); } - /** @return {number} */ - _findDraftIndex(comment, side) { - if (!comment.__draftID || !this.comments[side]) { + _findDraftIndex(comment: Comment, side: Side) { + if ( + !isDraft(comment) || + !comment.__draftID || + !this.comments || + !this.comments[side] + ) { return -1; } return this.comments[side].findIndex( - item => item.__draftID === comment.__draftID); + item => isDraft(item) && item.__draftID === comment.__draftID + ); } - _isSyntaxHighlightingEnabled(preferenceChangeRecord, diff) { - if (!preferenceChangeRecord || - !preferenceChangeRecord.base || - !preferenceChangeRecord.base.syntax_highlighting || - !diff) { + _isSyntaxHighlightingEnabled( + preferenceChangeRecord?: PolymerDeepPropertyChange< + DiffPreferencesInfo, + DiffPreferencesInfo + >, + diff?: DiffInfo + ) { + if ( + !preferenceChangeRecord || + !preferenceChangeRecord.base || + !preferenceChangeRecord.base.syntax_highlighting || + !diff + ) { return false; } - return !this._anyLineTooLong(diff) && - this.$.diff.getDiffLength(diff) <= SYNTAX_MAX_DIFF_LENGTH; + return ( + !this._anyLineTooLong(diff) && + this.$.diff.getDiffLength(diff) <= SYNTAX_MAX_DIFF_LENGTH + ); } /** - * @return {boolean} whether any of the lines in diff are longer + * @return whether any of the lines in diff are longer * than SYNTAX_MAX_LINE_LENGTH. */ - _anyLineTooLong(diff) { + _anyLineTooLong(diff?: DiffInfo) { if (!diff) return false; return diff.content.some(section => { - const lines = section.ab ? - section.ab : - (section.a || []).concat(section.b || []); + const lines = section.ab + ? section.ab + : (section.a || []).concat(section.b || []); return lines.some(line => line.length >= SYNTAX_MAX_LINE_LENGTH); }); } _listenToViewportRender() { - const renderUpdateListener = start => { + const renderUpdateListener: DiffLayerListener = start => { if (start > NUM_OF_LINES_THRESHOLD_FOR_VIEWPORT) { this.reporting.diffViewDisplayed(); this.$.syntaxLayer.removeListener(renderUpdateListener); @@ -1079,31 +1124,31 @@ this.reporting.timeEnd(TimingLabel.CONTENT); } - _handleNormalizeRange(event) { - this.reporting.reportInteraction('normalize-range', - { - side: event.detail.side, - lineNum: event.detail.lineNum, - }); + _handleNormalizeRange(event: CustomEvent) { + this.reporting.reportInteraction('normalize-range', { + side: event.detail.side, + lineNum: event.detail.lineNum, + }); } - _handleDiffContextExpanded(event) { - this.reporting.reportInteraction( - 'diff-context-expanded', {numLines: event.detail.numLines} - ); + _handleDiffContextExpanded(event: CustomEvent) { + this.reporting.reportInteraction('diff-context-expanded', { + numLines: event.detail.numLines, + }); } /** * Find the last chunk for the given side. * - * @param {!Object} diff - * @param {boolean} leftSide true if checking the base of the diff, - * false if testing the revision. - * @return {Object|null} returns the chunk object or null if there was - * no chunk for that side. + * @param leftSide true if checking the base of the diff, + * false if testing the revision. + * @return returns the chunk object or null if there was + * no chunk for that side. */ - _lastChunkForSide(diff, leftSide) { - if (!diff.content.length) { return null; } + _lastChunkForSide(diff: DiffInfo | undefined, leftSide: boolean) { + if (!diff?.content.length) { + return null; + } let chunkIndex = diff.content.length; let chunk; @@ -1113,19 +1158,20 @@ chunkIndex--; chunk = diff.content[chunkIndex]; } while ( - // We haven't reached the beginning. + // We haven't reached the beginning. chunkIndex >= 0 && - - // The chunk doesn't have both sides. - !chunk.ab && - - // The chunk doesn't have the given side. - ((leftSide && (!chunk.a || !chunk.a.length)) || - (!leftSide && (!chunk.b || !chunk.b.length)))); + // The chunk doesn't have both sides. + !chunk.ab && + // The chunk doesn't have the given side. + ((leftSide && (!chunk.a || !chunk.a.length)) || + (!leftSide && (!chunk.b || !chunk.b.length))) + ); // If we reached the beginning of the diff and failed to find a chunk // with the given side, return null. - if (chunkIndex === -1) { return null; } + if (chunkIndex === -1) { + return null; + } return chunk; } @@ -1133,32 +1179,50 @@ /** * Check whether the specified side of the diff has a trailing newline. * - * @param {!Object} diff - * @param {boolean} leftSide true if checking the base of the diff, - * false if testing the revision. - * @return {boolean|null} Return true if the side has a trailing newline. - * Return false if it doesn't. Return null if not applicable (for - * example, if the diff has no content on the specified side). + * @param leftSide true if checking the base of the diff, + * false if testing the revision. + * @return Return true if the side has a trailing newline. + * Return false if it doesn't. Return null if not applicable (for + * example, if the diff has no content on the specified side). */ - _hasTrailingNewlines(diff, leftSide) { + _hasTrailingNewlines(diff: DiffInfo | undefined, leftSide: boolean) { const chunk = this._lastChunkForSide(diff, leftSide); - if (!chunk) { return null; } + if (!chunk) return null; let lines; if (chunk.ab) { lines = chunk.ab; } else { lines = leftSide ? chunk.a : chunk.b; } + if (!lines) return null; return lines[lines.length - 1] === ''; } - _showNewlineWarningLeft(diff) { + _showNewlineWarningLeft(diff?: DiffInfo) { return this._hasTrailingNewlines(diff, true) === false; } - _showNewlineWarningRight(diff) { + _showNewlineWarningRight(diff?: DiffInfo) { return this._hasTrailingNewlines(diff, false) === false; } } -customElements.define(GrDiffHost.is, GrDiffHost); +declare global { + interface HTMLElementTagNameMap { + 'gr-diff-host': GrDiffHost; + } +} + +// TODO(TS): Be more specific than CustomEvent, which has detail:any. +declare global { + interface HTMLElementEventMap { + render: CustomEvent; + 'normalize-range': CustomEvent; + 'diff-context-expanded': CustomEvent; + 'create-comment': CustomEvent; + 'comment-discard': CustomEvent; + 'comment-update': CustomEvent; + 'comment-save': CustomEvent; + 'root-id-changed': CustomEvent; + } +}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js index 11539b1..ac96ba2 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -20,8 +20,8 @@ import {GrDiffBuilderImage} from '../gr-diff-builder/gr-diff-builder-image.js'; import {GerritNav} from '../../core/gr-navigation/gr-navigation.js'; import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js'; -import {DiffSide} from '../gr-diff/gr-diff-utils.js'; import {sortComments} from '../gr-comment-api/gr-comment-api.js'; +import {Side} from '../../../constants/constants.js'; const basicFixture = fixtureFromElement('gr-diff-host'); @@ -36,6 +36,8 @@ async getLoggedIn() { return getLoggedIn; }, }); element = basicFixture.instantiate(); + element.changeNum = 123; + element.path = 'some/path'; sinon.stub(element.reporting, 'time'); sinon.stub(element.reporting, 'timeEnd'); }); @@ -47,6 +49,8 @@ getDiffLayers() { return pluginLayers; }, }); element = basicFixture.instantiate(); + element.changeNum = 123; + element.path = 'some/path'; }); test('plugin layers requested', () => { element.patchRange = {}; @@ -172,7 +176,6 @@ ], }; - element._removeComment({}); // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem // to believe that one object deepEquals another even when they do :-/. assert.equal(JSON.stringify(element.comments), JSON.stringify({ @@ -249,12 +252,20 @@ }); test('thread-discard handling', () => { - const threads = [ - {comments: [{id: 4711}]}, - {comments: [{id: 42}]}, - ]; + const threads = element._createThreads([ + { + id: 4711, + __commentSide: 'left', + updated: '2015-12-20 15:01:20.396000000', + }, + { + id: 42, + __commentSide: 'left', + updated: '2017-12-20 15:01:20.396000000', + }, + ]); element._parentIndex = 1; - element.changeNum = '2'; + element.changeNum = 2; element.path = 'some/path'; element.projectName = 'Some project'; const threadEls = threads.map( @@ -268,8 +279,8 @@ return threadEl; }); assert.equal(threadEls.length, 2); - assert.equal(threadEls[0].rootId, 4711); - assert.equal(threadEls[1].rootId, 42); + assert.equal(threadEls[0].comments[0].id, 4711); + assert.equal(threadEls[1].comments[0].id, 42); for (const threadEl of threadEls) { element.appendChild(threadEl); } @@ -279,7 +290,7 @@ const attachedThreads = element.queryAllEffectiveChildren( 'gr-comment-thread'); assert.equal(attachedThreads.length, 1); - assert.equal(attachedThreads[0].rootId, 42); + assert.equal(attachedThreads[0].comments[0].id, 42); }); suite('render reporting', () => { @@ -393,6 +404,8 @@ setup(() => { getLoggedIn = false; element = basicFixture.instantiate(); + element.changeNum = 123; + element.path = 'some/path'; }); test('reload() loads files weblinks', () => { @@ -852,6 +865,8 @@ suite('blame', () => { setup(() => { element = basicFixture.instantiate(); + element.changeNum = 123; + element.path = 'some/path'; }); test('clearBlame', () => { @@ -933,9 +948,8 @@ }); test('passes in changeNum', () => { - const value = '12345'; - element.changeNum = value; - assert.equal(element.$.diff.changeNum, value); + element.changeNum = 12345; + assert.equal(element.$.diff.changeNum, 12345); }); test('passes in noAutoRender', () => { @@ -963,9 +977,8 @@ }); test('passes in changeNum', () => { - const value = '12345'; - element.changeNum = value; - assert.equal(element.$.diff.changeNum, value); + element.changeNum = 12345; + assert.equal(element.$.diff.changeNum, 12345); }); test('passes in projectName', () => { @@ -1016,6 +1029,7 @@ setup(() => { element = basicFixture.instantiate(); + element.changeNum = 123; element.path = 'file.txt'; element.patchRange = {basePatchNum: 1}; reportStub = sinon.stub(element.reporting, 'reportInteraction'); @@ -1167,23 +1181,17 @@ assert.equal(actualThreads.length, 2); - assert.equal( - actualThreads[0].start_datetime, '2015-12-23 15:00:20.396000000'); assert.equal(actualThreads[0].commentSide, 'left'); assert.equal(actualThreads[0].comments.length, 2); assert.deepEqual(actualThreads[0].comments[0], comments[0]); assert.deepEqual(actualThreads[0].comments[1], comments[1]); assert.equal(actualThreads[0].patchNum, undefined); - assert.equal(actualThreads[0].rootId, 'sallys_confession'); assert.equal(actualThreads[0].lineNum, 1); - assert.equal( - actualThreads[1].start_datetime, '2015-12-20 15:01:20.396000000'); assert.equal(actualThreads[1].commentSide, 'left'); assert.equal(actualThreads[1].comments.length, 1); assert.deepEqual(actualThreads[1].comments[0], comments[2]); assert.equal(actualThreads[1].patchNum, undefined); - assert.equal(actualThreads[1].rootId, 'new_draft'); assert.equal(actualThreads[1].lineNum, undefined); }); @@ -1205,7 +1213,6 @@ const expectedThreads = [ { - start_datetime: '2015-12-24 15:00:10.396000000', commentSide: 'left', comments: [{ id: 'betsys_confession', @@ -1222,7 +1229,6 @@ line: 1, }], patchNum: 5, - rootId: 'betsys_confession', range: { start_line: 1, start_character: 1, @@ -1264,14 +1270,12 @@ id: 'sallys_confession', message: 'i like you, jack', updated: '2015-12-23 15:00:20.396000000', - // line: 1, - // __commentSide: 'left', + __commentSide: 'left', }, { id: 'jacks_reply', message: 'i like you, too', updated: '2015-12-24 15:01:20.396000000', - // __commentSide: 'left', - // line: 1, + __commentSide: 'left', in_reply_to: 'sallys_confession', }, ]; @@ -1375,9 +1379,9 @@ const threads = []; assert.deepEqual(element._filterThreadElsForLocation(threads, line), []); assert.deepEqual(element._filterThreadElsForLocation(threads, line, - DiffSide.LEFT), []); + Side.LEFT), []); assert.deepEqual(element._filterThreadElsForLocation(threads, line, - DiffSide.RIGHT), []); + Side.RIGHT), []); }); test('_filterThreadElsForLocation for line comments', () => { @@ -1403,9 +1407,9 @@ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line), [l3, r5]); assert.deepEqual(element._filterThreadElsForLocation(threadEls, line, - DiffSide.LEFT), [l3]); + Side.LEFT), [l3]); assert.deepEqual(element._filterThreadElsForLocation(threadEls, line, - DiffSide.RIGHT), [r5]); + Side.RIGHT), [r5]); }); test('_filterThreadElsForLocation for file comments', () => { @@ -1423,11 +1427,11 @@ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line), [l, r]); assert.deepEqual(element._filterThreadElsForLocation(threadEls, line, - DiffSide.BOTH), [l, r]); + Side.BOTH), [l, r]); assert.deepEqual(element._filterThreadElsForLocation(threadEls, line, - DiffSide.LEFT), [l]); + Side.LEFT), [l]); assert.deepEqual(element._filterThreadElsForLocation(threadEls, line, - DiffSide.RIGHT), [r]); + Side.RIGHT), [r]); }); suite('syntax layer with syntax_highlighting on', () => { @@ -1441,6 +1445,8 @@ }; element.patchRange = {}; element.prefs = prefs; + element.changeNum = 123; + element.path = 'some/path'; }); test('gr-diff-host provides syntax highlighting layer to gr-diff', () => { @@ -1551,6 +1557,8 @@ }, }); element = basicFixture.instantiate(); + element.changeNum = 123; + element.path = 'some/path'; const prefs = { line_length: 10, show_tabs: true,
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts index a85af34..39e12ca 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
@@ -31,7 +31,7 @@ import {CancelablePromise, util} from '../../../scripts/util'; import {customElement, property} from '@polymer/decorators'; import {DiffContent} from '../../../types/common'; -import {DiffSide} from '../gr-diff/gr-diff-utils'; +import {Side} from '../../../constants/constants'; const WHOLE_FILE = -1; @@ -567,8 +567,8 @@ for (let i = 0; i < numLines; i++) { // If this line should not be collapsed. if ( - this.keyLocations[DiffSide.LEFT][leftOffset + i] || - this.keyLocations[DiffSide.RIGHT][rightOffset + i] + this.keyLocations[Side.LEFT][leftOffset + i] || + this.keyLocations[Side.RIGHT][rightOffset + i] ) { // If any lines have been accumulated into the chunk leading up to // this non-collapse line, then add them as a chunk and start a new
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 0aa42c3..8984dc8 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
@@ -18,17 +18,12 @@ import {CommentRange} from '../../../types/common'; import {FILE, LineNumber} from './gr-diff-line'; -export enum DiffSide { - LEFT = 'left', - RIGHT = 'right', -} - /** * Compare two ranges. Either argument may be falsy, but will only return * true if both are falsy or if neither are falsy and have the same position * values. */ -export function rangesEqual(a: CommentRange, b: CommentRange): boolean { +export function rangesEqual(a?: CommentRange, b?: CommentRange): boolean { if (!a && !b) { return true; }
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 8ac47a2..159c056 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -27,7 +27,7 @@ import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin'; import {htmlTemplate} from './gr-diff_html'; import {FILE, LineNumber} from './gr-diff-line'; -import {DiffSide, getLineNumber, rangesEqual} from './gr-diff-utils'; +import {getLineNumber, 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'; @@ -95,7 +95,7 @@ const RENDER_DIFF_TABLE_DEBOUNCE_NAME = 'renderDiffTable'; -interface LineOfInterest { +export interface LineOfInterest { number: number; leftSide: boolean; } @@ -534,7 +534,7 @@ this.dispatchEvent( new CustomEvent('line-selected', { detail: { - side: el.classList.contains('left') ? DiffSide.LEFT : DiffSide.RIGHT, + side: el.classList.contains('left') ? Side.LEFT : Side.RIGHT, number: el.getAttribute('data-value'), path: this.path, }, @@ -614,7 +614,7 @@ ); return false; } - const patchNum = el.classList.contains(DiffSide.LEFT) + const patchNum = el.classList.contains(Side.LEFT) ? this.patchRange.basePatchNum : this.patchRange.patchNum; @@ -717,7 +717,7 @@ let patchNum = this.patchRange.patchNum; if ( - (lineEl.classList.contains(DiffSide.LEFT) || + (lineEl.classList.contains(Side.LEFT) || contentEl.classList.contains('remove')) && this.patchRange.basePatchNum !== 'PARENT' && !isMergeParent(this.patchRange.basePatchNum) @@ -730,7 +730,7 @@ _getIsParentCommentByLineAndContent(lineEl: Element, contentEl: Element) { if (!this.patchRange) throw Error('patch range not set'); return ( - (lineEl.classList.contains(DiffSide.LEFT) || + (lineEl.classList.contains(Side.LEFT) || contentEl.classList.contains('remove')) && (this.patchRange.basePatchNum === 'PARENT' || isMergeParent(this.patchRange.basePatchNum)) @@ -740,7 +740,7 @@ _getCommentSideByLineAndContent(lineEl: Element, contentEl: Element): Side { let side = Side.RIGHT; if ( - lineEl.classList.contains(DiffSide.LEFT) || + lineEl.classList.contains(Side.LEFT) || contentEl.classList.contains('remove') ) { side = Side.LEFT;
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts index b6e1885..2b997fd 100644 --- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts +++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
@@ -51,7 +51,7 @@ DropDownValueChangeEvent, GrDropdownList, } from '../../shared/gr-dropdown-list/gr-dropdown-list'; -import {WebLink} from '../../core/gr-navigation/gr-navigation'; +import {GeneratedWebLink} from '../../core/gr-navigation/gr-navigation'; // Maximum length for patch set descriptions. const PATCH_DESC_MAX_LENGTH = 500; @@ -63,9 +63,9 @@ export type PatchRangeChangeEvent = CustomEvent<PatchRangeChangeDetail>; -interface FilesWebLinks { - meta_a: WebLink[]; - meta_b: WebLink[]; +export interface FilesWebLinks { + meta_a: GeneratedWebLink[]; + meta_b: GeneratedWebLink[]; } export interface GrPatchRangeSelect {
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.js b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.js index 2d0924a..eb9f47d 100644 --- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.js +++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.js
@@ -25,6 +25,7 @@ import {createCommentApiMockWithTemplateElement} from '../../../test/mocks/comment-api'; import {html} from '@polymer/polymer/lib/utils/html-tag.js'; import {SPECIAL_PATCH_SET_NUM} from '../../../utils/patch-set-util.js'; +import {ChangeComments} from '../gr-comment-api/gr-comment-api.js'; const commentApiMockElement = createCommentApiMockWithTemplateElement( 'gr-patch-range-select-comment-api-mock', html` @@ -355,7 +356,7 @@ test('_computePatchSetCommentsString', () => { // Test string with unresolved comments. - element.changeComments._comments = { + const comments = { foo: [{ id: '27dcee4d_f7b77cfa', message: 'test', @@ -377,6 +378,7 @@ }], abc: [], }; + element.changeComments = new ChangeComments(comments, {}, {}, 123); assert.equal(element._computePatchSetCommentsString( element.changeComments, 1), ' (3 comments, 1 unresolved)');
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts index 4bfacd4..1c51f99 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -27,10 +27,17 @@ CustomKeyboardEvent, KeyboardShortcutMixin, } from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin'; -import {sortComments} from '../../diff/gr-comment-api/gr-comment-api'; +import { + isDraft, + isRobot, + sortComments, + UIComment, + UIDraft, + UIRobot, +} from '../../diff/gr-comment-api/gr-comment-api'; import {GerritNav} from '../../core/gr-navigation/gr-navigation'; import {appContext} from '../../../services/app-context'; -import {CommentSide, SpecialFilePath} from '../../../constants/constants'; +import {CommentSide, Side, SpecialFilePath} from '../../../constants/constants'; import {computeDisplayPath} from '../../../utils/path-list-util'; import {customElement, observe, property} from '@polymer/decorators'; import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api'; @@ -42,7 +49,7 @@ RepoName, UrlEncodedCommentId, } from '../../../types/common'; -import {Comment, GrComment, RobotComment} from '../gr-comment/gr-comment'; +import {GrComment} from '../gr-comment/gr-comment'; import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces'; import {GrStorage, StorageLocation} from '../gr-storage/gr-storage'; @@ -102,7 +109,7 @@ changeNum?: NumericChangeId; @property({type: Array}) - comments: Comment[] = []; + comments: UIComment[] = []; @property({type: Object, reflectToAttribute: true}) range?: CommentRange; @@ -111,7 +118,7 @@ keyEventTarget: HTMLElement = document.body; @property({type: String, reflectToAttribute: true}) - commentSide?: string; + commentSide?: Side; @property({type: String}) patchNum?: PatchSetNum; @@ -151,10 +158,10 @@ _showActions?: boolean; @property({type: Object}) - _lastComment?: Comment; + _lastComment?: UIComment; @property({type: Array}) - _orderedComments: Comment[] = []; + _orderedComments: UIComment[] = []; @property({type: Object}) _projectConfig?: ConfigInfo; @@ -197,7 +204,7 @@ addOrEditDraft(lineNum?: number, rangeParam?: CommentRange) { const lastComment = this.comments[this.comments.length - 1] || {}; - if (lastComment.__draft) { + if (isDraft(lastComment)) { const commentEl = this._commentElWithDraftID( lastComment.id || lastComment.__draftID ); @@ -237,7 +244,7 @@ _getDiffUrlForPath(path: string) { if (!this.changeNum) throw new Error('changeNum is missing'); if (!this.projectName) throw new Error('projectName is missing'); - if (this.comments[0].__draft) { + if (isDraft(this.comments[0])) { return GerritNav.getUrlForDiffById( this.changeNum, this.projectName, @@ -251,14 +258,15 @@ } _getDiffUrlForComment( - projectName: RepoName, - changeNum: NumericChangeId, - path: string, - patchNum: PatchSetNum + projectName?: RepoName, + changeNum?: NumericChangeId, + path?: string, + patchNum?: PatchSetNum ) { + if (!projectName || !changeNum || !path) return undefined; if ( (this.comments.length && this.comments[0].side === 'PARENT') || - this.comments[0].__draft + isDraft(this.comments[0]) ) { return GerritNav.getUrlForDiffById( changeNum, @@ -271,9 +279,7 @@ } const id = this.comments[0].id; if (!id) throw new Error('A published comment is missing the id.'); - if (!this.changeNum) throw new Error('changeNum is missing'); - if (!this.projectName) throw new Error('projectName is missing'); - return GerritNav.getUrlForComment(this.changeNum, this.projectName, id); + return GerritNav.getUrlForComment(changeNum, projectName, id); } _isPatchsetLevelComment(path: string) { @@ -315,19 +321,19 @@ if (this._orderedComments.length) { this._lastComment = this._getLastComment(); this.unresolved = this._lastComment.unresolved; - this.hasDraft = this._lastComment.__draft; - this.isRobotComment = !!(this._lastComment as RobotComment).robot_id; + this.hasDraft = isDraft(this._lastComment); + this.isRobotComment = isRobot(this._lastComment); } } - _shouldDisableAction(_showActions?: boolean, _lastComment?: Comment) { - return !_showActions || !_lastComment || !!_lastComment.__draft; + _shouldDisableAction(_showActions?: boolean, _lastComment?: UIComment) { + return !_showActions || !_lastComment || isDraft(_lastComment); } - _hideActions(_showActions?: boolean, _lastComment?: Comment) { + _hideActions(_showActions?: boolean, _lastComment?: UIComment) { return ( this._shouldDisableAction(_showActions, _lastComment) || - !!(_lastComment as RobotComment).robot_id + isRobot(_lastComment) ); } @@ -371,7 +377,7 @@ if (this._orderedComments) { for (let i = 0; i < this._orderedComments.length; i++) { const comment = this._orderedComments[i]; - const isRobotComment = !!(comment as RobotComment).robot_id; + const isRobotComment = !!(comment as UIRobot).robot_id; // False if it's an unresolved comment under UNRESOLVED_EXPAND_COUNT. const resolvedThread = !this.unresolved || @@ -417,8 +423,8 @@ } } - _isDraft(comment: Comment) { - return !!comment.__draft; + _isDraft(comment: UIComment) { + return isDraft(comment); } _processCommentReply(quote?: boolean) { @@ -463,9 +469,9 @@ const els = this.root?.querySelectorAll('gr-comment'); if (!els) return null; for (const el of els) { - if (el.comment?.id === id || el.comment?.__draftID === id) { - return el; - } + const c = el.comment; + if (isRobot(c)) continue; + if (c?.id === id || (isDraft(c) && c?.__draftID === id)) return el; } return null; } @@ -487,7 +493,7 @@ } _newDraft(lineNum?: number, range?: CommentRange) { - const d: Comment = { + const d: UIDraft = { __draft: true, __draftID: Math.random().toString(36), __date: new Date(), @@ -529,14 +535,16 @@ return isOnParent ? CommentSide.PARENT : CommentSide.REVISION; } - _computeRootId(comments: PolymerDeepPropertyChange<Comment[], unknown>) { + _computeRootId(comments: PolymerDeepPropertyChange<UIComment[], unknown>) { // Keep the root ID even if the comment was removed, so that notification // to sync will know which thread to remove. if (!comments.base.length) { return this.rootId; } const rootComment = comments.base[0]; - return rootComment.id || rootComment.__draftID; + if (rootComment.id) return rootComment.id; + if (isDraft(rootComment)) return rootComment.__draftID; + throw new Error('Missing id in root comment.'); } _handleCommentDiscard(e: Event) { @@ -599,12 +607,12 @@ this.updateThreadProperties(); } - _indexOf(comment: Comment | undefined, arr: Comment[]) { + _indexOf(comment: UIComment | undefined, arr: UIComment[]) { if (!comment) return -1; for (let i = 0; i < arr.length; i++) { const c = arr[i]; if ( - (c.__draftID && c.__draftID === comment.__draftID) || + (isDraft(c) && isDraft(comment) && c.__draftID === comment.__draftID) || (c.id && c.id === comment.id) ) { return i;
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts index 8bd1f69..d68da94 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -38,22 +38,27 @@ import {KeyboardShortcutMixin} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin'; import {getRootElement} from '../../../scripts/rootElement'; import {appContext} from '../../../services/app-context'; -import {customElement, property, observe} from '@polymer/decorators'; +import {customElement, observe, property} from '@polymer/decorators'; import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api'; import {GrTextarea} from '../gr-textarea/gr-textarea'; import {GrStorage, StorageLocation} from '../gr-storage/gr-storage'; import {GrOverlay} from '../gr-overlay/gr-overlay'; import { - RobotCommentInfo, - PatchSetNum, - CommentInfo, - ConfigInfo, AccountDetailInfo, NumericChangeId, + ConfigInfo, + PatchSetNum, } from '../../../types/common'; import {GrButton} from '../gr-button/gr-button'; import {GrConfirmDeleteCommentDialog} from '../gr-confirm-delete-comment-dialog/gr-confirm-delete-comment-dialog'; import {GrDialog} from '../gr-dialog/gr-dialog'; +import {Side} from '../../../constants/constants'; +import { + isDraft, + UIComment, + UIDraft, + UIRobot, +} from '../../diff/gr-comment-api/gr-comment-api'; const STORAGE_DEBOUNCE_INTERVAL = 400; const TOAST_DEBOUNCE_INTERVAL = 200; @@ -84,23 +89,6 @@ 'When disagreeing, explain the advantage of your approach.', ]; -export interface Draft { - collapsed?: boolean; - __editing?: boolean; - __otherEditing?: boolean; - __draft?: boolean; - __draftID?: string; - __commentSide?: string; - __date?: Date; -} - -export type Comment = Draft & CommentInfo; -export type RobotComment = Draft & RobotCommentInfo; - -export function isRobotComment(c: Comment | RobotComment): c is RobotComment { - return (c as RobotComment).robot_id !== undefined; -} - interface CommentOverlays { confirmDelete?: GrOverlay | null; confirmDiscard?: GrOverlay | null; @@ -117,7 +105,7 @@ export interface CommentEventDetail { patchNum?: PatchSetNum; - comment?: Comment | RobotComment; + comment?: UIComment; } @customElement('gr-comment') @@ -174,10 +162,10 @@ changeNum?: NumericChangeId; @property({type: Object, notify: true, observer: '_commentChanged'}) - comment?: Comment | RobotComment; + comment?: UIComment | UIRobot; @property({type: Array}) - comments?: (Comment | RobotComment)[]; + comments?: (UIComment | UIRobot)[]; @property({type: Boolean, reflectToAttribute: true}) isRobotComment = false; @@ -235,7 +223,7 @@ _messageText = ''; @property({type: String}) - commentSide?: string; + commentSide?: Side; @property({type: String}) side?: string; @@ -311,7 +299,7 @@ } } - _getAuthor(comment: Comment) { + _getAuthor(comment: UIComment) { return comment.author || this._selfAccount; } @@ -410,7 +398,7 @@ } @observe('comment') - _isRobotComment(comment: RobotComment) { + _isRobotComment(comment: UIRobot) { this.isRobotComment = !!comment.robot_id; } @@ -433,7 +421,7 @@ return 'DRAFT' + (unableToSave ? '(Failed to save)' : ''); } - save(opt_comment?: Comment) { + save(opt_comment?: UIComment) { let comment = opt_comment; if (!comment) { comment = this.comment; @@ -456,7 +444,8 @@ this._eraseDraftComment(); return this.$.restAPI.getResponseObject(response).then(obj => { - const resComment = (obj as unknown) as Comment; + const resComment = (obj as unknown) as UIDraft; + if (!isDraft(this.comment)) throw new Error('Can only save drafts.'); resComment.__draft = true; // Maintain the ephemeral draft ID for identification by other // elements. @@ -495,7 +484,7 @@ }); } - _commentChanged(comment: Comment) { + _commentChanged(comment: UIComment) { this.editing = !!comment.__editing; this.resolved = !comment.unresolved; if (this.editing) { @@ -513,7 +502,7 @@ c => c.in_reply_to && c.in_reply_to === comment.id && - !(c as RobotComment).robot_id + !(c as UIRobot).robot_id ); } @@ -587,7 +576,7 @@ _computeSaveDisabled( draft: string, - comment: Comment | undefined, + comment: UIComment | undefined, resolved?: boolean ) { // If resolved state has changed and a msg exists, save should be enabled. @@ -753,8 +742,8 @@ ); } - _hasNoFix(comment: Comment) { - return !comment || !(comment as RobotComment).fix_suggestions; + _hasNoFix(comment: UIComment) { + return !comment || !(comment as UIRobot).fix_suggestions; } _handleDiscard(e: Event) { @@ -785,7 +774,7 @@ _discardDraft() { if (!this.comment) return Promise.reject(new Error('undefined comment')); - if (!this.comment.__draft) { + if (!isDraft(this.comment)) { return Promise.reject(new Error('Cannot discard a non-draft comment.')); } this.discarding = true; @@ -883,7 +872,7 @@ this._handleFailedDraftRequest(); } - _saveDraft(draft?: Comment) { + _saveDraft(draft?: UIComment) { if (!draft || this.changeNum === undefined || this.patchNum === undefined) { throw new Error('undefined draft or changeNum or patchNum'); } @@ -907,7 +896,7 @@ }); } - _deleteDraft(draft: Comment) { + _deleteDraft(draft: UIComment) { if (this.changeNum === undefined || this.patchNum === undefined) { throw new Error('undefined changeNum or patchNum'); } @@ -937,7 +926,7 @@ _loadLocalDraft( changeNum: number, patchNum?: PatchSetNum, - comment?: Comment + comment?: UIComment ) { // Polymer 2: check for undefined if ([changeNum, patchNum, comment].includes(undefined)) { @@ -1012,7 +1001,7 @@ return overlay.open(); } - _computeHideRunDetails(comment: RobotComment, collapsed: boolean) { + _computeHideRunDetails(comment: UIRobot, collapsed: boolean) { if (!comment) return true; return !(comment.robot_id && comment.url && !collapsed); }
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts index 331fb42..2d91d3d 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
@@ -15,19 +15,18 @@ * limitations under the License. */ import {GrAnnotationActionsContext} from './gr-annotation-actions-context'; -import {GrDiffLine, LineNumber} from '../../diff/gr-diff/gr-diff-line'; -import {CoverageRange} from '../../../types/types'; +import {GrDiffLine} from '../../diff/gr-diff/gr-diff-line'; +import { + CoverageRange, + DiffLayer, + DiffLayerListener, +} from '../../../types/types'; import {Side} from '../../../constants/constants'; import {PluginApi} from '../../plugins/gr-plugin-types'; +import {NumericChangeId} from '../../../types/common'; type AddLayerFunc = (ctx: GrAnnotationActionsContext) => void; -type LayerUpdateListener = ( - start: LineNumber, - end: LineNumber, - side: Side -) => void; - type NotifyFunc = ( path: string, start: number, @@ -36,10 +35,10 @@ ) => void; export type CoverageProvider = ( - changeNum: number, + changeNum: NumericChangeId, path: string, - basePatchNum: number, - patchNum: number + basePatchNum?: number, + patchNum?: number ) => Promise<Array<CoverageRange>>; export class GrAnnotationActionsInterface { @@ -205,8 +204,8 @@ } } -export class AnnotationLayer { - private listeners: LayerUpdateListener[] = []; +export class AnnotationLayer implements DiffLayer { + private listeners: DiffLayerListener[] = []; /** * Used to create an instance of the Annotation Layer interface. @@ -232,12 +231,12 @@ * Should accept as arguments the line numbers for the start and end of * the update and the side as a string. */ - addListener(fn: () => void) { - this.listeners.push(fn); + addListener(listener: DiffLayerListener) { + this.listeners.push(listener); } - removeListener(fn: () => void) { - this.listeners = this.listeners.filter(f => f !== fn); + removeListener(listener: DiffLayerListener) { + this.listeners = this.listeners.filter(f => f !== listener); } /** @@ -271,7 +270,7 @@ * @param end The line where the update ends. * @param side The side of the update. ('left' or 'right') */ - notifyListeners(start: LineNumber, end: LineNumber, side: Side) { + notifyListeners(start: number, end: number, side: Side) { for (const listener of this.listeners) { listener(start, end, side); }
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts index ff9446c..4fc6f9f 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -21,10 +21,7 @@ import {patchNumEquals} from '../../../utils/patch-set-util'; import {customElement} from '@polymer/decorators'; import {ChangeInfo, RevisionInfo} from '../../../types/common'; -import { - CoverageProvider, - GrAnnotationActionsInterface, -} from './gr-annotation-actions-js-api'; +import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api'; import {GrAdminApi} from '../../plugins/gr-admin-api/gr-admin-api'; import { JsApiService, @@ -33,7 +30,7 @@ ShowRevisionActionsDetail, } from './gr-js-api-types'; import {EventType, TargetElement} from '../../plugins/gr-plugin-types'; -import {HighlightJS} from '../../../types/types'; +import {DiffLayer, HighlightJS} from '../../../types/types'; const elements: {[key: string]: HTMLElement} = {}; const eventCallbacks: {[key: string]: EventCallback[]} = {}; @@ -248,7 +245,7 @@ } getDiffLayers(path: string, changeNum: number) { - const layers = []; + const layers: DiffLayer[] = []; for (const cb of this._getEventCallbacks(EventType.ANNOTATE_DIFF)) { const annotationApi = (cb as unknown) as GrAnnotationActionsInterface; try { @@ -279,7 +276,9 @@ * provider, the first one is returned. If no plugin offers a coverage provider, * will resolve to null. */ - getCoverageAnnotationApi(): Promise<CoverageProvider | undefined> { + getCoverageAnnotationApi(): Promise< + GrAnnotationActionsInterface | undefined + > { return getPluginLoader() .awaitPluginsLoaded() .then( @@ -287,7 +286,7 @@ this._getEventCallbacks(EventType.ANNOTATE_DIFF).find(cb => { const annotationApi = (cb as unknown) as GrAnnotationActionsInterface; return annotationApi.getCoverageProvider(); - }) as CoverageProvider | undefined + }) as GrAnnotationActionsInterface | undefined ); }
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts index b09fe88..505e62e 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
@@ -16,6 +16,8 @@ */ import {ActionInfo, ChangeInfo, PatchSetNum} from '../../../types/common'; import {EventType, TargetElement} from '../../plugins/gr-plugin-types'; +import {DiffLayer} from '../../../types/types'; +import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api'; export interface ShowChangeDetail { change: ChangeInfo; @@ -45,5 +47,8 @@ origMsg: string ): string; addElement(key: TargetElement, el: HTMLElement): void; + getDiffLayers(path: string, changeNum: number): DiffLayer[]; + disposeDiffLayers(path: string): void; + getCoverageAnnotationApi(): Promise<GrAnnotationActionsInterface | undefined>; // TODO(TS): Add more methods when needed for the TS conversion. }
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts index 06ae026..5b9f360 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
@@ -133,6 +133,7 @@ FixId, FilePathToDiffInfoMap, ChangeViewChangeInfo, + BlameInfo, } from '../../../types/common'; import { CancelConditionCallback, @@ -2524,7 +2525,7 @@ req.fetchOptions.headers.append('Cache-Control', 'no-cache'); } - return this._getChangeURLAndFetch(req); + return this._getChangeURLAndFetch(req) as Promise<DiffInfo | undefined>; } getDiffComments( @@ -2639,11 +2640,9 @@ _setRanges(comments?: CommentInfo[]) { comments = comments || []; - comments.sort((a, b) => { - if (!a.updated) return 1; - if (!b.updated) return -1; - return parseDate(a.updated).valueOf() - parseDate(b.updated).valueOf(); - }); + comments.sort( + (a, b) => parseDate(a.updated).valueOf() - parseDate(b.updated).valueOf() + ); for (const comment of comments) { this._setRange(comments, comment); } @@ -3472,7 +3471,7 @@ patchNum, params: base ? {base: 't'} : undefined, anonymizedEndpoint: '/files/*/blame', - }); + }) as Promise<BlameInfo[] | undefined>; } /**