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