From fe186333be019af1eb3f7b406f25d2b3002b3217 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 27 Mar 2017 20:33:20 -0400 Subject: [PATCH] Drastically speed up tag selector refresh with many tags 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 #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 #1204 --- .../content/zotero/bindings/tagselector.xml | 152 ++++++++++++------ .../content/zotero/xpcom/collectionTreeRow.js | 6 +- chrome/content/zotero/xpcom/data/tags.js | 2 +- .../default/zotero/bindings/tagselector.css | 9 +- test/tests/tagSelectorTest.js | 5 +- 5 files changed, 113 insertions(+), 61 deletions(-) diff --git a/chrome/content/zotero/bindings/tagselector.xml b/chrome/content/zotero/bindings/tagselector.xml index cd69fccc98..feb78cf897 100644 --- a/chrome/content/zotero/bindings/tagselector.xml +++ b/chrome/content/zotero/bindings/tagselector.xml @@ -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); } } @@ -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(); @@ -415,7 +414,53 @@ + + + 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 }; + ]]> + + + + } */ 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); diff --git a/chrome/content/zotero/xpcom/data/tags.js b/chrome/content/zotero/xpcom/data/tags.js index e5b689bbfa..d1af5c1598 100644 --- a/chrome/content/zotero/xpcom/data/tags.js +++ b/chrome/content/zotero/xpcom/data/tags.js @@ -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 " diff --git a/chrome/skin/default/zotero/bindings/tagselector.css b/chrome/skin/default/zotero/bindings/tagselector.css index fa6bf2f45e..21e0e06512 100644 --- a/chrome/skin/default/zotero/bindings/tagselector.css +++ b/chrome/skin/default/zotero/bindings/tagselector.css @@ -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 { diff --git a/test/tests/tagSelectorTest.js b/test/tests/tagSelectorTest.js index 13b286262f..20ae5532ed 100644 --- a/test/tests/tagSelectorTest.js +++ b/test/tests/tagSelectorTest.js @@ -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(); @@ -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'); @@ -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();