From a0d86ae964a788583468c257752f074cecd7c075 Mon Sep 17 00:00:00 2001 From: Tao Zhou <taoalpha@google.com> Date: Mon, 30 Mar 2020 09:35:34 +0200 Subject: [PATCH] Update sorting for threads Threads are sorted by: 1. unresolved & has drafts should show up first 2. then sort by file path 3. then file level comments first 4. then sort by line 5. then sort by updated time 6. then sort by id This should avoid most of the re-arrangements from previous sort by date solution. The re-arrangement will still happen if you change a comment from unresolved to resolved or vice versa Change-Id: I2e09211d5ab3c861c2346668d6c2b64118301972 --- .../change/gr-thread-list/gr-thread-list.js | 23 ++++++++++ .../gr-thread-list/gr-thread-list_test.html | 46 +++++++++---------- 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js index 850bfb4273..e89be0f58f 100644 --- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js +++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js @@ -116,6 +116,12 @@ class GrThreadList extends GestureEventListeners( _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 const c1Date = c1.__date || util.parseDate(c1.updated); const c2Date = c2.__date || util.parseDate(c2.updated); const dateCompare = c2Date - c1Date; @@ -123,11 +129,28 @@ class GrThreadList extends GestureEventListeners( if (!c1.unresolved) { return 1; } if (!c2.unresolved) { return -1; } } + if (c2.hasDraft || c1.hasDraft) { if (!c1.hasDraft) { return 1; } if (!c2.hasDraft) { return -1; } } + // TODO: Update here once we introduce patchset level comments + // they may not have or have a special line or path attribute + + if (c1.thread.path !== c2.thread.path) { + return c1.thread.path.localeCompare(c2.thread.path); + } + + // 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 (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) { return 0; } diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html index cb6ba28486..3e8707771d 100644 --- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html @@ -114,7 +114,6 @@ suite('gr-thread-list tests', () => { }, patch_set: 2, id: '8caddf38_44770ec1', - line: 4, updated: '2018-02-13 22:48:40.000000000', message: 'Another unresolved comment', unresolved: true, @@ -122,7 +121,6 @@ suite('gr-thread-list tests', () => { ], patchNum: 2, path: '/COMMIT_MSG', - line: 4, rootId: '8caddf38_44770ec1', start_datetime: '2018-02-13 22:48:40.000000000', }, @@ -205,7 +203,7 @@ suite('gr-thread-list tests', () => { }, patch_set: 4, id: 'rc2', - line: 5, + line: 7, updated: '2019-03-08 18:49:18.000000000', message: 'test', unresolved: true, @@ -228,7 +226,7 @@ suite('gr-thread-list tests', () => { ], patchNum: 4, path: '/COMMIT_MSG', - line: 5, + line: 7, rootId: 'rc2', start_datetime: '2019-03-08 18:49:18.000000000', }, @@ -259,25 +257,26 @@ suite('gr-thread-list tests', () => { test('_computeSortedThreads', () => { assert.equal(element._sortedThreads.length, 7); - // Draft and unresolved + // Draft and unresolved for commit-msg at line 5 assert.equal(element._sortedThreads[0].thread.rootId, 'ecf0b9fa_fe1a5f62'); - // Unresolved robot comment + // /COMMIT_MSG + // unresolved no draft and file level assert.equal(element._sortedThreads[1].thread.rootId, - 'rc2'); - // Unresolved robot comment - assert.equal(element._sortedThreads[2].thread.rootId, - 'rc1'); - // unresolved - assert.equal(element._sortedThreads[3].thread.rootId, - 'scaddf38_44770ec1'); - // unresolved - assert.equal(element._sortedThreads[4].thread.rootId, '8caddf38_44770ec1'); - // resolved and draft + // 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 + // resolved and on file test.txt assert.equal(element._sortedThreads[6].thread.rootId, '09a9fb0a_1484e6cf'); }); @@ -298,19 +297,20 @@ suite('gr-thread-list tests', () => { assert.equal(element._sortedThreads.length, 6); assert.equal(element._sortedThreads[0].thread.rootId, 'ecf0b9fa_fe1a5f62'); - // Unresolved robot comment + // /COMMIT_MSG + // unresolved no draft and file level assert.equal(element._sortedThreads[1].thread.rootId, - 'rc1'); - // unresolved + '8caddf38_44770ec1'); + // unresolved no draft at line 4 assert.equal(element._sortedThreads[2].thread.rootId, 'scaddf38_44770ec1'); - // unresolved + // unresolved no draft at line 5 assert.equal(element._sortedThreads[3].thread.rootId, - '8caddf38_44770ec1'); + 'rc1'); // resolved and draft assert.equal(element._sortedThreads[4].thread.rootId, 'zcf0b9fa_fe1a5f62'); - // resolved + // resolved and on file test.txt assert.equal(element._sortedThreads[5].thread.rootId, '09a9fb0a_1484e6cf'); });