Move comment threading to builder

We want to decouple gr-diff from the comment thread element, but we need to keep the comment thread group element because the grouping depends on the viewMode and so on. This means combining comments into threads should not be the responsibility of the comment thread group element, because other comment thread implementations use models that already come threaded. This is only the very first step: * creation of the gr-diff-comment-thread elements also needs to move out of gr-diff-comment-thread-group. * gr-diff-builder.js is only a temporary home - eventually both threading and rendering will move all the way out to gr-diff-host Change-Id: Ifa3853aab619776609d42bc0d1422f4bdf598e3d
2018-09-20 17:18:53 +02:00
parent 0a7075fea6
commit 45365b9bc2
5 changed files with 274 additions and 274 deletions
--- 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
@@ -352,6 +352,52 @@
    return result;
  };

+  /**
+   * @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;
+  };
+
  /**
   * @param {GrDiffLine} line
   * @param {string=} opt_side
@@ -379,7 +425,7 @@
    }
    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);
    }
--- 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 @@ limitations under the License.

    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 @@ limitations under the License.
        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);
--- 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 @@ limitations under the License.
        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]]"
--- 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;
-    },
  });
 })();
--- 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 @@ limitations under the License.
      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 @@ limitations under the License.
        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 @@ limitations under the License.
      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 @@ limitations under the License.
          '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 @@ limitations under the License.
      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 = [