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
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 850bfb4..e89be0f5 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 @@
_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 @@
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 cb6ba28..3e87077 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 @@
},
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 @@
],
patchNum: 2,
path: '/COMMIT_MSG',
- line: 4,
rootId: '8caddf38_44770ec1',
start_datetime: '2018-02-13 22:48:40.000000000',
},
@@ -205,7 +203,7 @@
},
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 @@
],
patchNum: 4,
path: '/COMMIT_MSG',
- line: 5,
+ line: 7,
rootId: 'rc2',
start_datetime: '2019-03-08 18:49:18.000000000',
},
@@ -259,25 +257,26 @@
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 @@
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');
});
