Merge changes I6b58d8ba,Ifa3853aa * changes: Extract helpers for isOnParent and patchNum Move comment threading to builder
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js index 60bf5ca..4b85c22 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -353,6 +353,92 @@ }; /** + * @param {Array<Object>} comments + * @param {string} patchForNewThreads + */ + GrDiffBuilder.prototype._getThreads = function(comments, patchForNewThreads) { + const sortedComments = comments.slice(0).sort((a, b) => { + if (b.__draft && !a.__draft ) { return 0; } + if (a.__draft && !b.__draft ) { return 1; } + return util.parseDate(a.updated) - util.parseDate(b.updated); + }); + + const threads = []; + for (const comment of sortedComments) { + // If the comment is in reply to another comment, find that comment's + // 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)); + if (thread) { + thread.comments.push(comment); + continue; + } + } + + // Otherwise, this comment starts its own thread. + const newThread = { + start_datetime: comment.updated, + comments: [comment], + commentSide: comment.__commentSide, + /** + * Determines what the patchNum of a thread should be. Use patchNum from + * comment if it exists, otherwise the property of the thread group. + * This is needed for switching between side-by-side and unified views + * when there are unsaved drafts. + */ + patchNum: comment.patch_set || patchForNewThreads, + rootId: comment.id || comment.__draftID, + }; + if (comment.range) { + newThread.range = Object.assign({}, comment.range); + } + threads.push(newThread); + } + return threads; + }; + + /** + * Returns the patch number that new comment threads should be attached to. + * + * @param {GrDiffLine} line The line new thread will be attached to. + * @param {string=} opt_side Set to LEFT to force adding it to the LEFT side - + * will be ignored if the left is a parent or a merge parent + * @return {number} Patch set to attach the new thread to + */ + GrDiffBuilder.prototype._determinePatchNumForNewThreads = function( + patchRange, line, opt_side) { + if ((line.type === GrDiffLine.Type.REMOVE || + opt_side === GrDiffBuilder.Side.LEFT) && + patchRange.basePatchNum !== 'PARENT' && + !Gerrit.PatchSetBehavior.isMergeParent(patchRange.basePatchNum)) { + return patchRange.basePatchNum; + } else { + return patchRange.patchNum; + } + }; + + /** + * Returns whether the comments on the given line are on a (merge) parent. + * + * @param {string} firstCommentSide + * @param {{basePatchNum: number, patchNum: number}} patchRange + * @param {GrDiffLine} line The line the comments are on. + * @param {string=} opt_side + * @return {boolean} True iff the comments on the given line are on a (merge) + * parent. + */ + GrDiffBuilder.prototype._determineIsOnParent = function( + firstCommentSide, patchRange, line, opt_side) { + return ((line.type === GrDiffLine.Type.REMOVE || + opt_side === GrDiffBuilder.Side.LEFT) && + (patchRange.basePatchNum === 'PARENT' || + Gerrit.PatchSetBehavior.isMergeParent( + patchRange.basePatchNum))) || + firstCommentSide === 'PARENT'; + }; + + /** * @param {GrDiffLine} line * @param {string=} opt_side * @return {!Object} @@ -360,26 +446,19 @@ GrDiffBuilder.prototype._commentThreadGroupForLine = function( line, opt_side) { const comments = - this._getCommentsForLine(this._comments, line, opt_side); + this._getCommentsForLine(this._comments, line, opt_side); if (!comments || comments.length === 0) { return null; } - let patchNum = this._comments.meta.patchRange.patchNum; - let isOnParent = comments[0].side === 'PARENT' || false; - if (line.type === GrDiffLine.Type.REMOVE || - opt_side === GrDiffBuilder.Side.LEFT) { - if (this._comments.meta.patchRange.basePatchNum === 'PARENT' || - Gerrit.PatchSetBehavior.isMergeParent( - this._comments.meta.patchRange.basePatchNum)) { - isOnParent = true; - } else { - patchNum = this._comments.meta.patchRange.basePatchNum; - } - } + const patchNum = this._determinePatchNumForNewThreads( + this._comments.meta.patchRange, line, opt_side); + const isOnParent = this._determineIsOnParent( + comments[0].side, this._comments.meta.patchRange, line, opt_side); + const threadGroupEl = this._createThreadGroupFn(patchNum, isOnParent, opt_side); - threadGroupEl.comments = comments; + threadGroupEl.threads = this._getThreads(comments, patchNum); if (opt_side) { threadGroupEl.setAttribute('data-side', opt_side); }
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html index 4d7b821..3238cbc 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -84,6 +84,166 @@ teardown(() => { sandbox.restore(); }); + test('_getThreads', () => { + const patchForNewThreads = 3; + const comments = [ + { + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + in_reply_to: 'sallys_confession', + }, + { + id: 'new_draft', + message: 'i do not like either of you', + __commentSide: 'left', + __draft: true, + updated: '2015-12-20 15:01:20.396000000', + }, + ]; + + let expectedThreadGroups = [ + { + start_datetime: '2015-12-23 15:00:20.396000000', + commentSide: 'left', + comments: [{ + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + in_reply_to: 'sallys_confession', + }], + rootId: 'sallys_confession', + patchNum: 3, + }, + { + start_datetime: '2015-12-20 15:01:20.396000000', + commentSide: 'left', + comments: [ + { + id: 'new_draft', + message: 'i do not like either of you', + __commentSide: 'left', + __draft: true, + updated: '2015-12-20 15:01:20.396000000', + }, + ], + rootId: 'new_draft', + patchNum: 3, + }, + ]; + + assert.deepEqual( + builder._getThreads(comments, patchForNewThreads), + expectedThreadGroups); + + // Patch num should get inherited from comment rather + comments.push({ + id: 'betsys_confession', + message: 'i like you, jack', + updated: '2015-12-24 15:00:10.396000000', + range: { + start_line: 1, + start_character: 1, + end_line: 1, + end_character: 2, + }, + __commentSide: 'left', + }); + + expectedThreadGroups = [ + { + start_datetime: '2015-12-23 15:00:20.396000000', + commentSide: 'left', + comments: [{ + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + __commentSide: 'left', + }, { + id: 'jacks_reply', + in_reply_to: 'sallys_confession', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + }], + patchNum: 3, + rootId: 'sallys_confession', + }, + { + start_datetime: '2015-12-24 15:00:10.396000000', + commentSide: 'left', + comments: [{ + id: 'betsys_confession', + message: 'i like you, jack', + updated: '2015-12-24 15:00:10.396000000', + range: { + start_line: 1, + start_character: 1, + end_line: 1, + end_character: 2, + }, + __commentSide: 'left', + }], + patchNum: 3, + rootId: 'betsys_confession', + range: { + start_line: 1, + start_character: 1, + end_line: 1, + end_character: 2, + }, + }, + { + start_datetime: '2015-12-20 15:01:20.396000000', + commentSide: 'left', + comments: [ + { + id: 'new_draft', + message: 'i do not like either of you', + __commentSide: 'left', + __draft: true, + updated: '2015-12-20 15:01:20.396000000', + }, + ], + rootId: 'new_draft', + patchNum: 3, + }, + ]; + + assert.deepEqual( + builder._getThreads(comments, patchForNewThreads), + expectedThreadGroups); + }); + + test('multiple comments at same location but not threaded', () => { + const comments = [ + { + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + }, + ]; + assert.equal(builder._getThreads(comments, '3').length, 2); + }); + test('_createElement classStr applies all classes', () => { const node = builder._createElement('div', 'test classes'); assert.isTrue(node.classList.contains('gr-diff')); @@ -312,11 +472,23 @@ right: [r5], }; + function threadForComment(c, patchNum) { + return { + commentSide: c.__commentSide, + comments: [c], + patchNum, + rootId: c.id, + start_datetime: c.updated, + }; + } + function checkThreadGroupProps(threadGroupEl, patchNum, isOnParent, comments) { assert.equal(createThreadGroupFn.lastCall.args[0], patchNum); assert.equal(createThreadGroupFn.lastCall.args[1], isOnParent); - assert.deepEqual(threadGroupEl.comments, comments); + assert.deepEqual( + threadGroupEl.threads, + comments.map(c => threadForComment(c, patchNum))); } let line = new GrDiffLine(GrDiffLine.Type.BOTH);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html index fb801e5..58b7c32 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
@@ -31,7 +31,7 @@ margin-top: .2em; } </style> - <template is="dom-repeat" items="[[_threads]]" as="thread"> + <template is="dom-repeat" items="[[threads]]" as="thread"> <gr-diff-comment-thread comments="[[thread.comments]]" comment-side="[[thread.commentSide]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js index b053330..ae45c93 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
@@ -22,10 +22,6 @@ properties: { changeNum: String, - comments: { - type: Array, - value() { return []; }, - }, projectName: String, patchForNewThreads: String, range: Object, @@ -37,16 +33,12 @@ type: Number, value: null, }, - _threads: { + threads: { type: Array, value() { return []; }, }, }, - observers: [ - '_commentsChanged(comments.*)', - ], - get threadEls() { return Polymer.dom(this.root).querySelectorAll('gr-diff-comment-thread'); }, @@ -58,7 +50,7 @@ * @param {!Object} opt_range */ addNewThread(commentSide, opt_range) { - this.push('_threads', { + this.push('threads', { comments: [], commentSide, patchNum: this.patchForNewThreads, @@ -67,9 +59,9 @@ }, removeThread(rootId) { - for (let i = 0; i < this._threads.length; i++) { - if (this._threads[i].rootId === rootId) { - this.splice('_threads', i, 1); + for (let i = 0; i < this.threads.length; i++) { + if (this.threads[i].rootId === rootId) { + this.splice('threads', i, 1); return; } } @@ -114,10 +106,6 @@ a.endChar === b.endChar; }, - _commentsChanged() { - this._threads = this._getThreads(this.comments); - }, - _sortByDate(threadGroups) { if (!threadGroups.length) { return; } return threadGroups.sort((a, b) => { @@ -141,51 +129,5 @@ range.end_character + '-' + comment.__commentSide; }, - - /** - * Determines what the patchNum of a thread should be. Use patchNum from - * comment if it exists, otherwise the property of the thread group. - * This is needed for switching between side-by-side and unified views when - * there are unsaved drafts. - */ - _getPatchNum(comment) { - return comment.patch_set || this.patchForNewThreads; - }, - - _getThreads(comments) { - const sortedComments = comments.slice(0).sort((a, b) => { - if (b.__draft && !a.__draft ) { return 0; } - if (a.__draft && !b.__draft ) { return 1; } - return util.parseDate(a.updated) - util.parseDate(b.updated); - }); - - const threads = []; - for (const comment of sortedComments) { - // If the comment is in reply to another comment, find that comment's - // 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)); - if (thread) { - thread.comments.push(comment); - continue; - } - } - - // Otherwise, this comment starts its own thread. - const newThread = { - start_datetime: comment.updated, - comments: [comment], - commentSide: comment.__commentSide, - patchNum: this._getPatchNum(comment), - rootId: comment.id || comment.__draftID, - }; - if (comment.range) { - newThread.range = Object.assign({}, comment.range); - } - threads.push(newThread); - } - return threads; - }, }); })();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html index b0ee743..1fb8136 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
@@ -51,145 +51,6 @@ sandbox.restore(); }); - test('_getThreads', () => { - element.patchForNewThreads = 3; - const comments = [ - { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - in_reply_to: 'sallys_confession', - }, - { - id: 'new_draft', - message: 'i do not like either of you', - __commentSide: 'left', - __draft: true, - updated: '2015-12-20 15:01:20.396000000', - }, - ]; - - let expectedThreadGroups = [ - { - start_datetime: '2015-12-23 15:00:20.396000000', - commentSide: 'left', - comments: [{ - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - in_reply_to: 'sallys_confession', - }], - rootId: 'sallys_confession', - patchNum: 3, - }, - { - start_datetime: '2015-12-20 15:01:20.396000000', - commentSide: 'left', - comments: [ - { - id: 'new_draft', - message: 'i do not like either of you', - __commentSide: 'left', - __draft: true, - updated: '2015-12-20 15:01:20.396000000', - }, - ], - rootId: 'new_draft', - patchNum: 3, - }, - ]; - - assert.deepEqual(element._getThreads(comments), expectedThreadGroups); - - // Patch num should get inherited from comment rather - comments.push({ - id: 'betsys_confession', - message: 'i like you, jack', - updated: '2015-12-24 15:00:10.396000000', - range: { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }, - __commentSide: 'left', - }); - - expectedThreadGroups = [ - { - start_datetime: '2015-12-23 15:00:20.396000000', - commentSide: 'left', - comments: [{ - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', - }, { - id: 'jacks_reply', - in_reply_to: 'sallys_confession', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - }], - patchNum: 3, - rootId: 'sallys_confession', - }, - { - start_datetime: '2015-12-24 15:00:10.396000000', - commentSide: 'left', - comments: [{ - id: 'betsys_confession', - message: 'i like you, jack', - updated: '2015-12-24 15:00:10.396000000', - range: { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }, - __commentSide: 'left', - }], - patchNum: 3, - rootId: 'betsys_confession', - range: { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }, - }, - { - start_datetime: '2015-12-20 15:01:20.396000000', - commentSide: 'left', - comments: [ - { - id: 'new_draft', - message: 'i do not like either of you', - __commentSide: 'left', - __draft: true, - updated: '2015-12-20 15:01:20.396000000', - }, - ], - rootId: 'new_draft', - patchNum: 3, - }, - ]; - - assert.deepEqual(element._getThreads(comments), expectedThreadGroups); - }); - test('getThread', () => { const range = { start_line: 1, @@ -197,37 +58,57 @@ end_line: 1, end_character: 2, }; - element.comments = [ + element.threads = [ { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', + rootId: 'sallys_confession', + commentSide: 'left', + comments: [ + { + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + in_reply_to: 'sallys_confession', + }, { + id: 'new_draft', + message: 'i do not like either of you', + __commentSide: 'left', + __draft: true, + in_reply_to: 'sallys_confession', + updated: '2015-12-20 15:01:20.396000000', + }, + ], + }, + { + rootId: 'right_side_comment', + commentSide: 'right', + comments: [ + { + id: 'right_side_comment', + message: 'right side comment', + __commentSide: 'right', + __draft: true, + updated: '2015-12-20 15:01:20.396000000', + }, + ], }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - in_reply_to: 'sallys_confession', - }, { - id: 'new_draft', - message: 'i do not like either of you', - __commentSide: 'left', - __draft: true, - in_reply_to: 'sallys_confession', - updated: '2015-12-20 15:01:20.396000000', - }, { - id: 'right_side_comment', - message: 'right side comment', - __commentSide: 'right', - __draft: true, - updated: '2015-12-20 15:01:20.396000000', - }, { - id: 'betsys_confession', - message: 'i like you more, jack', - updated: '2015-12-24 15:00:10.396000000', + rootId: 'betsys_confession', + commentSide: 'left', range, - __commentSide: 'left', + comments: [ + { + id: 'betsys_confession', + message: 'i like you more, jack', + updated: '2015-12-24 15:00:10.396000000', + range, + __commentSide: 'left', + }, + ], }, ]; @@ -241,24 +122,6 @@ assert.deepEqual(element.getThread('left', range).comments.length, 1); }); - test('multiple comments at same location but not threaded', () => { - element.patchForNewThreads = 3; - const comments = [ - { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - }, - ]; - assert.equal(element._getThreads(comments).length, 2); - }); - test('_sortByDate', () => { let threadGroups = [ { @@ -329,17 +192,6 @@ 'range-1-2-3-4-left'); }); - test('thread groups are updated when comments change', () => { - const commentsChangedStub = sandbox.stub(element, '_commentsChanged'); - element.comments = []; - element.comments.push({ - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - }); - assert(commentsChangedStub.called); - }); - test('addNewThread', () => { const locationRange = 'range-1-2-3-4'; element._threads = [{locationRange: 'line'}]; @@ -347,18 +199,6 @@ assert(element._threads.length, 2); }); - test('_getPatchNum', () => { - element.patchForNewThreads = 3; - const comment = { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - }; - assert.equal(element._getPatchNum(comment), 3); - comment.patch_set = 4; - assert.equal(element._getPatchNum(comment), 4); - }); - test('removeThread', () => { const locationRange = 'range-1-2-3-4'; element._threads = [