Skip to content

Commit

Permalink
Drastically speed up tag selector refresh with many tags
Browse files Browse the repository at this point in the history 
When refreshing, if fewer than 100 tags to show, just create them from
scratch instead of updating the full set. Otherwise, remove the full set
from DOM and add it back in after updates to avoid reflows (from zotero#1204).

There are various things that could be done to optimize this further
(avoiding unnecessary sorting during full refreshes, calculating a hash
of the full set and not updating it every time), but we should probably
just replace it with @tnajdek's React version first.

Closes zotero#1204
  • Loading branch information
@dstillman
dstillman committed Mar 28, 2017
1 parent 8edd4b0 commit fe18633
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 61 deletions.
152 changes: 100 additions & 52 deletions chrome/content/zotero/bindings/tagselector.xml
View file
Expand Up @@ -267,64 +267,64 @@
fetch = true;
}
var emptyColored = true;
var emptyRegular = true;
this._emptyRegular = true;
var tagsBox = this.id('tags-box');
var tagColors = Zotero.Tags.getColors(this.libraryID);
// If new data, rebuild boxes
// If new data, rebuild buttons
if (fetch || this._dirty) {
this._tags = yield Zotero.Tags.getAll(this.libraryID, this._types)
.tap(() => Zotero.Promise.check(this.mode));
tagsBox.textContent = "";
// Add colored tags that aren't already real tags
let regularTags = new Set(this._tags.map(tag => tag.tag));
let coloredTags = new Set(tagColors.keys());
[for (x of coloredTags) if (!regularTags.has(x)) x].forEach(x =>
this._tags.push(Zotero.Tags.cleanData({ tag: x }))
);
let { div, emptyRegular } = this.createTagsList(this._tags);
// Sort by name
let collation = Zotero.getLocaleCollation();
this._tags.sort(function (a, b) {
return collation.compareString(1, a.tag, b.tag);
});
var fragment = document.createDocumentFragment();
let lastTag;
for (let i = 0; i < this._tags.length; i++) {
let tagData = this._tags[i];
// Only show tags of different types once
if (tagData.tag === lastTag) {
continue;
}
lastTag = tagData.tag;
let elem = this._insertClickableTag(fragment, tagData);
let visible = this._updateClickableTag(
elem, tagData.tag, tagColors
);
if (visible) {
emptyRegular = false;
}
}
tagsBox.appendChild(fragment);
this._emptyRegular = emptyRegular;
tagsBox.innerHTML = "";
tagsBox.appendChild(div);
this._dirty = false;
this._tagsDiv = null;
}
// Otherwise just update based on visibility
else {
elems = tagsBox.childNodes;
for (let i = 0; i < elems.length; i++) {
let elem = elems[i];
let visible = this._updateClickableTag(
elem, elem.textContent, tagColors
);
if (visible) {
emptyRegular = false;
// If only a few tags, regenerate buttons from scratch
if (Object.keys(this.scope).length <= 100) {
// If full set is currently displayed, store it for later
if (!this._tagsDiv) {
this._tagsDiv = tagsBox.firstChild;
}
let tags = [];
for (let name in this.scope) {
tags.push(...this.scope[name].map(type => {
return {
tag: name,
type: type
};
}));
}
let { div, emptyRegular } = this.createTagsList(tags);
tagsBox.replaceChild(div, tagsBox.firstChild);
this._emptyRegular = emptyRegular;
}
// Otherwise swap in the stored buttons from the last full run,
// after updating their visibility
else {
let oldDiv = tagsBox.removeChild(tagsBox.firstChild);
if (!this._tagsDiv) {
this._tagsDiv = oldDiv;
}
let elems = this._tagsDiv.childNodes;
let tagColors = Zotero.Tags.getColors(this.libraryID);
for (let i = 0; i < elems.length; i++) {
let elem = elems[i];
let visible = this._updateClickableTag(
elem, elem.textContent, tagColors
);
if (visible) {
this._emptyRegular = false;
}
}
tagsBox.appendChild(this._tagsDiv);
}
}
Expand Down Expand Up @@ -397,9 +397,8 @@
//end tag cloud code
this.updateNumSelected();
var empty = this._emptyRegular = emptyRegular;
// TODO: Show loading again when switching libraries/collections?
this.id('tags-deck').selectedIndex = empty ? 1 : 2;
this.id('tags-deck').selectedIndex = this._emptyRegular ? 1 : 2;
if (this.onRefresh) {
this.onRefresh();
Expand All @@ -415,7 +414,53 @@
</body>
</method>

<method name="createTagsList">
<parameter name="tags"/>
<body><![CDATA[
var tagColors = Zotero.Tags.getColors(this.libraryID);
// Add colored tags that aren't already real tags
var regularTags = new Set(tags.map(tag => tag.tag));
[...new Set(tagColors.keys())].filter(x => !regularTags.has(x)).forEach((x) => {
tags.push(Zotero.Tags.cleanData({ tag: x }));
});
// Sort by name
var t = new Date();
Zotero.debug("Sorting tags");
var collation = Zotero.getLocaleCollation();
tags.sort(function (a, b) {
return collation.compareString(1, a.tag, b.tag);
});
Zotero.debug(`Sorted tags in ${new Date() - t} ms`);
var div = document.createElementNS('http://www.w3.org/1999/xhtml', 'div');
var emptyRegular = true;
var lastTag;
for (let i = 0; i < tags.length; i++) {
let tagData = tags[i];
// Only show tags of different types once
if (tagData.tag === lastTag) {
continue;
}
lastTag = tagData.tag;
let elem = this._insertClickableTag(div, tagData);
let visible = this._updateClickableTag(
elem, tagData.tag, tagColors
);
if (visible) {
emptyRegular = false;
}
}
return { div, emptyRegular };
]]>
</body>
</method>

<method name="insertSorted">
<parameter name="tagsList"/>
<parameter name="tagObjs"/>
<body><![CDATA[
var tagColors = Zotero.Tags.getColors(this._libraryID);
Expand All @@ -426,8 +471,7 @@
});
// Create tag elements in sorted order
var tagsBox = this.id('tags-box');
var tagElems = tagsBox.childNodes;
var tagElems = tagsList.childNodes;
var j = 0;
loop:
for (let i = 0; i < tagObjs.length; i++) {
Expand All @@ -450,7 +494,7 @@
}
j++;
}
this._insertClickableTag(tagsBox, tagObj, tagElems[j]);
this._insertClickableTag(tagsList, tagObj, tagElems[j]);
this._updateClickableTag(
tagElems[j], tagElems[j].textContent, tagColors
);
Expand Down Expand Up @@ -553,7 +597,11 @@
}.bind(this));
if (tagObjs.length) {
this.insertSorted(tagObjs);
this.insertSorted(this.id('tags-box').firstChild, tagObjs);
// If full set isn't currently displayed, update it too
if (this._tagsDiv) {
this.insertSorted(this._tagsDiv, tagObjs);
}
}
}
// Don't add anything for item or collection-item; just update scope
Expand Down
6 changes: 3 additions & 3 deletions chrome/content/zotero/xpcom/collectionTreeRow.js
View file
Expand Up @@ -351,16 +351,16 @@ Zotero.CollectionTreeRow.prototype.getSearchObject = Zotero.Promise.coroutine(fu
/**
* Returns all the tags used by items in the current view
*
* @return {Promise}
* @return {Promise<Object[]>}
*/
Zotero.CollectionTreeRow.prototype.getChildTags = Zotero.Promise.coroutine(function* () {
switch (this.type) {
// TODO: implement?
case 'share':
return false;
return [];

case 'bucket':
return false;
return [];
}
var results = yield this.getSearchResults();
return Zotero.Tags.getAllWithinItemsList(results);
Expand Down
2 changes: 1 addition & 1 deletion chrome/content/zotero/xpcom/data/tags.js
View file
Expand Up @@ -171,7 +171,7 @@ Zotero.Tags = new function() {
throw new Error("ids must be an array");
}
if (!ids.length) {
return {};
return [];
}

var prefix = "SELECT DISTINCT name AS tag, type FROM itemTags "
Expand Down
9 changes: 6 additions & 3 deletions chrome/skin/default/zotero/bindings/tagselector.css
View file
Expand Up @@ -16,13 +16,16 @@ groupbox
}

#tags-box {
overflow-x: hidden;
overflow-y: auto;
background-color: -moz-field;
}

#tags-box > div {
display: flex;
flex-wrap: wrap;
align-items: flex-start;
align-content: flex-start;
overflow-x: hidden;
overflow-y: auto;
background-color: -moz-field;
}

#tags-box button {
Expand Down
5 changes: 3 additions & 2 deletions test/tests/tagSelectorTest.js
View file
Expand Up @@ -48,6 +48,7 @@ describe("Tag Selector", function () {
beforeEach(function* () {
var libraryID = Zotero.Libraries.userLibraryID;
yield clearTagColors(libraryID);
yield doc.getElementById('zotero-tag-selector').refresh(true);
})
after(function () {
win.close();
Expand Down Expand Up @@ -194,7 +195,7 @@ describe("Tag Selector", function () {
var libraryID = Zotero.Libraries.userLibraryID;

var tagSelector = doc.getElementById('zotero-tag-selector');
var tagElems = tagSelector.id('tags-box').childNodes;
var tagElems = tagSelector.id('tags-box').getElementsByTagName('button');
var count = tagElems.length;

yield Zotero.Tags.setColor(libraryID, "Top", '#AAAAAA');
Expand Down Expand Up @@ -228,7 +229,7 @@ describe("Tag Selector", function () {
yield promise;

var tagSelector = doc.getElementById('zotero-tag-selector');
var tagElems = tagSelector.id('tags-box').childNodes;
var tagElems = tagSelector.id('tags-box').getElementsByTagName('button');

// Make sure the colored tags are still in the right position
var tags = new Map();
Expand Down

0 comments on commit fe18633

Please sign in to comment.