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 5045b6c08e..522ee526c3 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 @@ -349,20 +349,33 @@ return result; }; + /** + * @param {number} changeNum + * @param {number|string} patchNum + * @param {string} path + * @param {boolean} isOnParent + * @param {string} commentSide + * @return {!Object} + */ GrDiffBuilder.prototype.createCommentThreadGroup = function(changeNum, - patchNum, path, isOnParent, range) { + patchNum, path, isOnParent, commentSide) { const threadGroupEl = document.createElement('gr-diff-comment-thread-group'); threadGroupEl.changeNum = changeNum; + threadGroupEl.commentSide = commentSide; threadGroupEl.patchForNewThreads = patchNum; threadGroupEl.path = path; threadGroupEl.isOnParent = isOnParent; threadGroupEl.projectName = this._projectName; - threadGroupEl.range = range; threadGroupEl.parentIndex = this._parentIndex; return threadGroupEl; }; + /** + * @param {number} line + * @param {string=} opt_side + * @return {!Object} + */ GrDiffBuilder.prototype._commentThreadGroupForLine = function( line, opt_side) { const comments = @@ -385,7 +398,7 @@ } const threadGroupEl = this.createCommentThreadGroup( this._comments.meta.changeNum, patchNum, this._comments.meta.path, - isOnParent); + isOnParent, opt_side); threadGroupEl.comments = comments; if (opt_side) { threadGroupEl.setAttribute('data-side', opt_side); 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 bf94bb642b..407cf933e3 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 @@ -32,14 +32,15 @@ limitations under the License. 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 6978d052a6..2f6dd9ea49 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 @@ -19,6 +19,7 @@ properties: { changeNum: String, + commentSide: String, comments: { type: Array, value() { return []; }, @@ -44,28 +45,41 @@ '_commentsChanged(comments.*)', ], - addNewThread(locationRange) { + /** + * Adds a new thread. Range is optional because a comment can be + * added to a line without a range selected. + * + * @param {!Object} opt_range + */ + addNewThread(opt_range) { this.push('_threads', { comments: [], - locationRange, patchNum: this.patchForNewThreads, + range: opt_range, }); }, - removeThread(locationRange) { + removeThread(rootId) { for (let i = 0; i < this._threads.length; i++) { - if (this._threads[i].locationRange === locationRange) { + if (this._threads[i].rootId === rootId) { this.splice('_threads', i, 1); return; } } }, - getThreadForRange(rangeToCheck) { + /** + * Fetch the thread group at the given range, or the range-less thread + * on the line if no range is provided. + * + * @param {!Object=} opt_range + * @return {!Object|undefined} + */ + getThread(opt_range) { const threads = [].filter.call( Polymer.dom(this.root).querySelectorAll('gr-diff-comment-thread'), thread => { - return thread.locationRange === rangeToCheck; + return thread.range === opt_range; }); if (threads.length === 1) { return threads[0]; @@ -116,13 +130,6 @@ const threads = []; for (const comment of sortedComments) { - let locationRange; - if (!comment.range) { - locationRange = 'line-' + comment.__commentSide; - } else { - locationRange = this._calculateLocationRange(comment.range, comment); - } - // If the comment is in reply to another comment, find that comment's // thread and append to it. if (comment.in_reply_to) { @@ -138,9 +145,9 @@ threads.push({ start_datetime: comment.updated, comments: [comment], - locationRange, commentSide: comment.__commentSide, patchNum: this._getPatchNum(comment), + rootId: comment.id, }); } 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 713a162c73..13596c021e 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 @@ -83,7 +83,7 @@ limitations under the License. __commentSide: 'left', in_reply_to: 'sallys_confession', }], - locationRange: 'line-left', + rootId: 'sallys_confession', patchNum: 3, }, ]; @@ -122,7 +122,7 @@ limitations under the License. __commentSide: 'left', }], patchNum: 3, - locationRange: 'line-left', + rootId: 'sallys_confession', }, { start_datetime: '2015-12-24 15:00:10.396000000', @@ -140,7 +140,7 @@ limitations under the License. __commentSide: 'left', }], patchNum: 3, - locationRange: 'range-1-1-1-2-left', + rootId: 'betsys_confession', }, ]; diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html index 6a12118508..f300992de0 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html @@ -65,6 +65,7 @@ limitations under the License. show-actions="[[_showActions]]" comment-side="[[comment.__commentSide]]" side="[[comment.side]]" + root-id="[[rootId]]" project-config="[[_projectConfig]]" on-create-fix-comment="_handleCommentFix" on-comment-discard="_handleCommentDiscard"> diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js index d019c625bc..d4be9d436e 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js @@ -57,6 +57,7 @@ type: Number, value: null, }, + rootId: String, unresolved: { type: Boolean, notify: true, diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index a531436256..01f8f5f129 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -389,13 +389,21 @@ const patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl); const isOnParent = this._getIsParentCommentByLineAndContent(lineEl, contentEl); - const threadEl = this._getOrCreateThreadAtLineRange(contentEl, patchNum, + const threadEl = this._getOrCreateThread(contentEl, patchNum, side, isOnParent, opt_range); threadEl.addOrEditDraft(opt_lineNum, opt_range); }, - _getThreadForRange(threadGroupEl, rangeToCheck) { - return threadGroupEl.getThreadForRange(rangeToCheck); + /** + * Fetch the thread group at the given range, or the range-less thread + * on the line if no range is provided. + * + * @param {!Object} threadGroupEl + * @param {!Object=} opt_range + * @return {!Object} + */ + _getThread(threadGroupEl, opt_range) { + return threadGroupEl.getThread(opt_range); }, _getThreadGroupForLine(contentEl) { @@ -417,31 +425,32 @@ }, /** + * Gets or creates a comment thread for a specific spot on a diff. + * May include a range, if the comment is a range comment. + * * @param {!Object} contentEl * @param {number} patchNum * @param {string} commentSide * @param {boolean} isOnParent * @param {!Object=} opt_range + * @return {!Object} */ - _getOrCreateThreadAtLineRange(contentEl, patchNum, commentSide, + _getOrCreateThread(contentEl, patchNum, commentSide, isOnParent, opt_range) { - const rangeToCheck = this._getRangeString(commentSide, opt_range); - // Check if thread group exists. let threadGroupEl = this._getThreadGroupForLine(contentEl); if (!threadGroupEl) { threadGroupEl = this.$.diffBuilder.createCommentThreadGroup( - this.changeNum, patchNum, this.path, isOnParent); + this.changeNum, patchNum, this.path, isOnParent, commentSide); contentEl.appendChild(threadGroupEl); } - let threadEl = this._getThreadForRange(threadGroupEl, rangeToCheck); + let threadEl = this._getThread(threadGroupEl, opt_range); if (!threadEl) { - threadGroupEl.addNewThread(rangeToCheck, commentSide); + threadGroupEl.addNewThread(opt_range); Polymer.dom.flush(); - threadEl = this._getThreadForRange(threadGroupEl, rangeToCheck); - threadEl.commentSide = commentSide; + threadEl = this._getThread(threadGroupEl, opt_range); } return threadEl; }, @@ -495,7 +504,7 @@ _handleThreadDiscard(e) { const el = Polymer.dom(e).rootTarget; - el.parentNode.removeThread(el.locationRange); + el.parentNode.removeThread(el.rootId); }, _handleCommentDiscard(e) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html index d235180572..2d699f2739 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html @@ -358,18 +358,15 @@ limitations under the License. element.patchRange = {basePatchNum: 1, patchNum: 2}; element.path = 'file.txt'; - sandbox.stub(element.$.diffBuilder, 'createCommentThreadGroup', () => { - const threadGroup = - document.createElement('gr-diff-comment-thread-group'); - threadGroup.patchForNewThreads = 1; - return threadGroup; - }); + const mock = document.createElement('mock-diff-response'); + element.$.diffBuilder._builder = element.$.diffBuilder._getDiffBuilder( + mock.diffResponse, {}, {tab_size: 2, line_length: 80}); // No thread groups. assert.isNotOk(element._getThreadGroupForLine(contentEl)); // A thread group gets created. - assert.isOk(element._getOrCreateThreadAtLineRange(contentEl, + assert.isOk(element._getOrCreateThread(contentEl, patchNum, commentSide, side)); // Try to fetch a thread with a different range. @@ -380,7 +377,7 @@ limitations under the License. endChar: 3, }; - assert.isOk(element._getOrCreateThreadAtLineRange( + assert.isOk(element._getOrCreateThread( contentEl, patchNum, commentSide, side, range)); // The new thread group can be fetched. assert.isOk(element._getThreadGroupForLine(contentEl));