From 61cb01b7c20819669a377abab7369dfc98273ffe Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 22 May 2015 19:15:21 -0400 Subject: [PATCH] Collection/item tree selection improvements Wait for the pane's collectionSelected() to finish before returning from collectionTreeView select methods (e.g., selectLibrary()), and wait for previous items view to finish loading before creating a new one in collectionSelected(). This ensures that the items view has been created (though not loaded) before returning from a select. The tree can still get a bit confused switching between collections, but I think we're getting closer to fixing that. Also switch the items tree to use the same pattern. This also fixes dragging items to collections (#731). --- .../zotero/xpcom/collectionTreeView.js | 39 ++- .../content/zotero/xpcom/data/collection.js | 31 ++- chrome/content/zotero/xpcom/itemTreeView.js | 61 ++--- .../content/zotero/xpcom/libraryTreeView.js | 35 ++- chrome/content/zotero/zoteroPane.js | 238 +++++++++--------- test/tests/collectionTreeViewTest.js | 83 ++++-- 6 files changed, 297 insertions(+), 190 deletions(-) diff --git a/chrome/content/zotero/xpcom/collectionTreeView.js b/chrome/content/zotero/xpcom/collectionTreeView.js index 9c19861b45..3674860f82 100644 --- a/chrome/content/zotero/xpcom/collectionTreeView.js +++ b/chrome/content/zotero/xpcom/collectionTreeView.js @@ -71,7 +71,6 @@ Object.defineProperty(Zotero.CollectionTreeView.prototype, "selectedTreeRow", { }); - /* * Called by the tree itself */ @@ -248,6 +247,29 @@ Zotero.CollectionTreeView.prototype.reload = function() }.bind(this)); } + +/** + * Select a row and wait for its items view to be created + * + * Note that this doesn't wait for the items view to be loaded. For that, add a 'load' event + * listener to the items view. + * + * @param {Integer} row + * @return {Promise} + */ +Zotero.CollectionTreeView.prototype.selectWait = Zotero.Promise.method(function (row) { + if (this.selection.selectEventsSuppressed) { + this.selection.select(row); + return; + } + var deferred = Zotero.Promise.defer(); + this.addEventListener('select', () => deferred.resolve()); + this.selection.select(row); + return deferred.promise; +}); + + + /* * Called by Zotero.Notifier on any changes to collections in the data layer */ @@ -389,7 +411,10 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function* } } + var deferred = Zotero.Promise.defer(); + this.addEventListener('select', () => deferred.resolve()); this.selection.selectEventsSuppressed = false; + return deferred.promise; }); /** @@ -823,7 +848,7 @@ Zotero.CollectionTreeView.prototype.selectByID = Zotero.Promise.coroutine(functi switch (type) { case 'L': - this.selectLibrary(id); + yield this.selectLibrary(id); return; case 'C': @@ -841,14 +866,14 @@ Zotero.CollectionTreeView.prototype.selectByID = Zotero.Promise.coroutine(functi } var row = this._rowMap[type + id]; this._treebox.ensureRowIsVisible(row); - this.selection.select(row); + yield this.selectWait(row); }); /** * @param {Integer} libraryID Library to select */ -Zotero.CollectionTreeView.prototype.selectLibrary = function (libraryID) { +Zotero.CollectionTreeView.prototype.selectLibrary = Zotero.Promise.coroutine(function* (libraryID) { if (Zotero.suppressUIUpdates) { Zotero.debug("UI updates suppressed -- not changing library selection"); return false; @@ -857,7 +882,7 @@ Zotero.CollectionTreeView.prototype.selectLibrary = function (libraryID) { // Select local library if (!libraryID) { this._treebox.ensureRowIsVisible(0); - this.selection.select(0); + yield this.selectWait(0); return true; } @@ -874,12 +899,12 @@ Zotero.CollectionTreeView.prototype.selectLibrary = function (libraryID) { var row = this._rowMap['L' + libraryID]; if (row !== undefined) { this._treebox.ensureRowIsVisible(row); - this.selection.select(row); + this.selectWait(row); return true; } return false; -} +}); Zotero.CollectionTreeView.prototype.selectCollection = function (id) { diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index e4827c33aa..310d7a1c01 100644 --- a/chrome/content/zotero/xpcom/data/collection.js +++ b/chrome/content/zotero/xpcom/data/collection.js @@ -360,23 +360,22 @@ Zotero.Collection.prototype.addItems = Zotero.Promise.coroutine(function* (itemI yield this.loadChildItems(); var current = this.getChildItems(true); - return Zotero.DB.executeTransaction(function* () { - for (let i=0; i deferred.resolve()); } this.selection.selectEventsSuppressed = false; if (madeChanges) { Zotero.debug("Yielding for select promise"); // TEMP - yield promise; + return deferred.promise; } }); @@ -1630,6 +1631,12 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i return false; } + var selected = this.getSelectedItems(true); + if (selected.length == 1 && selected[0] == id) { + Zotero.debug("Item " + id + " is already selected"); + return; + } + var row = this._rowMap[id]; // Get the row of the parent, if there is one @@ -1674,17 +1681,20 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i row = this._rowMap[id]; } - // This function calls nsITreeSelection.select(), which triggers the 's 'onselect' - // attribute, which calls ZoteroPane.itemSelected(), which calls ZoteroItemPane.viewItem(), - // which refreshes the itembox. But since the 'onselect' doesn't handle promises, - // itemSelected() isn't waited for and 'yield selectItem(itemID)' continues before the - // itembox has been refreshed. To get around this, we make a promise resolver that's - // triggered by itemSelected() when it's done. - if (!this.selection.selectEventsSuppressed) { - var itemSelectedPromise = this._getItemSelectedPromise(id); + // this.selection.select() triggers the 's 'onselect' attribute, which calls + // ZoteroPane.itemSelected(), which calls ZoteroItemPane.viewItem(), which refreshes the + // itembox. But since the 'onselect' doesn't handle promises, itemSelected() isn't waited for + // here, which means that 'yield selectItem(itemID)' continues before the itembox has been + // refreshed. To get around this, we wait for a select event that's triggered by + // itemSelected() when it's done. + if (this.selection.selectEventsSuppressed) { + this.selection.select(row); + } + else { + var deferred = Zotero.Promise.defer(); + this.addEventListener('select', () => deferred.resolve()); + this.selection.select(row); } - - this.selection.select(row); // If |expand|, open row if container if (expand && this.isContainer(row) && !this.isContainerOpen(row)) { @@ -1692,8 +1702,8 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i } this.selection.select(row); - if (!this.selection.selectEventsSuppressed) { - yield itemSelectedPromise; + if (deferred) { + yield deferred.promise; } // We aim for a row 5 below the target row, since ensureRowIsVisible() does @@ -1718,23 +1728,6 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i }); -Zotero.ItemTreeView.prototype._getItemSelectedPromise = function (itemID) { - if (itemID) { - var selected = this.getSelectedItems(true); - var alreadySelected = selected.length == 1 && selected[0] == itemID; - if (alreadySelected) { - return Zotero.Promise.resolve(); - } - } - return new Zotero.Promise(function () { - this._itemSelectedPromiseResolver = { - resolve: arguments[0], - reject: arguments[1] - }; - }.bind(this)); -} - - /** * Select multiple top-level items * diff --git a/chrome/content/zotero/xpcom/libraryTreeView.js b/chrome/content/zotero/xpcom/libraryTreeView.js index 18bbd906c9..7110f7cc56 100644 --- a/chrome/content/zotero/xpcom/libraryTreeView.js +++ b/chrome/content/zotero/xpcom/libraryTreeView.js @@ -26,10 +26,14 @@ Zotero.LibraryTreeView = function () { this._initialized = false; this._listeners = { - load: [] + load: [], + select: [] }; this._rows = []; this._rowMap = {}; + + this.id = Zotero.Utilities.randomString(); + Zotero.debug("Creating " + this.type + "s view with id " + this.id); }; Zotero.LibraryTreeView.prototype = { @@ -43,10 +47,17 @@ Zotero.LibraryTreeView.prototype = { this._listeners[event].push(listener); } } + else { + if (!this._listeners[event]) { + this._listeners[event] = []; + } + this._listeners[event].push(listener); + } }, _runListeners: Zotero.Promise.coroutine(function* (event) { + if (!this._listeners[event]) return; var listener; while (listener = this._listeners[event].shift()) { yield Zotero.Promise.resolve(listener()); @@ -55,13 +66,33 @@ Zotero.LibraryTreeView.prototype = { /** - * Returns a reference to the tree row at a given row + * Return a reference to the tree row at a given row + * + * @return {Zotero.CollectionTreeRow|Zotero.ItemTreeRow} */ getRow: function(row) { return this._rows[row]; }, + /** + * Return the index of the row with a given ID (e.g., "C123" for collection 123) + * + * @param {String} - Row id + * @return {Integer} + */ + getRowByID: function (id) { + var type = id[0]; + id = ('' + id).substr(1); + return this._rowMap[type + id]; + }, + + + onSelect: function () { + return this._runListeners('select'); + }, + + /** * Add a tree row to the main array, update the row count, tell the treebox that the row * count changed, and update the row map diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 10937ec19e..c860bc9b92 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -1107,110 +1107,127 @@ var ZoteroPane = new function() }); - this.onCollectionSelected = Zotero.Promise.coroutine(function* () { - var collectionTreeRow = this.getCollectionTreeRow(); - if (!collectionTreeRow) { - return; - } - - if (this.itemsView && this.itemsView.collectionTreeRow == collectionTreeRow) { - Zotero.debug("Collection selection hasn't changed"); - return; - } - - if (this.itemsView) { - this.itemsView.unregister(); - document.getElementById('zotero-items-tree').view = this.itemsView = null; - } - - if (this.collectionsView.selection.count != 1) { - return; - } - - // Clear quick search and tag selector when switching views - document.getElementById('zotero-tb-search').value = ""; - - // XBL functions might not yet be available - var tagSelector = document.getElementById('zotero-tag-selector'); - if (tagSelector.clearAll) { - tagSelector.clearAll(); - } - - // Not necessary with seltype="cell", which calls nsITreeView::isSelectable() - /*if (collectionTreeRow.isSeparator()) { - document.getElementById('zotero-items-tree').view = this.itemsView = null; - return; - }*/ - - collectionTreeRow.setSearch(''); - collectionTreeRow.setTags(getTagSelection()); - - // Enable or disable toolbar icons and menu options as necessary - const disableIfNoEdit = [ - "cmd_zotero_newCollection", - "cmd_zotero_newSavedSearch", - "zotero-tb-add", - "cmd_zotero_newItemFromCurrentPage", - "zotero-tb-lookup", - "cmd_zotero_newStandaloneNote", - "zotero-tb-note-add", - "zotero-tb-attachment-add" - ]; - for(var i=0; i