Merge "Update comment thread creation/removal to no longer use locationRange"

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

polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html

@@ -32,14 +32,15 @@ limitations under the License.
     <template is="dom-repeat" items="[[_threads]]" as="thread">
       <gr-diff-comment-thread
           comments="[[thread.comments]]"
-          comment-side="[[thread.commentSide]]"
+          comment-side="[[commentSide]]"
           is-on-parent="[[isOnParent]]"
           parent-index="[[parentIndex]]"
           change-num="[[changeNum]]"
-          location-range="[[thread.locationRange]]"
           patch-num="[[thread.patchNum]]"
+          root-id="[[thread.rootId]]"
           path="[[path]]"
-          project-name="[[projectName]]"></gr-diff-comment-thread>
+          project-name="[[projectName]]"
+          range="[[thread.range]]"></gr-diff-comment-thread>
     </template>
   </template>
   <script src="gr-diff-comment-thread-group.js"></script>

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;

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',
         },
       ];
 

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"></gr-diff-comment>

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,

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 = this._getThread(threadGroupEl, opt_range);
-        threadEl.commentSide = commentSide;
       }
       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) {

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 mock = document.createElement('mock-diff-response');
-          const threadGroup =
+        element.$.diffBuilder._builder = element.$.diffBuilder._getDiffBuilder(
-          document.createElement('gr-diff-comment-thread-group');
+            mock.diffResponse, {}, {tab_size: 2, line_length: 80});
-          threadGroup.patchForNewThreads = 1;
-          return threadGroup;
-        });
 
         // 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));