opendev/gerrit
Code Issues Proposed changes

Update gr-thread-list to less eagerly re-filter

Browse Source 
Previously, showing and hiding drafts/unresolved in the thread list was
done via CSS, based on a reflected attribute of the thread list.
Because the attribute updated immediately, the CSS effect took place
when unwanted.

Instead of using CSS to hide and show items, this change filters them
instead.

This change also makes one fundamental change in the filtering of
threads-- threads are considered unresolved/resolved based on the latest
non-draft comment.  If a thread consists of only a single draft comment,
the unresolved bit is taken from that comment.

Furthermore, this change re-sorts and filters threads whenever a toggle
is selected, but not when a comment thread is modified on its own.

Bug: Issue 8713
Change-Id: I6f4457dbd193e3b2c60caff1e1178972dae9207c
This commit is contained in:
Becky Siegel
2018-05-03 16:17:36 -07:00
parent 6226dce0d1
commit 14eb78ff3a
3 changed files with 91 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
View File

@@ -54,10 +54,6 @@ limitations under the License.
display: flex;
margin-right: 1rem;
}
.draftsOnly gr-diff-comment-thread,
.unresolvedOnly gr-diff-comment-thread {
display: none
}
.draftsOnly:not(.unresolvedOnly) gr-diff-comment-thread[has-draft],
.unresolvedOnly:not(.draftsOnly) gr-diff-comment-thread[unresolved],
.draftsOnly.unresolvedOnly gr-diff-comment-thread[has-draft][unresolved] {
@@ -68,12 +64,12 @@ limitations under the License.
<div class="toggleItem">
<paper-toggle-button
id="unresolvedToggle"
on-change="_toggleUnresolved"></paper-toggle-button>
checked="{{_unresolvedOnly}}"></paper-toggle-button>
Only unresolved threads</div>
<div class$="toggleItem draftToggle [[_computeShowDraftToggle(loggedIn)]]">
<paper-toggle-button
id="draftToggle"
on-change="_toggleDrafts"></paper-toggle-button>
checked="{{_draftsOnly}}"></paper-toggle-button>
Only threads with drafts</div>
</div>
<div id="threads">
@@ -82,7 +78,7 @@ limitations under the License.
</template>
<template
is="dom-repeat"
items="[[_sortedThreads]]"
items="[[_filteredThreads]]"
as="thread"
initial-count="5"
target-framerate="60">

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

@@ -34,10 +34,24 @@
loggedIn: Boolean,
_sortedThreads: {
type: Array,
computed: '_computeSortedThreads(threads.*)',
},
_filteredThreads: {
type: Array,
computed: '_computeFilteredThreads(_sortedThreads, _unresolvedOnly, ' +
'_draftsOnly)',
},
_unresolvedOnly: {
type: Boolean,
value: false,
},
_draftsOnly: {
type: Boolean,
value: false,
},
},
observers: ['_computeSortedThreads(threads.*)'],
_computeShowDraftToggle(loggedIn) {
return loggedIn ? 'show' : '';
},
@@ -49,36 +63,60 @@
* - Resolved threads with drafts (reverse chronological)
* - Resolved threads without drafts (reverse chronological)
* @param {!Object} changeRecord
* @return {!Array}
*/
_computeSortedThreads(changeRecord) {
const threads = changeRecord.base;
if (!threads) { return []; }
return threads.map(this._getThreadWithSortInfo).sort((c1, c2) => {
const c1Date = c1.__date || util.parseDate(c1.updated);
const c2Date = c2.__date || util.parseDate(c2.updated);
const dateCompare = c2Date - c1Date;
if (c2.unresolved || c1.unresolved) {
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; }
}
this._updateSortedThreads(threads);
},
if (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) {
return 0;
_updateSortedThreads(threads) {
this._sortedThreads =
threads.map(this._getThreadWithSortInfo).sort((c1, c2) => {
const c1Date = c1.__date || util.parseDate(c1.updated);
const c2Date = c2.__date || util.parseDate(c2.updated);
const dateCompare = c2Date - c1Date;
if (c2.unresolved || c1.unresolved) {
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; }
}
if (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) {
return 0;
}
return dateCompare ? dateCompare : c1.id.localeCompare(c2.id);
});
},
_computeFilteredThreads(sortedThreads, unresolvedOnly, draftsOnly) {
return sortedThreads.filter(c => {
if (draftsOnly) {
return c.hasDraft;
} else if (unresolvedOnly) {
return c.unresolved;
} else {
return c;
}
return dateCompare ? dateCompare : c1.id.localeCompare(c2.id);
}).map(threadInfo => threadInfo.thread);
},
_getThreadWithSortInfo(thread) {
const lastComment = thread.comments[thread.comments.length - 1] || {};
const lastNonDraftComment =
(lastComment.__draft && thread.comments.length > 1) ?
thread.comments[thread.comments.length - 2] :
lastComment;
return {
thread,
unresolved: !!lastComment.unresolved,
// Use the unresolved bit for the last non draft comment. This is what
// anybody other than the current user would see.
unresolved: !!lastNonDraftComment.unresolved,
hasDraft: !!lastComment.__draft,
updated: lastComment.updated,
};
@@ -100,6 +138,10 @@
},
_handleCommentsChanged(e) {
// Reset threads so thread computations occur on deep array changes to
// threads comments that are not observed naturally.
this._updateSortedThreads(this.threads);
this.dispatchEvent(new CustomEvent('thread-list-modified',
{detail: {rootId: e.detail.rootId, path: e.detail.path}}));
},
@@ -107,13 +149,5 @@
_isOnParent(side) {
return !!side;
},
_toggleUnresolved() {
this.$.threads.classList.toggle('unresolvedOnly');
},
_toggleDrafts() {
this.$.threads.classList.toggle('draftsOnly');
},
});
})();

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

@@ -37,15 +37,6 @@ limitations under the License.
let element;
let sandbox;
let threadElements;
const computeVisibleNumber = threads => {
let count = 0;
for (const thread of threads) {
if (getComputedStyle(thread).display !== 'none') {
count++;
}
}
return count;
};
setup(() => {
sandbox = sinon.sandbox.create();
@@ -74,7 +65,7 @@ limitations under the License.
in_reply_to: 'ecf0b9fa_fe1a5f62',
updated: '2018-02-13 22:48:48.018000000',
message: 'draft',
unresolved: true,
unresolved: false,
__draft: true,
__draftID: '0.m683trwff68',
__editing: false,
@@ -196,54 +187,67 @@ limitations under the License.
});
test('there are five threads by default', () => {
assert.equal(computeVisibleNumber(threadElements), 5);
assert.equal(Polymer.dom(element.root)
.querySelectorAll('gr-diff-comment-thread').length, 5);
});
test('_computeSortedThreads', () => {
assert.equal(element._sortedThreads.length, 5);
// Draft and unresolved
assert.equal(element._sortedThreads[0].rootId, 'ecf0b9fa_fe1a5f62');
assert.equal(element._sortedThreads[0].thread.rootId,
'ecf0b9fa_fe1a5f62');
// unresolved
assert.equal(element._sortedThreads[1].rootId, 'scaddf38_44770ec1');
assert.equal(element._sortedThreads[1].thread.rootId,
'scaddf38_44770ec1');
// unresolved
assert.equal(element._sortedThreads[2].rootId, '8caddf38_44770ec1');
assert.equal(element._sortedThreads[2].thread.rootId,
'8caddf38_44770ec1');
// resolved and draft
assert.equal(element._sortedThreads[3].rootId, 'zcf0b9fa_fe1a5f62');
assert.equal(element._sortedThreads[3].thread.rootId,
'zcf0b9fa_fe1a5f62');
// resolved
assert.equal(element._sortedThreads[4].rootId, '09a9fb0a_1484e6cf');
assert.equal(element._sortedThreads[4].thread.rootId,
'09a9fb0a_1484e6cf');
});
test('thread removal', () => {
threadElements[1].fire('thread-discard', {rootId: 'scaddf38_44770ec1'});
flushAsynchronousOperations();
assert.equal(element._sortedThreads.length, 4);
assert.equal(element._sortedThreads[0].rootId, 'ecf0b9fa_fe1a5f62');
assert.equal(element._sortedThreads[0].thread.rootId,
'ecf0b9fa_fe1a5f62');
// unresolved
assert.equal(element._sortedThreads[1].rootId, '8caddf38_44770ec1');
assert.equal(element._sortedThreads[1].thread.rootId,
'8caddf38_44770ec1');
// resolved and draft
assert.equal(element._sortedThreads[2].rootId, 'zcf0b9fa_fe1a5f62');
assert.equal(element._sortedThreads[2].thread.rootId,
'zcf0b9fa_fe1a5f62');
// resolved
assert.equal(element._sortedThreads[3].rootId, '09a9fb0a_1484e6cf');
assert.equal(element._sortedThreads[3].thread.rootId,
'09a9fb0a_1484e6cf');
});
test('toggle unresolved only shows unressolved comments', () => {
MockInteractions.tap(element.$.unresolvedToggle);
flushAsynchronousOperations();
assert.equal(computeVisibleNumber(threadElements), 3);
assert.equal(Polymer.dom(element.root)
.querySelectorAll('gr-diff-comment-thread').length, 3);
});
test('toggle drafts only shows threads with draft comments', () => {
MockInteractions.tap(element.$.draftToggle);
flushAsynchronousOperations();
assert.equal(computeVisibleNumber(threadElements), 2);
assert.equal(Polymer.dom(element.root)
.querySelectorAll('gr-diff-comment-thread').length, 2);
});
test('toggle drafts and unresolved only shows threads with drafts and ' +
'unresolved', () => {
'publicly unresolved ', () => {
MockInteractions.tap(element.$.draftToggle);
MockInteractions.tap(element.$.unresolvedToggle);
flushAsynchronousOperations();
assert.equal(computeVisibleNumber(threadElements), 1);
assert.equal(Polymer.dom(element.root)
.querySelectorAll('gr-diff-comment-thread').length, 2);
});
test('modification events are consumed and displatched', () => {