From f1af54236e2817e34159b7e3d717134734b6633c Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 22 Apr 2016 21:14:43 -0400 Subject: [PATCH] Add Zotero.Notifier.Queue to keep event groups separate, and use for sync A queue can be created and passed as an option to data layer methods, which will then queue events on that queue instead of the main internal queue. A queue or an array of queues can then be passed to Zotero.Notifier.commit() to commit those events. Some auxiliary functions don't yet take a queue, so those events will still get run on DB transaction commit. Sync data processing now processes notifier events in batches to reduce repaints, even though individual objects are processed within their own transactions (so that failures don't roll back other objects' data). Also remove some unused notifier code --- .../zotero/xpcom/collectionTreeView.js | 2 +- .../content/zotero/xpcom/data/collection.js | 10 +- .../content/zotero/xpcom/data/dataObject.js | 3 +- chrome/content/zotero/xpcom/data/feed.js | 14 +- chrome/content/zotero/xpcom/data/item.js | 56 ++++- chrome/content/zotero/xpcom/notifier.js | 212 +++++++----------- chrome/content/zotero/xpcom/search.js | 4 +- chrome/content/zotero/xpcom/sync/syncLocal.js | 62 ++--- test/tests/notifierTest.js | 60 +++++ 9 files changed, 241 insertions(+), 182 deletions(-) create mode 100644 test/tests/notifierTest.js diff --git a/chrome/content/zotero/xpcom/collectionTreeView.js b/chrome/content/zotero/xpcom/collectionTreeView.js index 5b112717d..06574410d 100644 --- a/chrome/content/zotero/xpcom/collectionTreeView.js +++ b/chrome/content/zotero/xpcom/collectionTreeView.js @@ -2087,7 +2087,7 @@ Zotero.CollectionTreeView.prototype.drop = Zotero.Promise.coroutine(function* (r var parentCollectionID = false; } - var unlock = Zotero.Notifier.begin(true); + var commitNotifier = Zotero.Notifier.begin(); try { for (var i=0; i parseInt(id)), - env.notifierData + env.notifierData, + env.options.notifierQueue ); } }); diff --git a/chrome/content/zotero/xpcom/data/feed.js b/chrome/content/zotero/xpcom/data/feed.js index c591c46b0..f5f0dc47f 100644 --- a/chrome/content/zotero/xpcom/data/feed.js +++ b/chrome/content/zotero/xpcom/data/feed.js @@ -271,7 +271,7 @@ Zotero.Feed.prototype._saveData = Zotero.Promise.coroutine(function* (env) { + "VALUES (" + Array(params.length).fill('?').join(', ') + ")"; yield Zotero.DB.queryAsync(sql, params); - Zotero.Notifier.queue('add', 'feed', this.libraryID); + Zotero.Notifier.queue('add', 'feed', this.libraryID, env.options.notifierQueue); } else if (changedCols.length) { let sql = "UPDATE feeds SET " + changedCols.map(v => v + '=?').join(', ') @@ -279,7 +279,7 @@ Zotero.Feed.prototype._saveData = Zotero.Promise.coroutine(function* (env) { params.push(this.libraryID); yield Zotero.DB.queryAsync(sql, params); - Zotero.Notifier.queue('modify', 'feed', this.libraryID); + Zotero.Notifier.queue('modify', 'feed', this.libraryID, env.options.notifierQueue); } else { Zotero.debug("Feed data did not change for feed " + this.libraryID, 5); @@ -305,12 +305,12 @@ Zotero.Feed.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) { }); -Zotero.Feed.prototype._finalizeErase = Zotero.Promise.coroutine(function* (){ +Zotero.Feed.prototype._finalizeErase = Zotero.Promise.coroutine(function* (env) { let notifierData = {}; notifierData[this.libraryID] = { libraryID: this.libraryID }; - Zotero.Notifier.trigger('delete', 'feed', this.id, notifierData); + Zotero.Notifier.queue('delete', 'feed', this.id, notifierData, env.options.notifierQueue); Zotero.Feeds.unregister(this.libraryID); let syncedFeeds = Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, 'feeds') || {}; @@ -395,7 +395,7 @@ Zotero.Feed.prototype._updateFeed = Zotero.Promise.coroutine(function* () { } let deferred = Zotero.Promise.defer(); this._updating = deferred.promise; - Zotero.Notifier.trigger('statusChanged', 'feed', this.id); + yield Zotero.Notifier.trigger('statusChanged', 'feed', this.id); this._set('_feedLastCheckError', null); try { @@ -491,7 +491,7 @@ Zotero.Feed.prototype._updateFeed = Zotero.Promise.coroutine(function* () { yield this.updateUnreadCount(); deferred.resolve(); this._updating = false; - Zotero.Notifier.trigger('statusChanged', 'feed', this.id); + yield Zotero.Notifier.trigger('statusChanged', 'feed', this.id); }); Zotero.Feed.prototype.updateFeed = Zotero.Promise.coroutine(function* () { @@ -511,6 +511,6 @@ Zotero.Feed.prototype.updateUnreadCount = Zotero.Promise.coroutine(function* () if (newCount != this._feedUnreadCount) { this._feedUnreadCount = newCount; - Zotero.Notifier.trigger('unreadCountUpdated', 'feed', this.id); + yield Zotero.Notifier.trigger('unreadCountUpdated', 'feed', this.id); } }); diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 9544e3705..038398230 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -1269,7 +1269,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { yield Zotero.DB.queryAsync(sql, env.sqlValues); if (!env.options.skipNotifier) { - Zotero.Notifier.queue('add', 'item', itemID, env.notifierData); + Zotero.Notifier.queue('add', 'item', itemID, env.notifierData, env.options.notifierQueue); } } else { @@ -1278,7 +1278,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { yield Zotero.DB.queryAsync(sql, env.sqlValues); if (!env.options.skipNotifier) { - Zotero.Notifier.queue('modify', 'item', itemID, env.notifierData); + Zotero.Notifier.queue('modify', 'item', itemID, env.notifierData, env.options.notifierQueue); } } @@ -1387,7 +1387,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { let newParentItemNotifierData = {}; //newParentItemNotifierData[newParentItem.id] = {}; - Zotero.Notifier.queue('modify', 'item', parentItemID, newParentItemNotifierData); + Zotero.Notifier.queue( + 'modify', 'item', parentItemID, newParentItemNotifierData, env.options.notifierQueue + ); switch (Zotero.ItemTypes.getName(itemTypeID)) { case 'note': @@ -1411,7 +1413,13 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { let newParentItemNotifierData = {}; //newParentItemNotifierData[newParentItem.id] = {}; - Zotero.Notifier.queue('modify', 'item', parentItemID, newParentItemNotifierData); + Zotero.Notifier.queue( + 'modify', + 'item', + parentItemID, + newParentItemNotifierData, + env.options.notifierQueue + ); } let oldParentKey = this._previousData.parentKey; @@ -1421,7 +1429,13 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { if (oldParentItemID) { let oldParentItemNotifierData = {}; //oldParentItemNotifierData[oldParentItemID] = {}; - Zotero.Notifier.queue('modify', 'item', oldParentItemID, oldParentItemNotifierData); + Zotero.Notifier.queue( + 'modify', + 'item', + oldParentItemID, + oldParentItemNotifierData, + env.options.notifierQueue + ); } else { Zotero.debug("Old source item " + oldParentKey @@ -1445,7 +1459,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { Zotero.Notifier.queue( 'remove', 'collection-item', - changedCollections[i] + '-' + this.id + changedCollections[i] + '-' + this.id, + {}, + env.options.notifierQueue ); } let parentOptions = { @@ -1511,9 +1527,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { yield Zotero.DB.queryAsync(sql, itemID); // Refresh trash - Zotero.Notifier.queue('refresh', 'trash', this.libraryID); + Zotero.Notifier.queue('refresh', 'trash', this.libraryID, {}, env.options.notifierQueue); if (this._deleted) { - Zotero.Notifier.queue('trash', 'item', this.id); + Zotero.Notifier.queue('trash', 'item', this.id, {}, env.options.notifierQueue); } if (parentItemID) { @@ -1633,7 +1649,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { tag: tag.tag, type: tagType }; - Zotero.Notifier.queue('add', 'item-tag', this.id + '-' + tagID, notifierData); + Zotero.Notifier.queue( + 'add', 'item-tag', this.id + '-' + tagID, notifierData, env.options.notifierQueue + ); } if (toRemove.length) { @@ -1647,7 +1665,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { libraryID: this.libraryID, tag: tag.tag }; - Zotero.Notifier.queue('remove', 'item-tag', this.id + '-' + tagID, notifierData); + Zotero.Notifier.queue( + 'remove', 'item-tag', this.id + '-' + tagID, notifierData, env.options.notifierQueue + ); } Zotero.Prefs.set('purge.tags', true); } @@ -1680,7 +1700,13 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { + "(collectionID, itemID, orderIndex) VALUES (?, ?, ?)"; yield Zotero.DB.queryAsync(sql, [collectionID, this.id, orderIndex]); - Zotero.Notifier.queue('add', 'collection-item', collectionID + '-' + this.id); + Zotero.Notifier.queue( + 'add', + 'collection-item', + collectionID + '-' + this.id, + {}, + env.options.notifierQueue + ); } // Add this item to any loaded collections' cached item lists after commit @@ -1700,7 +1726,13 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { for (let i=0; i q.id).join(", ") + "]"); + } + else { + Zotero.debug("Committing notifier event queue" + totals); + } - for (var type in runQueue) { - for (var event in runQueue[type]) { + for (let type in runQueue) { + for (let event in runQueue[type]) { if (runQueue[type][event].ids.length || event == 'refresh') { yield this.trigger( event, @@ -420,41 +400,17 @@ Zotero.Notifier = new function(){ /* * Reset the event queue */ - function reset() { + this.reset = function () { //Zotero.debug("Resetting notifier event queue"); - _locked = false; - _queue = []; + _queue = {}; _inTransaction = false; } - - - // - // These should rarely be used now that we have event queuing - // - - /* - * Disables Notifier notifications - * - * Returns false if the Notifier was already disabled, true otherwise - */ - function disable() { - if (_disabled) { - Zotero.debug('Notifier notifications are already disabled'); - return false; - } - Zotero.debug('Disabling Notifier notifications'); - _disabled = true; - return true; - } - - - function enable() { - Zotero.debug('Enabling Notifier notifications'); - _disabled = false; - } - - - function isEnabled() { - return !_disabled; - } } + + +Zotero.Notifier.Queue = function () { + this.id = Zotero.Utilities.randomString(); + Zotero.debug("Creating notifier queue " + this.id); + this._queue = {}; + this.size = 0; +}; diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js index 9fc9695d3..1a37070f3 100644 --- a/chrome/content/zotero/xpcom/search.js +++ b/chrome/content/zotero/xpcom/search.js @@ -210,10 +210,10 @@ Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) // Update library searches status yield Zotero.Libraries.get(this.libraryID).updateSearches(); - Zotero.Notifier.queue('add', 'search', this.id, env.notifierData); + Zotero.Notifier.queue('add', 'search', this.id, env.notifierData, env.options.notifierQueue); } else if (!env.options.skipNotifier) { - Zotero.Notifier.queue('modify', 'search', this.id, env.notifierData); + Zotero.Notifier.queue('modify', 'search', this.id, env.notifierData, env.options.notifierQueue); } if (env.isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) { diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 9370d68e1..60db984dc 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -433,17 +433,15 @@ Zotero.Sync.Data.Local = { } var batchSize = 10; - var batchCounter = 0; + var notifierQueues = []; try { for (let i = 0; i < json.length; i++) { // Batch notifier updates - if (batchCounter == 0) { - Zotero.Notifier.begin(); - } - else if (batchCounter == batchSize || i == json.length - 1) { - Zotero.Notifier.commit(); - Zotero.Notifier.begin(); + if (notifierQueues.length == batchSize) { + yield Zotero.Notifier.commit(notifierQueues); + notifierQueues = []; } + let notifierQueue = new Zotero.Notifier.Queue; let jsonObject = json[i]; let jsonData = jsonObject.data; @@ -454,6 +452,7 @@ Zotero.Sync.Data.Local = { saveOptions.isNewObject = false; saveOptions.skipCache = false; saveOptions.storageDetailsChanged = false; + saveOptions.notifierQueue = notifierQueue; Zotero.debug(`Processing ${objectType} ${libraryID}/${objectKey}`); Zotero.debug(jsonObject); @@ -542,14 +541,14 @@ Zotero.Sync.Data.Local = { obj, jsonObject, { - skipData: true + skipData: true, + notifierQueue } ); results.push(saveResults); if (!saveResults.processed) { throw saveResults.error; } - batchCounter++; return; } @@ -643,8 +642,11 @@ Zotero.Sync.Data.Local = { if (!saveResults.processed) { throw saveResults.error; } - batchCounter++; }.bind(this)); + + if (notifierQueue.size) { + notifierQueues.push(notifierQueue); + } } catch (e) { // Display nicer debug line for known errors @@ -668,20 +670,19 @@ Zotero.Sync.Data.Local = { options.onError(e); } - if (options.stopOnError) { + if (options.stopOnError || e.fatal) { throw e; } } } } - catch (e) { - Zotero.Notifier.reset(); - throw e; - } finally { - Zotero.Notifier.commit(); + if (notifierQueues.length) { + yield Zotero.Notifier.commit(notifierQueues); + } } + // // Conflict resolution // @@ -704,17 +705,15 @@ Zotero.Sync.Data.Local = { Zotero.debug("Processing resolved conflicts"); let batchSize = 50; - let batchCounter = 0; + let notifierQueues = []; try { for (let i = 0; i < mergeData.length; i++) { // Batch notifier updates - if (batchCounter == 0) { - Zotero.Notifier.begin(); - } - else if (batchCounter == batchSize || i == json.length - 1) { - Zotero.Notifier.commit(); - Zotero.Notifier.begin(); + if (notifierQueues.length == batchSize) { + yield Zotero.Notifier.commit(notifierQueues); + notifierQueues = []; } + let notifierQueue = new Zotero.Notifier.Queue; let json = mergeData[i]; @@ -722,6 +721,7 @@ Zotero.Sync.Data.Local = { Object.assign(saveOptions, options); // Tell _saveObjectFromJSON to save as unsynced saveOptions.saveAsChanged = true; + saveOptions.notifierQueue = notifierQueue; // Errors have to be thrown in order to roll back the transaction, so catch // those here and continue @@ -735,7 +735,9 @@ Zotero.Sync.Data.Local = { // Delete local object if (json.deleted) { try { - yield obj.erase(); + yield obj.erase({ + notifierQueue + }); } catch (e) { results.push({ @@ -784,6 +786,10 @@ Zotero.Sync.Data.Local = { } }.bind(this)); + + if (notifierQueue.size) { + notifierQueues.push(notifierQueue); + } } catch (e) { Zotero.logError(e); @@ -798,11 +804,10 @@ Zotero.Sync.Data.Local = { } } } - catch (e) { - Zotero.Notifier.reset(); - } finally { - Zotero.Notifier.commit(); + if (notifierQueues.length) { + yield Zotero.Notifier.commit(notifierQueues); + } } } } @@ -1001,6 +1006,7 @@ Zotero.Sync.Data.Local = { skipDateModifiedUpdate: true, skipSelect: true, skipCache: options.skipCache || false, + notifierQueue: options.notifierQueue, // Errors are logged elsewhere, so skip in DataObject.save() errorHandler: function (e) { return; diff --git a/test/tests/notifierTest.js b/test/tests/notifierTest.js new file mode 100644 index 000000000..1c49f1f4f --- /dev/null +++ b/test/tests/notifierTest.js @@ -0,0 +1,60 @@ +"use strict"; + +describe("Zotero.Notifier", function () { + describe("#trigger()", function () { + it("should trigger add events before modify events", function* () { + var deferred = Zotero.Promise.defer(); + var events = []; + var observer = { + notify: (action, type, ids) => { + events.push(action); + if (events.length == 2) { + deferred.resolve(); + } + } + }; + var id = Zotero.Notifier.registerObserver(observer, null, 'test_trigger'); + + yield Zotero.DB.executeTransaction(function* () { + var item = new Zotero.Item('book'); + item.setField('title', 'A'); + yield item.save(); + item.setField('title', 'B'); + yield item.save(); + + Zotero.Notifier.queue('unknown', 'item', item.id); + }); + + assert.isTrue(deferred.promise.isResolved()); + assert.lengthOf(events, 3); + assert.equal(events[0], 'add'); + assert.equal(events[1], 'modify'); + assert.equal(events[2], 'unknown'); + + Zotero.Notifier.unregisterObserver(id); + }); + + it("should add events to passed queue", function* () { + var collection = yield createDataObject('collection'); + + var deferred = Zotero.Promise.defer(); + var observer = { + notify: () => deferred.resolve() + }; + var id = Zotero.Notifier.registerObserver(observer, null, 'test_trigger'); + + var queue = new Zotero.Notifier.Queue; + var item = createUnsavedDataObject('item'); + item.setCollections([collection.id]); + yield item.saveTx({ + notifierQueue: queue + }); + assert.isTrue(deferred.promise.isPending()); + assert.equal(queue.size, 2); + yield Zotero.Notifier.commit(queue); + assert.isTrue(deferred.promise.isResolved()); + + Zotero.Notifier.unregisterObserver(id); + }); + }); +});