From 820755e152117cffb9b7202ecbb7f3bb9780f5b9 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 24 Mar 2017 05:18:55 -0400 Subject: [PATCH] Rework libraryTreeView event handling Changes `libraryTreeView::addEventListener('load')` and similar to `libraryTreeView::onLoad.addListener(listener, once)`, etc. `once` is an optional boolean that, when true, causes the listener to fire once and then be removed. This is implicit for 'load'. 'load' maintains its special behavior of running immediately if the treeview has already been loaded. Also adds `waitForLoad()` and `waitForSelect()` functions that return promises on event completion, since most uses of those events were just resolving deferreds. --- .../zotero/integration/addCitationDialog.js | 6 +- chrome/content/zotero/selectItemsDialog.js | 8 +- .../zotero/xpcom/collectionTreeView.js | 24 ++---- chrome/content/zotero/xpcom/itemTreeView.js | 16 ++-- .../content/zotero/xpcom/libraryTreeView.js | 78 ++++++++++++++----- chrome/content/zotero/zoteroPane.js | 37 ++++----- test/content/support.js | 8 +- test/tests/advancedSearchTest.js | 3 +- test/tests/itemTreeViewTest.js | 20 ++--- test/tests/relatedboxTest.js | 4 +- 10 files changed, 105 insertions(+), 99 deletions(-) diff --git a/chrome/content/zotero/integration/addCitationDialog.js b/chrome/content/zotero/integration/addCitationDialog.js index 7ec90e5215..f8394d15b7 100644 --- a/chrome/content/zotero/integration/addCitationDialog.js +++ b/chrome/content/zotero/integration/addCitationDialog.js @@ -661,11 +661,9 @@ var Zotero_Citation_Dialog = new function () { _multipleSourceButton.disabled = false; } } else { - collectionsView.addEventListener('load', Zotero.Promise.coroutine(function* () { + collectionsView.onLoad.addListener(Zotero.Promise.coroutine(function* () { if (itemsView) { - var deferred = Zotero.Promise.defer(); - itemsView.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield itemsView.waitForLoad(); _acceptButton.disabled = !itemsView.getSelectedItems().length; } })); diff --git a/chrome/content/zotero/selectItemsDialog.js b/chrome/content/zotero/selectItemsDialog.js index 6fb94f6d38..72d2e97626 100644 --- a/chrome/content/zotero/selectItemsDialog.js +++ b/chrome/content/zotero/selectItemsDialog.js @@ -49,9 +49,7 @@ var doLoad = Zotero.Promise.coroutine(function* () { collectionsView.hideSources = ['duplicates', 'trash', 'feeds']; document.getElementById('zotero-collections-tree').view = collectionsView; - var deferred = Zotero.Promise.defer(); - collectionsView.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield collectionsView.waitForLoad(); connectionSelectedDeferred = Zotero.Promise.defer(); yield connectionSelectedDeferred.promise; @@ -93,9 +91,7 @@ var onCollectionSelected = Zotero.Promise.coroutine(function* () // Create items list and wait for it to load itemsView = new Zotero.ItemTreeView(collectionTreeRow, (window.arguments[1] ? true : false)); document.getElementById('zotero-items-tree').view = itemsView; - var deferred = Zotero.Promise.defer(); - itemsView.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield itemsView.waitForLoad(); clearItemsPaneMessage(); diff --git a/chrome/content/zotero/xpcom/collectionTreeView.js b/chrome/content/zotero/xpcom/collectionTreeView.js index 0cbd63b904..831d72a01d 100644 --- a/chrome/content/zotero/xpcom/collectionTreeView.js +++ b/chrome/content/zotero/xpcom/collectionTreeView.js @@ -120,7 +120,7 @@ Zotero.CollectionTreeView.prototype.setTree = Zotero.Promise.coroutine(function* } this.selection.selectEventsSuppressed = false; - yield this._runListeners('load'); + yield this.runListeners('load'); this._initialized = true; } catch (e) { @@ -301,10 +301,9 @@ Zotero.CollectionTreeView.prototype.selectWait = Zotero.Promise.method(function if (this.selection.currentIndex == row) { return; }; - var deferred = Zotero.Promise.defer(); - this.addEventListener('select', () => deferred.resolve()); + var promise = this.waitForSelect(); this.selection.select(row); - return deferred.promise; + return promise; }); @@ -516,10 +515,9 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function* this._rememberScrollPosition(scrollPosition); - var deferred = Zotero.Promise.defer(); - this.addEventListener('select', () => deferred.resolve()); + var promise = this.waitForSelect(); this.selection.selectEventsSuppressed = false; - return deferred.promise; + return promise; }); /** @@ -1180,9 +1178,7 @@ Zotero.CollectionTreeView.prototype.selectItem = Zotero.Promise.coroutine(functi return false; } - var deferred = Zotero.Promise.defer(); - this.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield this.waitForLoad(); var currentLibraryID = this.getSelectedLibraryID(); // If in a different library @@ -1198,9 +1194,7 @@ Zotero.CollectionTreeView.prototype.selectItem = Zotero.Promise.coroutine(functi var itemsView = this.selectedTreeRow.itemTreeView; - deferred = Zotero.Promise.defer(); - itemsView.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield itemsView.waitForLoad(); var selected = yield itemsView.selectItem(itemID, expand); if (selected) { @@ -1217,9 +1211,7 @@ Zotero.CollectionTreeView.prototype.selectItem = Zotero.Promise.coroutine(functi } itemsView = this.selectedTreeRow.itemTreeView; - deferred = Zotero.Promise.defer(); - itemsView.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield itemsView.waitForLoad(); return itemsView.selectItem(itemID, expand); }); diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js index da1244d91b..8ac2938a74 100644 --- a/chrome/content/zotero/xpcom/itemTreeView.js +++ b/chrome/content/zotero/xpcom/itemTreeView.js @@ -276,7 +276,7 @@ Zotero.ItemTreeView.prototype.setTree = Zotero.Promise.coroutine(function* (tree Zotero.debug("Set tree for items view " + this.id + " in " + (Date.now() - start) + " ms"); this._initialized = true; - yield this._runListeners('load'); + yield this.runListeners('load'); } catch (e) { Zotero.debug(e, 1); @@ -953,14 +953,14 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio }*/ //this._treebox.endUpdateBatch(); + let selectPromise; if (madeChanges) { - var deferred = Zotero.Promise.defer(); - this.addEventListener('select', () => deferred.resolve()); + selectPromise = this.waitForSelect(); } this.selection.selectEventsSuppressed = false; if (madeChanges) { Zotero.debug("Yielding for select promise"); // TEMP - return deferred.promise; + return selectPromise; } }); @@ -1735,12 +1735,12 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i // 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. + let promise; if (this.selection.selectEventsSuppressed) { this.selection.select(row); } else { - var deferred = Zotero.Promise.defer(); - this.addEventListener('select', () => deferred.resolve()); + promise = this.waitForSelect(); this.selection.select(row); } @@ -1750,8 +1750,8 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i } this.selection.select(row); - if (deferred) { - yield deferred.promise; + if (promise) { + yield promise; } this.betterEnsureRowIsVisible(row, parentRow); diff --git a/chrome/content/zotero/xpcom/libraryTreeView.js b/chrome/content/zotero/xpcom/libraryTreeView.js index 01c50532c1..05b56ad01c 100644 --- a/chrome/content/zotero/xpcom/libraryTreeView.js +++ b/chrome/content/zotero/xpcom/libraryTreeView.js @@ -25,15 +25,28 @@ Zotero.LibraryTreeView = function () { this._initialized = false; - this._listeners = { - load: [], - select: [] - }; + this._listeners = {}; this._rows = []; this._rowMap = {}; this.id = Zotero.Utilities.randomString(); Zotero.debug("Creating " + this.type + "s view with id " + this.id); + + // + // Create .on(Load|Select).addListener() methods + // + var _createEventBinding = function (event, alwaysOnce) { + return alwaysOnce + ? { + addListener: listener => this._addListener(event, listener, true) + } + : { + addListener: (listener, once) => this._addListener(event, listener, once) + }; + }.bind(this); + + this.onLoad = _createEventBinding('load', true); + this.onSelect = _createEventBinding('select'); }; Zotero.LibraryTreeView.prototype = { @@ -41,31 +54,56 @@ Zotero.LibraryTreeView.prototype = { return this._initialized; }, - addEventListener: function(event, listener) { - if (event == 'load') { - // If already initialized run now - if (this._initialized) { - listener.call(this); - } - else { - this._listeners[event].push(listener); + + addEventListener: function (event, listener) { + Zotero.logError("Zotero.LibraryTreeView::addEventListener() is deprecated"); + this.addListener(event, listener); + }, + + + waitForLoad: function () { + return this._waitForEvent('load'); + }, + + + waitForSelect: function () { + return this._waitForEvent('select'); + }, + + + runListeners: Zotero.Promise.coroutine(function* (event) { + //Zotero.debug(`Calling ${event} listeners on ${this.type} tree ${this.id}`); + if (!this._listeners[event]) return; + for (let [listener, once] of this._listeners[event].entries()) { + yield Zotero.Promise.resolve(listener.call(this)); + if (once) { + this._listeners[event].delete(listener); } } + }), + + + _addListener: function(event, listener, once) { + // If already initialized run now + if (event == 'load' && this._initialized) { + listener.call(this); + } else { if (!this._listeners[event]) { - this._listeners[event] = []; + this._listeners[event] = new Map(); } - this._listeners[event].push(listener); + this._listeners[event].set(listener, once); } }, - _runListeners: Zotero.Promise.coroutine(function* (event) { - if (!this._listeners[event]) return; - var listener; - while (listener = this._listeners[event].shift()) { - yield Zotero.Promise.resolve(listener.call(this)); + _waitForEvent: Zotero.Promise.coroutine(function* (event) { + if (event == 'load' && this._initialized) { + return; } + return new Zotero.Promise((resolve, reject) => { + this._addListener(event, () => resolve(), true); + }); }), @@ -141,7 +179,7 @@ Zotero.LibraryTreeView.prototype = { }, - onSelect: function () { + runSelectListeners: function () { return this._runListeners('select'); }, diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 23be338ae0..57c267f3c7 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -402,11 +402,11 @@ var ZoteroPane = new function() // TODO: Remove now that no tab mode? var containerWindow = window; if(containerWindow.zoteroSavedCollectionSelection) { - this.collectionsView.addEventListener('load', Zotero.Promise.coroutine(function* () { + this.collectionsView.onLoad.addListener(Zotero.Promise.coroutine(function* () { yield this.collectionsView.selectByID(containerWindow.zoteroSavedCollectionSelection); if (containerWindow.zoteroSavedItemSelection) { - this.itemsView.addEventListener('load', function () { + this.itemsView.onLoad.addListener(function () { this.itemsView.rememberSelection(containerWindow.zoteroSavedItemSelection); delete containerWindow.zoteroSavedItemSelection; }.bind(this)); @@ -987,8 +987,7 @@ var ZoteroPane = new function() var cv = this.collectionsView; - var deferred = Zotero.Promise.defer(); - cv.addEventListener('select', () => deferred.resolve()); + var promise = cv.waitForSelect(); var selectedRow = cv.selection.currentIndex; yield cv.refresh(); @@ -1004,7 +1003,7 @@ var ZoteroPane = new function() this.collectionsView.selection.selectEventsSuppressed = false; - return deferred.promise; + return promise; }); @@ -1199,11 +1198,10 @@ var ZoteroPane = new function() // Wait for existing items view to finish loading before unloading it // // TODO: Cancel loading - let deferred = Zotero.Promise.defer(); - this.itemsView.addEventListener('load', () => deferred.resolve()); - if (deferred.promise.isPending()) { + let promise = this.itemsView.waitForLoad(); + if (promise.isPending()) { Zotero.debug("Waiting for items view " + this.itemsView.id + " to finish loading"); - yield deferred.promise; + yield promise; } this.itemsView.unregister(); @@ -1240,7 +1238,7 @@ var ZoteroPane = new function() Zotero.Prefs.clear('lastViewedFolder'); ZoteroPane_Local.displayErrorMessage(); }; - this.itemsView.addEventListener('load', () => this.setTagScope()); + this.itemsView.onLoad.addListener(() => this.setTagScope()); if (this.tagSelectorShown()) { let tagSelector = document.getElementById('zotero-tag-selector') let handler = function () { @@ -1250,7 +1248,7 @@ var ZoteroPane = new function() tagSelector.addEventListener('refresh', handler); } else { - this.itemsView.addEventListener('load', () => Zotero.uiIsReady()); + this.itemsView.onLoad.addListener(() => Zotero.uiIsReady()); } // If item data not yet loaded for library, load it now. @@ -1320,7 +1318,7 @@ var ZoteroPane = new function() Zotero.Prefs.set('lastViewedFolder', collectionTreeRow.id); }, this) .finally(function () { - return this.collectionsView.onSelect(); + return this.collectionsView.runListeners('select'); }.bind(this)); }; @@ -1387,12 +1385,9 @@ var ZoteroPane = new function() // Don't select item until items list has loaded // // This avoids an error if New Item is used while the pane is first loading. - var deferred = Zotero.Promise.defer(); - this.itemsView.addEventListener('load', function () { - deferred.resolve(); - }); - if (deferred.promise.isPending()) { - yield deferred.promise; + var promise = this.itemsView.waitForLoad(); + if (promise.isPending()) { + yield promise; } if (!this.itemsView || !this.itemsView.selection) { @@ -1598,7 +1593,7 @@ var ZoteroPane = new function() return true; }.bind(this))() .finally(function () { - return this.itemsView.onSelect(); + return this.itemsView.runListeners('select'); }.bind(this)); } @@ -2462,9 +2457,7 @@ var ZoteroPane = new function() // some menu items (e.g., export/createBib/loadReport) to appear gray in the menu at first and // then turn black once there are items. Pass a flag to prevent an accidental infinite loop. if (!collectionTreeRow.isHeader() && !this.itemsView.initialized && !noRepeat) { - this.itemsView.addEventListener('load', function () { - this.buildCollectionContextMenu(true); - }.bind(this)); + this.itemsView.onLoad.addListener(() => this.buildCollectionContextMenu(true)); } // Set attributes on the menu from the configuration object diff --git a/test/content/support.js b/test/content/support.js index f3fd248230..9f6f5dd408 100644 --- a/test/content/support.js +++ b/test/content/support.js @@ -190,15 +190,11 @@ var waitForItemsLoad = Zotero.Promise.coroutine(function* (win, collectionRowToS var zp = win.ZoteroPane; var cv = zp.collectionsView; - var deferred = Zotero.Promise.defer(); - cv.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield cv.waitForLoad(); if (collectionRowToSelect !== undefined) { yield cv.selectWait(collectionRowToSelect); } - deferred = Zotero.Promise.defer(); - zp.itemsView.addEventListener('load', () => deferred.resolve()); - return deferred.promise; + yield zp.itemsView.waitForLoad(); }); var waitForTagSelector = function (win) { diff --git a/test/tests/advancedSearchTest.js b/test/tests/advancedSearchTest.js index e49eb9ef78..3f2f905a5b 100644 --- a/test/tests/advancedSearchTest.js +++ b/test/tests/advancedSearchTest.js @@ -35,8 +35,7 @@ describe("Advanced Search", function () { var deferred = Zotero.Promise.defer(); o.search(); var iv = o.itemsView; - iv.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield iv.waitForLoad(); // Check results assert.equal(iv.rowCount, 1); diff --git a/test/tests/itemTreeViewTest.js b/test/tests/itemTreeViewTest.js index 2a30097aeb..254835ccd0 100644 --- a/test/tests/itemTreeViewTest.js +++ b/test/tests/itemTreeViewTest.js @@ -566,8 +566,7 @@ describe("Zotero.ItemTreeView", function() { let view = zp.itemsView; yield view.selectItem(item3.id, true); - var deferred = Zotero.Promise.defer(); - view.addEventListener('select', () => deferred.resolve()); + var promise = view.waitForSelect(); view.drop(view.getRowIndexByID(item2.id), 0, { dropEffect: 'copy', @@ -585,7 +584,7 @@ describe("Zotero.ItemTreeView", function() { mozItemCount: 1 }) - yield deferred.promise; + yield promise; // Old parent should be empty assert.isFalse(view.isContainerOpen(view.getRowIndexByID(item1.id))); @@ -606,8 +605,7 @@ describe("Zotero.ItemTreeView", function() { let view = zp.itemsView; yield view.selectItem(item3.id, true); - var deferred = Zotero.Promise.defer(); - view.addEventListener('select', () => deferred.resolve()); + var promise = view.waitForSelect(); view.drop(view.getRowIndexByID(item1.id), 0, { dropEffect: 'copy', @@ -625,7 +623,7 @@ describe("Zotero.ItemTreeView", function() { mozItemCount: 1 }) - yield deferred.promise; + yield promise; // Old parent should be empty assert.isFalse(view.isContainerOpen(view.getRowIndexByID(item2.id))); @@ -640,8 +638,7 @@ describe("Zotero.ItemTreeView", function() { var file = getTestDataDirectory(); file.append('test.png'); - var deferred = Zotero.Promise.defer(); - itemsView.addEventListener('select', () => deferred.resolve()); + var promise = itemsView.waitForSelect(); itemsView.drop(0, -1, { dropEffect: 'copy', @@ -659,7 +656,7 @@ describe("Zotero.ItemTreeView", function() { } }) - yield deferred.promise; + yield promise; var items = itemsView.getSelectedItems(); var path = yield items[0].getFilePathAsync(); assert.equal( @@ -669,8 +666,7 @@ describe("Zotero.ItemTreeView", function() { }); it("should create a top-level attachment when a URL is dragged", function* () { - var deferred = Zotero.Promise.defer(); - itemsView.addEventListener('select', () => deferred.resolve()); + var promise = itemsView.waitForSelect(); itemsView.drop(0, -1, { dropEffect: 'copy', @@ -688,7 +684,7 @@ describe("Zotero.ItemTreeView", function() { mozItemCount: 1, }) - yield deferred.promise; + yield promise; var item = itemsView.getSelectedItems()[0]; assert.equal(item.getField('url'), pdfURL); assert.equal( diff --git a/test/tests/relatedboxTest.js b/test/tests/relatedboxTest.js index 5521ca16e3..7bf1436992 100644 --- a/test/tests/relatedboxTest.js +++ b/test/tests/relatedboxTest.js @@ -35,9 +35,7 @@ describe("Related Box", function () { yield Zotero.Promise.delay(50); } while (!view); - var deferred = Zotero.Promise.defer(); - view.addEventListener('load', () => deferred.resolve()); - yield deferred.promise; + yield view.waitForLoad(); // Select the other item for (let i = 0; i < view.rowCount; i++) {