Merge "Do not sort threads again for modifications on thread"

polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js

@@ -96,56 +96,79 @@ class GrThreadList extends GestureEventListeners(
    * @param {!Object} changeRecord
    */
   _computeSortedThreads(changeRecord) {
-    const threads = changeRecord.base;
-    if (!threads) { return []; }
-    this._updateSortedThreads(threads);
+    const baseThreads = changeRecord.base;
+    const threads = changeRecord.value;
+    if (!baseThreads) { return []; }
+    // TODO: should change how data flows to solve the root cause
+    // We only want to sort on thread additions / removals to avoid
+    // re-rendering on modifications (add new reply / edit draft etc)
+    //  https://polymer-library.polymer-project.org/3.0/docs/devguide/observers#array-observation
+    let shouldSort = true;
+    if (threads.indexSplices) {
+      // Array splice mutations
+      shouldSort = threads.indexSplices.addedCount !== 0
+        || threads.indexSplices.removed.length;
+    } else {
+      // A replace mutation
+      shouldSort = threads.length !== baseThreads.length;
+    }
+    this._updateSortedThreads(baseThreads, shouldSort);
   }
 
-  // TODO(taoalpha): should allow only sort once during initialization
-  // to avoid flickering
-  _updateSortedThreads(threads) {
-    this._sortedThreads =
-        threads.map(this._getThreadWithSortInfo).sort((c1, c2) => {
-          // threads will be sorted by:
-          // - unresolved first
-          // - with drafts
-          // - file path
-          // - line
-          // - updated time
-          if (c2.unresolved || c1.unresolved) {
-            if (!c1.unresolved) { return 1; }
-            if (!c2.unresolved) { return -1; }
-          }
+  _updateSortedThreads(threads, shouldSort) {
+    const threadsWithInfo = threads.map(this._getThreadWithSortInfo);
+    if (this._sortedThreads && !shouldSort) {
+      // Instead of replacing the _sortedThreads which will trigger a re-render,
+      // we override all threads inside of it
+      for (const thread of threadsWithInfo) {
+        const idxInSortedThreads = this._sortedThreads
+            .findIndex(t => t.thread.rootId === thread.thread.rootId);
+        this._sortedThreads[idxInSortedThreads] = thread;
+      }
+      return;
+    }
 
-          if (c2.hasDraft || c1.hasDraft) {
-            if (!c1.hasDraft) { return 1; }
-            if (!c2.hasDraft) { return -1; }
-          }
+    this._sortedThreads = threadsWithInfo.sort((c1, c2) => {
+      // threads will be sorted by:
+      // - unresolved first
+      // - with drafts
+      // - file path
+      // - line
+      // - updated time
+      if (c2.unresolved || c1.unresolved) {
+        if (!c1.unresolved) { return 1; }
+        if (!c2.unresolved) { return -1; }
+      }
 
-          // TODO: Update here once we introduce patchset level comments
-          // they may not have or have a special line or path attribute
+      if (c2.hasDraft || c1.hasDraft) {
+        if (!c1.hasDraft) { return 1; }
+        if (!c2.hasDraft) { return -1; }
+      }
 
-          if (c1.thread.path !== c2.thread.path) {
-            return c1.thread.path.localeCompare(c2.thread.path);
-          }
+      // TODO: Update here once we introduce patchset level comments
+      // they may not have or have a special line or path attribute
 
-          // File level comments (no `line` property)
-          // should always show before any lines
-          if ([c1, c2].some(c => c.thread.line === undefined)) {
-            if (!c1.thread.line) { return -1; }
-            if (!c2.thread.line) { return 1; }
-          } else if (c1.thread.line !== c2.thread.line) {
-            return c1.thread.line - c2.thread.line;
-          }
+      if (c1.thread.path !== c2.thread.path) {
+        return c1.thread.path.localeCompare(c2.thread.path);
+      }
 
-          const c1Date = c1.__date || util.parseDate(c1.updated);
-          const c2Date = c2.__date || util.parseDate(c2.updated);
-          const dateCompare = c2Date - c1Date;
-          if (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) {
-            return 0;
-          }
-          return dateCompare ? dateCompare : c1.id.localeCompare(c2.id);
-        });
+      // File level comments (no `line` property)
+      // should always show before any lines
+      if ([c1, c2].some(c => c.thread.line === undefined)) {
+        if (!c1.thread.line) { return -1; }
+        if (!c2.thread.line) { return 1; }
+      } else if (c1.thread.line !== c2.thread.line) {
+        return c1.thread.line - c2.thread.line;
+      }
+
+      const c1Date = c1.__date || util.parseDate(c1.updated);
+      const c2Date = c2.__date || util.parseDate(c2.updated);
+      const dateCompare = c2Date - c1Date;
+      if (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) {
+        return 0;
+      }
+      return dateCompare ? dateCompare : c1.id.localeCompare(c2.id);
+    });
   }
 
   _computeFilteredThreads(sortedThreads, unresolvedOnly, draftsOnly,

polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html

@@ -293,7 +293,7 @@ suite('gr-thread-list tests', () => {
     assert.equal(element._filteredThreads.includes(thread), true);
   });
 
-  test('thread removal', () => {
+  test('thread removal and sort again', () => {
     threadElements[1].dispatchEvent(
         new CustomEvent('thread-discard', {
           detail: {rootId: 'rc2'},
@@ -321,6 +321,43 @@ suite('gr-thread-list tests', () => {
         '09a9fb0a_1484e6cf');
   });
 
+  test('modification on thread shold not trigger sort again', () => {
+    const modifiedThreads = [...element.threads];
+    modifiedThreads[5].comments = [...modifiedThreads[5].comments];
+    modifiedThreads[5].comments.push({
+      ...modifiedThreads[5].comments[0],
+      unresolved: false,
+    });
+    const currentSortedThreads = [...element._sortedThreads];
+    element.threads = modifiedThreads;
+    assert.notDeepEqual(currentSortedThreads, element._sortedThreads);
+
+    // exact same order as in _computeSortedThreads
+    assert.equal(element._sortedThreads.length, 7);
+    // Draft and unresolved for commit-msg at line 5
+    assert.equal(element._sortedThreads[0].thread.rootId,
+        'ecf0b9fa_fe1a5f62');
+    // /COMMIT_MSG
+    // unresolved no draft and file level
+    assert.equal(element._sortedThreads[1].thread.rootId,
+        '8caddf38_44770ec1');
+    // unresolved no draft at line 4
+    assert.equal(element._sortedThreads[2].thread.rootId,
+        'scaddf38_44770ec1');
+    // unresolved no draft at line 5
+    assert.equal(element._sortedThreads[3].thread.rootId,
+        'rc1');
+    // Unresolved no draft at line 7
+    assert.equal(element._sortedThreads[4].thread.rootId,
+        'rc2');
+    // resolved and draft on COMMIT_MSG
+    assert.equal(element._sortedThreads[5].thread.rootId,
+        'zcf0b9fa_fe1a5f62');
+    // resolved and on file test.txt
+    assert.equal(element._sortedThreads[6].thread.rootId,
+        '09a9fb0a_1484e6cf');
+  });
+
   test('toggle unresolved only shows unresolved comments', () => {
     MockInteractions.tap(element.shadowRoot.querySelector(
         '#unresolvedToggle'));