From 33dedd17532f4ff3baa9896c565e0471729f415f Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 23 Jun 2015 04:35:45 -0400 Subject: [PATCH] Tags overhaul [DB reupgrade] - Simplified schema - Tags are now added without reloading entire tag selector - On my system, adding 400 tags to an item (separately, with the tag selector updating each time) went from 59 seconds to 42. (Given that it takes only 13 seconds with the tag selector closed, though, there's clearly more work to be done.) - Tag selector now uses HTML flexbox (in identical fashion, for now, but with the possibility of fancier changes later, and with streamlined logic thanks to the flexbox 'order' property) - Various async fixes - Tests --- .../zotero-platform/unix/tagselector.css | 0 .../zotero-platform/win/tagselector.css | 0 chrome/content/zotero/bindings/tagsbox.xml | 154 ++--- .../content/zotero/bindings/tagselector.xml | 633 +++++++++--------- chrome/content/zotero/longTagFixer.js | 1 + chrome/content/zotero/tagColorChooser.js | 27 +- .../zotero/xpcom/collectionTreeView.js | 12 +- .../content/zotero/xpcom/data/dataObjects.js | 2 +- chrome/content/zotero/xpcom/data/item.js | 29 +- chrome/content/zotero/xpcom/data/tags.js | 239 +++---- chrome/content/zotero/xpcom/itemTreeView.js | 52 +- chrome/content/zotero/xpcom/schema.js | 6 +- chrome/content/zotero/xpcom/syncedSettings.js | 1 + chrome/content/zotero/xpcom/zotero.js | 1 - chrome/content/zotero/zoteroPane.js | 19 +- chrome/locale/en-US/zotero/zotero.dtd | 3 - .../default/zotero/bindings/tagselector.css | 44 +- components/zotero-autocomplete.js | 5 +- resource/schema/triggers.sql | 18 - resource/schema/userdata.sql | 4 +- test/tests/supportTest.js | 2 +- test/tests/tagSelectorTest.js | 232 ++++++- test/tests/tagsTest.js | 31 +- test/tests/tagsboxTest.js | 125 ++++ 24 files changed, 951 insertions(+), 689 deletions(-) delete mode 100644 chrome/content/zotero-platform/unix/tagselector.css delete mode 100644 chrome/content/zotero-platform/win/tagselector.css create mode 100644 test/tests/tagsboxTest.js diff --git a/chrome/content/zotero-platform/unix/tagselector.css b/chrome/content/zotero-platform/unix/tagselector.css deleted file mode 100644 index e69de29bb..000000000 diff --git a/chrome/content/zotero-platform/win/tagselector.css b/chrome/content/zotero-platform/win/tagselector.css deleted file mode 100644 index e69de29bb..000000000 diff --git a/chrome/content/zotero/bindings/tagsbox.xml b/chrome/content/zotero/bindings/tagsbox.xml index 47153bf4c..2b846030e 100644 --- a/chrome/content/zotero/bindings/tagsbox.xml +++ b/chrome/content/zotero/bindings/tagsbox.xml @@ -117,7 +117,9 @@ this.mode = this.getAttribute('mode'); } - this._notifierID = Zotero.Notifier.registerObserver(this, ['item-tag', 'setting']); + this._notifierID = Zotero.Notifier.registerObserver( + this, ['item-tag', 'setting'], 'tagsbox' + ); ]]> @@ -134,73 +136,72 @@ - - parseInt(x)); + if (!this.item || itemID != this.item.id) { + continue; + } + let data = extraData[ids[i]]; + let tagName = data.tag; + + if (event == 'add') { + var newTabIndex = this.add(tagName); + if (newTabIndex == -1) { + return; + } + if (this._tabDirection == -1) { + if (this._lastTabIndex > newTabIndex) { + this._lastTabIndex++; + } + } + else if (this._tabDirection == 1) { + if (this._lastTabIndex > newTabIndex) { + this._lastTabIndex++; + } + } + } + else if (event == 'modify') { + let oldTagName = data.old.tag; + this.remove(oldTagName); + this.add(tagName); + } + else if (event == 'remove') { + var oldTabIndex = this.remove(tagName); + if (oldTabIndex == -1) { + return; + } + if (this._tabDirection == -1) { + if (this._lastTabIndex > oldTabIndex) { + this._lastTabIndex--; + } + } + else if (this._tabDirection == 1) { + if (this._lastTabIndex >= oldTabIndex) { + this._lastTabIndex--; + } + } + } } - if (event == 'add') { - var newTabIndex = this.add(tagName); - if (newTabIndex == -1) { - return; - } - if (this._tabDirection == -1) { - if (this._lastTabIndex > newTabIndex) { - this._lastTabIndex++; - } - } - else if (this._tabDirection == 1) { - if (this._lastTabIndex > newTabIndex) { - this._lastTabIndex++; - } - } - } - else if (event == 'modify') { - Zotero.debug("EXTRA"); - Zotero.debug(extraData); - let oldTagName = extraData[tagName].old.tag; - this.remove(oldTagName); - this.add(tagName); - } - else if (event == 'remove') { - var oldTabIndex = this.remove(tagName); - if (oldTabIndex == -1) { - return; - } - if (this._tabDirection == -1) { - if (this._lastTabIndex > oldTabIndex) { - this._lastTabIndex--; - } - } - else if (this._tabDirection == 1) { - if (this._lastTabIndex >= oldTabIndex) { - this._lastTabIndex--; - } - } + this.updateCount(); + } + else if (type == 'tag') { + if (event == 'modify') { + return this.reload(); } } - - this.updateCount(); - } - else if (type == 'tag') { - if (event == 'modify') { - this.reload(); - } - } - ]]> - + }.bind(this)); + ]]> @@ -235,6 +236,9 @@ this._reloading = false; this._focusField(); + + var event = new Event('refresh'); + this.dispatchEvent(event); }, this); ]]> @@ -388,13 +392,10 @@ } // Tag color - let color = this._tagColors[valueText]; - if (color) { - valueElement.setAttribute( - 'style', - 'color:' + this._tagColors[valueText].color + '; ' - + 'font-weight: bold' - ); + var colorData = this._tagColors.get(valueText); + if (colorData) { + valueElement.style.color = colorData.color; + valueElement.style.fontWeight = 'bold'; } return valueElement; @@ -522,12 +523,10 @@ yield this.blurHandler(target); if (focusField) { - Zotero.debug("FOCUSING FIELD"); this._focusField(); } // Return focus to items pane else { - Zotero.debug("FOCUSING ITEM PANE"); var tree = document.getElementById('zotero-items-tree'); if (tree) { tree.focus(); @@ -757,8 +756,6 @@ false false null + null - null null @@ -173,7 +173,7 @@ Zotero.Promise.check(this.mode)); + + // If new data, rebuild boxes if (fetch || this._dirty) { this._tags = yield Zotero.Tags.getAll(this.libraryID, this._types) .tap(() => Zotero.Promise.check(this.mode)); - - // Remove children - tagsToggleBox.textContent = ""; + tagsBox.textContent = ""; // Sort by name - var collation = Zotero.getLocaleCollation(); - var orderedTags = this._tags.concat(); - orderedTags.sort(function(a, b) { + let collation = Zotero.getLocaleCollation(); + this._tags.sort(function (a, b) { return collation.compareString(1, a.tag, b.tag); }); - var tagColorsLowerCase = {}; - var colorTags = []; - for (let name in tagColors) { - colorTags[tagColors[name].position] = name; - tagColorsLowerCase[name.toLowerCase()] = true; - } - var positions = Object.keys(colorTags); - for (let i=positions.length-1; i>=0; i--) { - let name = colorTags[positions[i]]; - orderedTags.unshift({ - tag: name, - type: 0, - hasColor: true - }); - } - - var lastTag; - for (let i=0; i - - + + - - - + @@ -525,11 +452,12 @@ + extraData[x]) + // Ignore tag adds for items not in the current library, if there is one + .filter(function (x) { + if (!this._libraryID) return true; + return x.libraryID == this._libraryID; + }.bind(this)); + + if (tagObjs.length) { + yield this.insertSorted(tagObjs); + } + } + // Don't add anything for item or collection-item; just update scope + + return this.updateScope(); + } + var t = this.id('tags-search').inputField; if (t.value) { this.setSearch(t.value, true); @@ -610,34 +556,19 @@ - - - - - - - - - + - - - - - @@ -687,7 +610,7 @@ - + @@ -768,108 +690,216 @@ - - - + ]]> - - - + }.bind(this)); + ]]> + + + + + + + + - - - + var self = this; + elem.addEventListener('click', function(event) { + self.handleTagClick(event, this); + }); + if (this.editable) { + elem.addEventListener('mousedown', function (event) { + if (event.button == 2) { + // Without the setTimeout, the popup gets immediately hidden + // for some reason + setTimeout(function () { + _popupNode = elem; + self.id('tag-menu').openPopup( + null, + 'after_pointer', + event.clientX + 2, + event.clientY + 2, + true, + event + ); + event.stopPropagation(); + event.preventDefault(); + }); + } + }, true); + elem.addEventListener('dragover', this.dragObserver.onDragOver); + elem.addEventListener('dragexit', this.dragObserver.onDragExit); + elem.addEventListener('drop', this.dragObserver.onDrop, true); + } + return elem; + ]]> + + + + + + + + - - = Zotero.Tags.MAX_COLORED_TAGS && !tagColors[io.name]) { + = Zotero.Tags.MAX_COLORED_TAGS && !tagColors.has(io.name)) { var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] .getService(Components.interfaces.nsIPromptService); ps.alert(null, "", Zotero.getString('pane.tagSelector.maxColoredTags', Zotero.Tags.MAX_COLORED_TAGS)); return; } - // Opening a modal window directly from within this promise handler causes - // the opened window to block on the first yielded promise until the window - // is closed. - setTimeout(function () { - window.openDialog( - 'chrome://zotero/content/tagColorChooser.xul', - "zotero-tagSelector-colorChooser", - "chrome,modal,centerscreen", io - ); - - // Dialog cancel - if (typeof io.color == 'undefined') { - return; - } - - Zotero.Tags.setColor(self.libraryID, io.name, io.color, io.position); - }, 0); - }); + io.tagColors = tagColors; + + window.openDialog( + 'chrome://zotero/content/tagColorChooser.xul', + "zotero-tagSelector-colorChooser", + "chrome,modal,centerscreen", io + ); + + // Dialog cancel + if (typeof io.color == 'undefined') { + return; + } + + yield Zotero.Tags.setColor(this.libraryID, io.name, io.color, io.position); + }.bind(this)); ]]> @@ -928,7 +958,7 @@ return Zotero.DB.executeTransaction(function* () { ids = ids.split(','); var items = Zotero.Items.get(ids); - var value = node.getAttribute('value') + var value = node.textContent for (let i=0; i - - + + + oncommand="_openColorPickerWindow(_popupNode.textContent); event.stopPropagation()"/> + oncommand="document.getBindingParent(this).rename(_popupNode.textContent); event.stopPropagation()"/> + oncommand="document.getBindingParent(this).delete(_popupNode.textContent); event.stopPropagation()"/> - - + + + + - - - + + + + - @@ -989,14 +1024,16 @@ document.getElementById('display-all-tags').setAttribute('checked', !document.getBindingParent(this).filterToScope);"> + oncommand="document.getBindingParent(this).deselectAll(); event.stopPropagation();"/> + this.setAttribute('checked', showAutomatic); + ts.refresh(); + event.stopPropagation();"/> } - A tag name, or false if tag with id not found */ - this.getName = function (libraryID, tagID) { - if (!tagID) { - throw new Error("tagID not provided"); - } - if (_tagNamesByID[tagID]) { - return _tagNamesByID[tagID]; - } - _requireLoad(libraryID); - return false; + this.getName = function (tagID) { + return Zotero.DB.valueQueryAsync("SELECT name FROM tags WHERE tagID=?", tagID); } - /** - * Returns the tagID matching a given tag - * - * @param {Number} libraryID - * @param {String} name - */ - this.getID = function (libraryID, name) { - if (_tagIDsByName[libraryID] && _tagIDsByName[libraryID]['_' + name]) { - return _tagIDsByName[libraryID]['_' + name]; - } - _requireLoad(libraryID); - return false; - }; - - /** * Returns the tagID matching given fields, or creates a new tag and returns its id * - * @requireTransaction - * @param {Number} libraryID * @param {String} name - Tag data in API JSON format - * @param {Boolean} [create=false] - If no matching tag, create one + * @param {Boolean} [create=false] - If no matching tag, create one; + * requires a wrapping transaction * @return {Promise} tagID */ - this.getIDFromName = Zotero.Promise.coroutine(function* (libraryID, name, create) { - Zotero.DB.requireTransaction(); + this.getID = Zotero.Promise.coroutine(function* (name, create) { + if (create) { + Zotero.DB.requireTransaction(); + } data = this.cleanData({ tag: name }); - var sql = "SELECT tagID FROM tags WHERE libraryID=? AND name=?"; - var id = yield Zotero.DB.valueQueryAsync(sql, [libraryID, data.tag]); + var sql = "SELECT tagID FROM tags WHERE name=?"; + var id = yield Zotero.DB.valueQueryAsync(sql, data.tag); if (!id && create) { id = yield Zotero.ID.get('tags'); - let sql = "INSERT INTO tags (tagID, libraryID, name) VALUES (?, ?, ?)"; - let insertID = yield Zotero.DB.queryAsync(sql, [id, libraryID, data.tag]); + let sql = "INSERT INTO tags (tagID, name) VALUES (?, ?)"; + let insertID = yield Zotero.DB.queryAsync(sql, [id, data.tag]); if (!id) { id = insertID; } - _cacheTag(libraryID, id, data.tag); } return id; }); - /* - * Returns an array of tag types for tags matching given tag - */ - this.getTypes = Zotero.Promise.method(function (name, libraryID) { - if (libraryID != parseInt(libraryID)) { - throw new Error("libraryID must be an integer"); - } - - var sql = "SELECT type FROM tags WHERE libraryID=? AND name=?"; - return Zotero.DB.columnQueryAsync(sql, [libraryID, name.trim()]); - }); - - /** * Get all tags indexed by tagID * @@ -125,7 +86,7 @@ Zotero.Tags = new function() { */ this.getAll = Zotero.Promise.coroutine(function* (libraryID, types) { var sql = "SELECT DISTINCT name AS tag, type FROM tags " - + "JOIN itemTags USING (tagID) WHERE libraryID=?"; + + "JOIN itemTags USING (tagID) JOIN items USING (itemID) WHERE libraryID=?"; var params = [libraryID]; if (types) { sql += " AND type IN (" + types.join() + ")"; @@ -177,14 +138,15 @@ Zotero.Tags = new function() { /** - * Get the items associated with the given saved tag + * Get the items associated with the given tag * * @param {Number} tagID * @return {Promise} A promise for an array of itemIDs */ - this.getTagItems = function (tagID) { - var sql = "SELECT itemID FROM itemTags WHERE tagID=?"; - return Zotero.DB.columnQueryAsync(sql, tagID); + this.getTagItems = function (libraryID, tagID) { + var sql = "SELECT itemID FROM itemTags JOIN items USING (itemID) " + + "WHERE tagID=? AND libraryID=?"; + return Zotero.DB.columnQueryAsync(sql, [tagID, libraryID]); } @@ -198,28 +160,6 @@ Zotero.Tags = new function() { }); - this.load = Zotero.Promise.coroutine(function* (libraryID, reload) { - if (_loaded[libraryID] && !reload) { - return; - } - - Zotero.debug("Loading tags in library " + libraryID); - - var sql = 'SELECT tagID AS id, name FROM tags WHERE libraryID=?'; - var tags = yield Zotero.DB.queryAsync(sql, libraryID); - - _tagIDsByName[libraryID] = {} - - for (var i=0; i itemID + '-' + newTagID), notifierData ); - yield this.purge(libraryID, oldTagID); + yield this.purge(oldTagID); }.bind(this)); if (oldColorData) { @@ -302,30 +245,48 @@ Zotero.Tags = new function() { /** * @return {Promise} */ - this.erase = Zotero.Promise.coroutine(function* (libraryID, tagIDs) { + this.removeFromLibrary = Zotero.Promise.coroutine(function* (libraryID, tagIDs) { tagIDs = Zotero.flattenArguments(tagIDs); var deletedNames = []; var oldItemIDs = []; yield Zotero.DB.executeTransaction(function* () { - yield Zotero.Tags.load(libraryID); - + var notifierPairs = []; + var notifierData = {}; for (let i=0; i '?').join(', ') + ") AND itemID IN " + + "(SELECT itemID FROM items WHERE libraryID=?)"; + yield Zotero.DB.queryAsync(sql, tagIDs.concat([libraryID])); + + yield this.purge(tagIDs); // Update internal timestamps on all items that had these tags yield Zotero.Utilities.Internal.forEachChunkAsync( @@ -334,11 +295,11 @@ Zotero.Tags = new function() { function* (chunk) { let placeholders = chunk.map(function () '?').join(','); - sql = 'UPDATE items SET clientDateModified=? ' + sql = 'UPDATE items SET synced=0, clientDateModified=? ' + 'WHERE itemID IN (' + placeholders + ')' yield Zotero.DB.queryAsync(sql, [Zotero.DB.transactionDateTime].concat(chunk)); - yield Zotero.Items.reload(oldItemIDs, ['tags']); + yield Zotero.Items.reload(oldItemIDs, ['primaryData', 'tags'], true); } ); }.bind(this)); @@ -355,7 +316,7 @@ Zotero.Tags = new function() { /** - * Delete obsolete tags from database and clear internal cache entries + * Delete obsolete tags from database * * @param {Number} libraryID * @param {Number|Number[]} [tagIDs] - tagID or array of tagIDs to purge @@ -379,7 +340,7 @@ Zotero.Tags = new function() { for (let i=0; i} - A promise for a Map with tag names as keys and + * objects containing 'color' and 'position' as values */ this.getColors = Zotero.Promise.coroutine(function* (libraryID) { if (_libraryColorsByName[libraryID]) { @@ -487,14 +442,14 @@ Zotero.Tags = new function() { tagColors = tagColors || []; _libraryColors[libraryID] = tagColors; - _libraryColorsByName[libraryID] = {}; + _libraryColorsByName[libraryID] = new Map; // Also create object keyed by name for quick checking for individual tag colors for (let i=0; i - - - diff --git a/chrome/skin/default/zotero/bindings/tagselector.css b/chrome/skin/default/zotero/bindings/tagselector.css index f5f11e569..fa6bf2f45 100644 --- a/chrome/skin/default/zotero/bindings/tagselector.css +++ b/chrome/skin/default/zotero/bindings/tagselector.css @@ -6,41 +6,51 @@ groupbox padding: 1px 1px 0; } -#tags-toggle -{ +#tags-deck { + -moz-box-flex: 1; +} + +#tags-deck > box { + -moz-box-align: center; + -moz-box-pack: center; +} + +#tags-box { + display: flex; + flex-wrap: wrap; + align-items: flex-start; + align-content: flex-start; overflow-x: hidden; overflow-y: auto; - display: block; /* allow labels to wrap instead of all being in one line */ background-color: -moz-field; } -checkbox -{ - margin: .75em 0 .4em; -} - -#tags-toggle label -{ +#tags-box button { margin: .15em .05em .15em .3em !important; padding: 0 .25em 0 .25em !important; - -moz-user-focus: ignore; max-width: 250px; + overflow: hidden; + text-overflow: ellipsis; + background-color: transparent; + text-align: left; + white-space: nowrap; border: 1px solid transparent; /* always include border so height is same as zotero-clicky */ + -moz-appearance: none; + -moz-padding-start: 0 !important; + -moz-padding-end: 0 !important; + -moz-user-focus: ignore; } /* Visible out-of-scope tags should be grey */ -#tags-toggle label[inScope=false]:not([hasColor=true]) -{ +#tags-box button[inScope=false]:not([hasColor=true]) { color: #666 !important; } -#tags-toggle label[inScope=false][hasColor=true] -{ +#tags-box button[inScope=false][hasColor=true] { opacity: .6; } -#tags-toggle label[draggedOver="true"] -{ +#tags-box button[draggedOver="true"] { color: white !important; background: #666; } diff --git a/components/zotero-autocomplete.js b/components/zotero-autocomplete.js index c4d8bb03e..901f5229a 100644 --- a/components/zotero-autocomplete.js +++ b/components/zotero-autocomplete.js @@ -73,8 +73,9 @@ ZoteroAutoComplete.prototype.startSearch = Zotero.Promise.coroutine(function* (s case 'tag': var sql = "SELECT DISTINCT name AS val, NULL AS comment FROM tags WHERE name LIKE ?"; var sqlParams = [searchString + '%']; - if (typeof searchParams.libraryID != 'undefined') { - sql += " AND libraryID=?"; + if (searchParams.libraryID !== undefined) { + sql += " AND tagID IN (SELECT tagID FROM itemTags JOIN items USING (itemID) " + + "WHERE libraryID=?)"; sqlParams.push(searchParams.libraryID); } if (searchParams.itemID) { diff --git a/resource/schema/triggers.sql b/resource/schema/triggers.sql index 01827b0d1..041ce800a 100644 --- a/resource/schema/triggers.sql +++ b/resource/schema/triggers.sql @@ -210,24 +210,6 @@ CREATE TRIGGER fku_itemNotes END; --- itemTags libraryID -DROP TRIGGER IF EXISTS fki_itemTags_libraryID; -CREATE TRIGGER fki_itemTags_libraryID - BEFORE INSERT ON itemTags - FOR EACH ROW BEGIN - SELECT RAISE(ABORT, 'insert on table "itemTags" violates foreign key constraint "fki_itemTags_libraryID"') - WHERE (SELECT libraryID FROM tags WHERE tagID = NEW.tagID) != (SELECT libraryID FROM items WHERE itemID = NEW.itemID);--- - END; - -DROP TRIGGER IF EXISTS fku_itemTags_libraryID; -CREATE TRIGGER fku_itemTags_libraryID - BEFORE UPDATE ON itemTags - FOR EACH ROW BEGIN - SELECT RAISE(ABORT, 'update on table "itemTags" violates foreign key constraint "fku_itemTags_libraryID"') - WHERE (SELECT libraryID FROM tags WHERE tagID = NEW.tagID) != (SELECT libraryID FROM items WHERE itemID = NEW.itemID);--- - END; - - -- Make sure tags aren't empty DROP TRIGGER IF EXISTS fki_tags; CREATE TRIGGER fki_tags diff --git a/resource/schema/userdata.sql b/resource/schema/userdata.sql index d70659ae0..ad23af86f 100644 --- a/resource/schema/userdata.sql +++ b/resource/schema/userdata.sql @@ -115,9 +115,7 @@ CREATE INDEX itemAttachments_syncState ON itemAttachments(syncState); CREATE TABLE tags ( tagID INTEGER PRIMARY KEY, - libraryID INT NOT NULL, - name TEXT NOT NULL, - UNIQUE (libraryID, name) + name TEXT NOT NULL UNIQUE ); CREATE TABLE itemRelations ( diff --git a/test/tests/supportTest.js b/test/tests/supportTest.js index e7d35e732..aea221828 100644 --- a/test/tests/supportTest.js +++ b/test/tests/supportTest.js @@ -77,7 +77,7 @@ describe("Support Functions for Unit Testing", function() { let tags = data.itemWithTags.tags; for (let i=0; i= 0 && elems[i].style.display != 'none') { + names.push(elems[i].textContent); + } + } + return names; + } + + before(function* () { win = yield loadZoteroPane(); doc = win.document; @@ -11,6 +45,10 @@ describe("Tag Selector", function () { // Wait for things to settle yield Zotero.Promise.delay(100); }); + beforeEach(function* () { + var libraryID = Zotero.Libraries.userLibraryID; + yield clearTagColors(libraryID); + }) after(function () { win.close(); }); @@ -27,7 +65,7 @@ describe("Tag Selector", function () { } describe("#notify()", function () { - it("should add a tag when added to an item in the current view", function* () { + it("should add a tag when added to an item in the library root", function* () { var promise, tagSelector; if (collectionsView.selection.currentIndex != 0) { @@ -41,6 +79,10 @@ describe("Tag Selector", function () { item.setTags([ { tag: 'A' + }, + { + tag: 'B', + type: 1 } ]); promise = waitForTagSelector(); @@ -48,8 +90,11 @@ describe("Tag Selector", function () { yield promise; // Tag selector should have at least one tag - tagSelector = doc.getElementById('zotero-tag-selector'); - assert.isAbove(tagSelector.getVisible().length, 0); + assert.isAbove(getRegularTags().length, 1); + }); + + it("should add a tag when an item is added in a collection", function* () { + var promise, tagSelector; // Add collection promise = waitForTagSelector(); @@ -57,14 +102,13 @@ describe("Tag Selector", function () { yield promise; // Tag selector should be empty in new collection - tagSelector = doc.getElementById('zotero-tag-selector'); - assert.equal(tagSelector.getVisible().length, 0); + assert.equal(getRegularTags().length, 0); // Add item with tag to collection var item = createUnsavedDataObject('item'); item.setTags([ { - tag: 'B' + tag: 'C' } ]); item.setCollections([collection.id]); @@ -72,9 +116,80 @@ describe("Tag Selector", function () { yield item.saveTx(); yield promise; + // Tag selector should show the new item's tag + assert.equal(getRegularTags().length, 1); + }) + + it("should add a tag when an item is added to a collection", function* () { + var promise, tagSelector; + + // Add collection + promise = waitForTagSelector(); + var collection = yield createDataObject('collection'); + yield promise; + + // Tag selector should be empty in new collection + assert.equal(getRegularTags().length, 0); + + // Add item with tag to library root + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: 'C' + } + ]); + promise = waitForTagSelector() + yield item.saveTx(); + yield promise; + + // Tag selector should still be empty in collection + assert.equal(getRegularTags().length, 0); + + item.setCollections([collection.id]); + promise = waitForTagSelector(); + yield item.saveTx(); + yield promise; + // Tag selector should show the new item's tag tagSelector = doc.getElementById('zotero-tag-selector'); - assert.equal(tagSelector.getVisible().length, 1); + assert.equal(getRegularTags().length, 1); + }) + + it("shouldn't re-insert a new tag that matches an existing color", function* () { + var libraryID = Zotero.Libraries.userLibraryID; + + /*// Remove all tags in library + var tags = yield Zotero.Tags.getAll(libraryID); + tags.forEach(function (tag) { + var tagID = yield Zotero.Tags.getID(tag); + yield Zotero.Tags.removeFromLibrary(libraryID, tagID); + });*/ + + // Add B and A as colored tags without any items + yield Zotero.Tags.setColor(libraryID, "B", '#990000'); + yield Zotero.Tags.setColor(libraryID, "A", '#CC9933'); + + // Add A to an item to make it a real tag + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: "A" + } + ]); + var promise = waitForTagSelector(); + yield item.saveTx(); + yield promise; + + var tagSelector = doc.getElementById('zotero-tag-selector'); + var tagElems = tagSelector.id('tags-box').childNodes; + + // Make sure the colored tags are still in the right position + var tags = new Map(); + for (let i = 0; i < tagElems.length; i++) { + tags.set(tagElems[i].textContent, tagElems[i].style.order); + } + assert.isBelow(parseInt(tags.get("B")), 0); + assert.isBelow(parseInt(tags.get("B")), parseInt(tags.get("A"))); }) it("should remove a tag when an item is removed from a collection", function* () { @@ -91,13 +206,12 @@ describe("Tag Selector", function () { } ]); item.setCollections([collection.id]); - promise = waitForTagSelector() + promise = waitForTagSelector(); yield item.saveTx(); yield promise; // Tag selector should show the new item's tag - var tagSelector = doc.getElementById('zotero-tag-selector'); - assert.equal(tagSelector.getVisible().length, 1); + assert.equal(getRegularTags().length, 1); item.setCollections(); promise = waitForTagSelector(); @@ -105,8 +219,7 @@ describe("Tag Selector", function () { yield promise; // Tag selector shouldn't show the removed item's tag - tagSelector = doc.getElementById('zotero-tag-selector'); - assert.equal(tagSelector.getVisible().length, 0); + assert.equal(getRegularTags().length, 0); }) it("should remove a tag when an item in a collection is moved to the trash", function* () { @@ -128,8 +241,7 @@ describe("Tag Selector", function () { yield promise; // Tag selector should show the new item's tag - var tagSelector = doc.getElementById('zotero-tag-selector'); - assert.equal(tagSelector.getVisible().length, 1); + assert.equal(getRegularTags().length, 1); // Move item to trash item.deleted = true; @@ -138,8 +250,96 @@ describe("Tag Selector", function () { yield promise; // Tag selector shouldn't show the deleted item's tag - tagSelector = doc.getElementById('zotero-tag-selector'); - assert.equal(tagSelector.getVisible().length, 0); + assert.equal(getRegularTags().length, 0); + }) + + it("should remove a tag when a tag is deleted for a library", function* () { + yield selectLibrary(win); + + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: 'A' + } + ]); + var promise = waitForTagSelector(); + yield item.saveTx(); + yield promise; + + // Tag selector should show the new item's tag + assert.include(getRegularTags(), "A"); + + // Remove tag from library + promise = waitForTagSelector(); + var dialogPromise = waitForDialog(); + var tagSelector = doc.getElementById('zotero-tag-selector'); + yield tagSelector.delete("A"); + yield promise; + + // Tag selector shouldn't show the deleted item's tag + assert.notInclude(getRegularTags(), "A"); }) }) + + describe("#rename()", function () { + it("should rename a tag and update the tag selector", function* () { + yield selectLibrary(win); + + var tag = Zotero.Utilities.randomString(); + var newTag = Zotero.Utilities.randomString(); + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: tag + } + ]); + var promise = waitForTagSelector(); + yield item.saveTx(); + yield promise; + + var tagSelector = doc.getElementById('zotero-tag-selector'); + promise = waitForTagSelector(); + var promptPromise = waitForWindow("chrome://global/content/commonDialog.xul", function (dialog) { + dialog.document.getElementById('loginTextbox').value = newTag; + dialog.document.documentElement.acceptDialog(); + }) + yield tagSelector.rename(tag); + yield promise; + + var tags = getRegularTags(); + assert.include(tags, newTag); + }) + }) + + describe("#_openColorPickerWindow()", function () { + it("should assign a color to a tag", function* () { + yield selectLibrary(win); + var tag = "b " + Zotero.Utilities.randomString(); + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: "a" + }, + { + tag: tag + } + ]); + var promise = waitForTagSelector(); + yield item.saveTx(); + yield promise; + + var tagSelector = doc.getElementById('zotero-tag-selector'); + + assert.include(getRegularTags(), "a"); + + var dialogPromise = waitForDialog(false, undefined, 'chrome://zotero/content/tagColorChooser.xul'); + var tagSelectorPromise = waitForTagSelector(); + yield tagSelector._openColorPickerWindow(tag); + yield dialogPromise; + yield tagSelectorPromise; + + assert.include(getColoredTags(), tag); + assert.notInclude(getRegularTags(), tag); + }) + }); }) diff --git a/test/tests/tagsTest.js b/test/tests/tagsTest.js index b9ac5e3c6..7c2f1de76 100644 --- a/test/tests/tagsTest.js +++ b/test/tests/tagsTest.js @@ -8,7 +8,7 @@ describe("Zotero.Tags", function () { item.addTag(tagName); yield item.saveTx(); - assert.typeOf(Zotero.Tags.getID(Zotero.Libraries.userLibraryID, tagName), "number"); + assert.typeOf((yield Zotero.Tags.getID(tagName)), "number"); }) }) @@ -19,32 +19,49 @@ describe("Zotero.Tags", function () { item.addTag(tagName); yield item.saveTx(); - var tagID = Zotero.Tags.getID(Zotero.Libraries.userLibraryID, tagName); - assert.equal(Zotero.Tags.getName(Zotero.Libraries.userLibraryID, tagID), tagName); + var libraryID = Zotero.Libraries.userLibraryID; + var tagID = yield Zotero.Tags.getID(tagName); + assert.equal((yield Zotero.Tags.getName(tagID)), tagName); + }) + }) + + describe("#removeFromLibrary()", function () { + it("should reload tags of associated items", function* () { + var libraryID = Zotero.Libraries.userLibraryID; + + var tagName = Zotero.Utilities.randomString(); + var item = createUnsavedDataObject('item'); + item.addTag(tagName); + yield item.saveTx(); + assert.lengthOf(item.getTags(), 1); + + var tagID = yield Zotero.Tags.getID(tagName); + yield Zotero.Tags.removeFromLibrary(libraryID, tagID); + assert.lengthOf(item.getTags(), 0); }) }) describe("#purge()", function () { it("should remove orphaned tags", function* () { var libraryID = Zotero.Libraries.userLibraryID; + var tagName = Zotero.Utilities.randomString(); var item = createUnsavedDataObject('item'); item.addTag(tagName); yield item.saveTx(); - var tagID = Zotero.Tags.getID(libraryID, tagName); + var tagID = yield Zotero.Tags.getID(tagName); assert.typeOf(tagID, "number"); yield item.eraseTx(); - assert.equal(Zotero.Tags.getName(libraryID, tagID), tagName); + assert.equal((yield Zotero.Tags.getName(tagID)), tagName); yield Zotero.DB.executeTransaction(function* () { yield Zotero.Tags.purge(); }); - yield Zotero.Tags.load(libraryID); - assert.isFalse(Zotero.Tags.getName(libraryID, tagID)); + assert.isFalse(yield Zotero.Tags.getName(tagID)); }) }) }) diff --git a/test/tests/tagsboxTest.js b/test/tests/tagsboxTest.js new file mode 100644 index 000000000..3c10710fc --- /dev/null +++ b/test/tests/tagsboxTest.js @@ -0,0 +1,125 @@ +"use strict"; + +describe("Item Tags Box", function () { + var win, doc, collectionsView; + + before(function* () { + win = yield loadZoteroPane(); + doc = win.document; + + // Wait for things to settle + yield Zotero.Promise.delay(100); + }); + after(function () { + win.close(); + }); + + function waitForTagsBox() { + var deferred = Zotero.Promise.defer(); + var tagsbox = doc.getElementById('zotero-editpane-tags'); + var onRefresh = function (event) { + tagsbox.removeEventListener('refresh', onRefresh); + deferred.resolve(); + } + tagsbox.addEventListener('refresh', onRefresh); + return deferred.promise; + } + + describe("#notify()", function () { + it("should update an existing tag on rename", function* () { + var tag = Zotero.Utilities.randomString(); + var newTag = Zotero.Utilities.randomString(); + + var tabbox = doc.getElementById('zotero-view-tabbox'); + tabbox.selectedIndex = 0; + + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: tag + } + ]); + yield item.saveTx(); + + var tabbox = doc.getElementById('zotero-view-tabbox'); + tabbox.selectedIndex = 2; + yield waitForTagsBox(); + var tagsbox = doc.getElementById('zotero-editpane-tags'); + var rows = tagsbox.id('tagRows').getElementsByTagName('row'); + assert.equal(rows.length, 1); + assert.equal(rows[0].textContent, tag); + + yield Zotero.Tags.rename(Zotero.Libraries.userLibraryID, tag, newTag); + + var rows = tagsbox.id('tagRows').getElementsByTagName('row'); + assert.equal(rows.length, 1); + assert.equal(rows[0].textContent, newTag); + }) + + it("should update when a tag's color is removed", function* () { + var libraryID = Zotero.Libraries.userLibraryID; + + var tag = Zotero.Utilities.randomString(); + var tabbox = doc.getElementById('zotero-view-tabbox'); + tabbox.selectedIndex = 0; + + yield Zotero.Tags.setColor(libraryID, tag, "#990000"); + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: tag, + }, + { + tag: "_A" + } + ]); + yield item.saveTx(); + + var tabbox = doc.getElementById('zotero-view-tabbox'); + tabbox.selectedIndex = 2; + yield waitForTagsBox(); + var tagsbox = doc.getElementById('zotero-editpane-tags'); + var rows = tagsbox.id('tagRows').getElementsByTagName('row'); + + // Colored tags aren't sorted first, for now + assert.notOk(rows[0].getElementsByTagName('label')[0].style.color); + assert.ok(rows[1].getElementsByTagName('label')[0].style.color); + assert.equal(rows[0].textContent, "_A"); + assert.equal(rows[1].textContent, tag); + + yield Zotero.Tags.setColor(libraryID, tag, false); + + assert.notOk(rows[1].getElementsByTagName('label')[0].style.color); + }) + + it("should update when a tag is removed from the library", function* () { + var tag = Zotero.Utilities.randomString(); + + var tabbox = doc.getElementById('zotero-view-tabbox'); + tabbox.selectedIndex = 0; + + var item = createUnsavedDataObject('item'); + item.setTags([ + { + tag: tag + } + ]); + yield item.saveTx(); + + var tabbox = doc.getElementById('zotero-view-tabbox'); + tabbox.selectedIndex = 2; + yield waitForTagsBox(); + var tagsbox = doc.getElementById('zotero-editpane-tags'); + var rows = tagsbox.id('tagRows').getElementsByTagName('row'); + assert.equal(rows.length, 1); + assert.equal(rows[0].textContent, tag); + + yield Zotero.Tags.removeFromLibrary( + Zotero.Libraries.userLibraryID, (yield Zotero.Tags.getID(tag)) + ); + + var rows = tagsbox.id('tagRows').getElementsByTagName('row'); + assert.equal(rows.length, 0); + }) + }) +})