From 5a2ec43de1a6e66e00f000baf18ec15f7127bc3e Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 29 May 2015 05:03:05 -0400
Subject: [PATCH] Add Notifier.queue()

Notifier.trigger() needs to be async, since if it actually runs it waits for
promises returned from observers. But the vast majority of trigger() calls are
in transactions where they just queue and can therefore be synchronous. This
replaces all such calls with Notifier.queue().

This should fix a race condition that was causing the emptyTrash() test to fail
intermittently.
---
 chrome/content/zotero/fileInterface.js        |   1 +
 .../content/zotero/xpcom/data/collection.js   |   8 +-
 chrome/content/zotero/xpcom/data/group.js     |   6 +-
 chrome/content/zotero/xpcom/data/item.js      |  28 ++--
 chrome/content/zotero/xpcom/data/items.js     |   4 +-
 chrome/content/zotero/xpcom/data/tags.js      |   6 +-
 chrome/content/zotero/xpcom/fulltext.js       |   4 +-
 chrome/content/zotero/xpcom/notifier.js       | 146 +++++++++++-------
 chrome/content/zotero/xpcom/search.js         |   6 +-
 chrome/content/zotero/xpcom/storage.js        |   1 +
 chrome/content/zotero/xpcom/syncedSettings.js |   4 +-
 11 files changed, 126 insertions(+), 88 deletions(-)

diff --git a/chrome/content/zotero/fileInterface.js b/chrome/content/zotero/fileInterface.js
index fd06984f2..6a64ca064 100644
--- a/chrome/content/zotero/fileInterface.js
+++ b/chrome/content/zotero/fileInterface.js
@@ -341,6 +341,7 @@ var Zotero_File_Interface = new function() {
 				Zotero.UnresponsiveScriptIndicator.enable();
 				
 				if(importCollection) {
+					// TODO: yield or change to .queue()
 					Zotero.Notifier.trigger('refresh', 'collection', importCollection.id);
 				}
 				if (!worked) {
diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js
index 7d3aafe61..82a07bbd7 100644
--- a/chrome/content/zotero/xpcom/data/collection.js
+++ b/chrome/content/zotero/xpcom/data/collection.js
@@ -292,7 +292,7 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 			));
 		}
 		if (!isNew) {
-			Zotero.Notifier.trigger('move', 'collection', this.id);
+			Zotero.Notifier.queue('move', 'collection', this.id);
 		}
 		env.parentIDs = parentIDs;
 	}
@@ -307,10 +307,10 @@ Zotero.Collection.prototype._finalizeSave = Zotero.Promise.coroutine(function* (
 	
 	if (!env.options.skipNotifier) {
 		if (env.isNew) {
-			Zotero.Notifier.trigger('add', 'collection', this.id, env.notifierData);
+			Zotero.Notifier.queue('add', 'collection', this.id, env.notifierData);
 		}
 		else  {
-			Zotero.Notifier.trigger('modify', 'collection', this.id, env.notifierData);
+			Zotero.Notifier.queue('modify', 'collection', this.id, env.notifierData);
 		}
 	}
 	
@@ -611,7 +611,7 @@ Zotero.Collection.prototype._eraseData = Zotero.Promise.coroutine(function* (env
 	//return Zotero.Collections.reloadAll();
 	
 	if (!env.skipNotifier) {
-		Zotero.Notifier.trigger('delete', 'collection', collections, notifierData);
+		Zotero.Notifier.queue('delete', 'collection', collections, notifierData);
 	}
 });
 
diff --git a/chrome/content/zotero/xpcom/data/group.js b/chrome/content/zotero/xpcom/data/group.js
index 166cfdc9d..1c5fc13eb 100644
--- a/chrome/content/zotero/xpcom/data/group.js
+++ b/chrome/content/zotero/xpcom/data/group.js
@@ -245,10 +245,10 @@ Zotero.Group.prototype.save = Zotero.Promise.coroutine(function* () {
 				yield this.load();
 				Zotero.Groups.register(this)
 			}.bind(this)));
-			Zotero.Notifier.trigger('add', 'group', this.id);
+			Zotero.Notifier.queue('add', 'group', this.id);
 		}
 		else {
-			Zotero.Notifier.trigger('modify', 'group', this.id);
+			Zotero.Notifier.queue('modify', 'group', this.id);
 		}
 	}.bind(this));
 });
@@ -293,7 +293,7 @@ Zotero.Group.prototype.erase = Zotero.Promise.coroutine(function* () {
 		yield Zotero.DB.queryAsync(sql, this.libraryID)
 		
 		Zotero.Groups.unregister(this.id);
-		Zotero.Notifier.trigger('delete', 'group', this.id, notifierData);
+		Zotero.Notifier.queue('delete', 'group', this.id, notifierData);
 	}.bind(this));
 	
 	yield Zotero.purgeDataObjects();
diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 49b243f61..74dc75abe 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -1231,7 +1231,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		}
 		
 		if (!env.options.skipNotifier) {
-			Zotero.Notifier.trigger('add', 'item', itemID, env.notifierData);
+			Zotero.Notifier.queue('add', 'item', itemID, env.notifierData);
 		}
 	}
 	else {
@@ -1240,7 +1240,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		yield Zotero.DB.queryAsync(sql, env.sqlValues);
 		
 		if (!env.options.skipNotifier) {
-			Zotero.Notifier.trigger('modify', 'item', itemID, env.notifierData);
+			Zotero.Notifier.queue('modify', 'item', itemID, env.notifierData);
 		}
 	}
 	
@@ -1349,7 +1349,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			
 			let newParentItemNotifierData = {};
 			//newParentItemNotifierData[newParentItem.id] = {};
-			Zotero.Notifier.trigger('modify', 'item', parentItemID, newParentItemNotifierData);
+			Zotero.Notifier.queue('modify', 'item', parentItemID, newParentItemNotifierData);
 			
 			switch (Zotero.ItemTypes.getName(itemTypeID)) {
 				case 'note':
@@ -1373,7 +1373,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 				
 				let newParentItemNotifierData = {};
 				//newParentItemNotifierData[newParentItem.id] = {};
-				Zotero.Notifier.trigger('modify', 'item', parentItemID, newParentItemNotifierData);
+				Zotero.Notifier.queue('modify', 'item', parentItemID, newParentItemNotifierData);
 			}
 			
 			let oldParentKey = this._previousData.parentKey;
@@ -1383,7 +1383,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 				if (oldParentItemID) {
 					let oldParentItemNotifierData = {};
 					//oldParentItemNotifierData[oldParentItemID] = {};
-					Zotero.Notifier.trigger('modify', 'item', oldParentItemID, oldParentItemNotifierData);
+					Zotero.Notifier.queue('modify', 'item', oldParentItemID, oldParentItemNotifierData);
 				}
 				else {
 					Zotero.debug("Old source item " + oldParentKey
@@ -1406,7 +1406,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 						yield this.loadCollections();
 						this.removeFromCollection(changedCollections[i]);
 						
-						Zotero.Notifier.trigger(
+						Zotero.Notifier.queue(
 							'remove',
 							'collection-item',
 							changedCollections[i] + '-' + this.id
@@ -1472,9 +1472,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		yield Zotero.DB.queryAsync(sql, itemID);
 		
 		// Refresh trash
-		Zotero.Notifier.trigger('refresh', 'trash', this.libraryID);
+		Zotero.Notifier.queue('refresh', 'trash', this.libraryID);
 		if (this._deleted) {
-			Zotero.Notifier.trigger('trash', 'item', this.id);
+			Zotero.Notifier.queue('trash', 'item', this.id);
 		}
 		
 		if (parentItemID) {
@@ -1594,7 +1594,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			// "OR REPLACE" allows changing type
 			let sql = "INSERT OR REPLACE INTO itemTags (itemID, tagID, type) VALUES (?, ?, ?)";
 			yield Zotero.DB.queryAsync(sql, [this.id, tagID, tag.type ? tag.type : 0]);
-			Zotero.Notifier.trigger('add', 'item-tag', this.id + '-' + tag.tag);
+			Zotero.Notifier.queue('add', 'item-tag', this.id + '-' + tag.tag);
 		}
 		
 		if (toRemove.length) {
@@ -1604,7 +1604,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 				let tagID = Zotero.Tags.getID(this.libraryID, tag.tag);
 				let sql = "DELETE FROM itemTags WHERE itemID=? AND tagID=? AND type=?";
 				yield Zotero.DB.queryAsync(sql, [this.id, tagID, tag.type ? tag.type : 0]);
-				Zotero.Notifier.trigger('remove', 'item-tag', this.id + '-' + tag.tag);
+				Zotero.Notifier.queue('remove', 'item-tag', this.id + '-' + tag.tag);
 			}
 			Zotero.Prefs.set('purge.tags', true);
 		}
@@ -1634,7 +1634,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			yield Zotero.DB.queryAsync(sql, [collectionID, this.id, orderIndex]);
 			
 			yield this.ContainerObjectsClass.refreshChildItems(collectionID);
-			Zotero.Notifier.trigger('add', 'collection-item', collectionID + '-' + this.id);
+			Zotero.Notifier.queue('add', 'collection-item', collectionID + '-' + this.id);
 		}
 		
 		if (toRemove.length) {
@@ -1646,7 +1646,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			for (let i=0; i<toRemove.length; i++) {
 				let collectionID = toRemove[i];
 				yield this.ContainerObjectsClass.refreshChildItems(collectionID);
-				Zotero.Notifier.trigger('remove', 'collection-item', collectionID + '-' + this.id);
+				Zotero.Notifier.queue('remove', 'collection-item', collectionID + '-' + this.id);
 			}
 		}
 	}
@@ -1714,7 +1714,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			}
 		}
 		
-		Zotero.Notifier.trigger('modify', 'item', removed.concat(newids));
+		Zotero.Notifier.queue('modify', 'item', removed.concat(newids));
 	}
 	
 	// Update child item counts and contents
@@ -3977,7 +3977,7 @@ Zotero.Item.prototype._erasePreCommit = Zotero.Promise.coroutine(function* (env)
 	this.ObjectsClass.unload(this.id);
 	
 	if (!env.skipNotifier) {
-		Zotero.Notifier.trigger('delete', 'item', this.id, env.deletedItemNotifierData);
+		Zotero.Notifier.queue('delete', 'item', this.id, env.deletedItemNotifierData);
 	}
 	
 	Zotero.Prefs.set('purge.items', true);
diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js
index 105a96d99..2a7632e57 100644
--- a/chrome/content/zotero/xpcom/data/items.js
+++ b/chrome/content/zotero/xpcom/data/items.js
@@ -485,7 +485,7 @@ Zotero.Items = function() {
 				let item = yield this.getAsync(id);
 				if (!item) {
 					Zotero.debug('Item ' + id + ' does not exist in Items.trash()!', 1);
-					Zotero.Notifier.trigger('delete', 'item', id);
+					Zotero.Notifier.queue('delete', 'item', id);
 					continue;
 				}
 				item.deleted = true;
@@ -515,7 +515,7 @@ Zotero.Items = function() {
 		if (deletedIDs.length) {
 			yield Zotero.Utilities.Internal.forEachChunkAsync(deletedIDs, 50, function* (chunk) {
 				yield this.erase(chunk);
-				Zotero.Notifier.trigger('refresh', 'trash', libraryID);
+				yield Zotero.Notifier.trigger('refresh', 'trash', libraryID);
 			}.bind(this));
 		}
 		
diff --git a/chrome/content/zotero/xpcom/data/tags.js b/chrome/content/zotero/xpcom/data/tags.js
index 611be8d7b..072d34366 100644
--- a/chrome/content/zotero/xpcom/data/tags.js
+++ b/chrome/content/zotero/xpcom/data/tags.js
@@ -274,7 +274,7 @@ Zotero.Tags = new function() {
 					tag: oldName
 				}
 			};
-			Zotero.Notifier.trigger(
+			Zotero.Notifier.queue(
 				'modify',
 				'item-tag',
 				oldItemIDs.map(function (itemID) itemID + '-' + newName),
@@ -425,7 +425,7 @@ Zotero.Tags = new function() {
 		sql = "DROP TABLE tagDelete";
 		yield Zotero.DB.queryAsync(sql);
 		
-		Zotero.Notifier.trigger('delete', 'tag', toDelete, notifierData);
+		Zotero.Notifier.queue('delete', 'tag', toDelete, notifierData);
 		
 		Zotero.Prefs.set('purge.tags', false);
 	});
@@ -610,7 +610,7 @@ Zotero.Tags = new function() {
 			};
 			
 			if (affectedItems.length) {
-				Zotero.Notifier.trigger('redraw', 'item', affectedItems, { column: 'title' });
+				yield Zotero.Notifier.trigger('redraw', 'item', affectedItems, { column: 'title' });
 			}
 		}
 	});
diff --git a/chrome/content/zotero/xpcom/fulltext.js b/chrome/content/zotero/xpcom/fulltext.js
index df1c01a2a..d6ddb5e75 100644
--- a/chrome/content/zotero/xpcom/fulltext.js
+++ b/chrome/content/zotero/xpcom/fulltext.js
@@ -424,6 +424,8 @@ Zotero.Fulltext = new function(){
 			var sql = "REPLACE INTO fulltextContent (itemID, textContent) VALUES (?,?)";
 			Zotero.DB.query(sql, [itemID, {string:text}]);
 			*/
+			
+			Zotero.Notifier.queue('refresh', 'item', itemID);
 		}.bind(this));
 		
 		// If there's a processor cache file, delete it (whether or not we just used it)
@@ -432,8 +434,6 @@ Zotero.Fulltext = new function(){
 		if (cacheFile.exists()) {
 			cacheFile.remove(false);
 		}
-		
-		Zotero.Notifier.trigger('refresh', 'item', itemID);
 	}.bind(this));
 	
 	
diff --git a/chrome/content/zotero/xpcom/notifier.js b/chrome/content/zotero/xpcom/notifier.js
index d5179b53a..2fcc9ce9e 100644
--- a/chrome/content/zotero/xpcom/notifier.js
+++ b/chrome/content/zotero/xpcom/notifier.js
@@ -88,6 +88,7 @@ Zotero.Notifier = new function(){
 		delete _observers[id];
 	}
 	
+	
 	/**
 	* Trigger a notification to the appropriate observers
 	*
@@ -106,71 +107,23 @@ Zotero.Notifier = new function(){
 	* - New events and types should be added to the order arrays in commit()
 	**/
 	this.trigger = Zotero.Promise.coroutine(function* (event, type, ids, extraData, force) {
+		if (_inTransaction && !force) {
+			return this.queue(event, type, ids, extraData);
+		}
+		
 		if (_disabled){
 			Zotero.debug("Notifications are disabled");
 			return false;
 		}
 		
-		if (_types && _types.indexOf(type) == -1){
-			throw ('Invalid type ' + type + ' in Notifier.trigger()');
+		if (_types && _types.indexOf(type) == -1) {
+			throw new Error("Invalid type '" + type + "'");
 		}
 		
 		ids = Zotero.flattenArguments(ids);
 		
-		var queue = _inTransaction && !force;
-		
 		if (Zotero.Debug.enabled) {
-			Zotero.debug("Notifier.trigger("
-				+ "'" + event + "', "
-				+ "'" + type + "', "
-				+ "[" + ids.join() + "]"
-				+ (extraData ? ", " + extraData : "")
-				+ ")"
-				+ (queue
-					? " queued"
-					: " called " + "[observers: " + _countObserversForType(type) + "]")
-			);
-			if (extraData) {
-				Zotero.debug(extraData);
-			}
-		}
-		
-		// Merge with existing queue
-		if (queue) {
-			if (!_queue[type]) {
-				_queue[type] = [];
-			}
-			if (!_queue[type][event]) {
-				_queue[type][event] = {};
-			}
-			if (!_queue[type][event].ids) {
-				_queue[type][event].ids = [];
-				_queue[type][event].data = {};
-			}
-			
-			// Merge ids
-			_queue[type][event].ids = _queue[type][event].ids.concat(ids);
-			
-			// Merge extraData keys
-			if (extraData) {
-				// If just a single id, extra data can be keyed by id or passed directly
-				if (ids.length == 1) {
-					let id = ids[0];
-					_queue[type][event].data[id] = extraData[id] ? extraData[id] : extraData;
-				}
-				// For multiple ids, check for data keyed by the id
-				else {
-					for (let i = 0; i < ids.length; i++) {
-						let id = ids[i];
-						if (extraData[id]) {
-							_queue[type][event].data[id] = extraData[id];
-						}
-					}
-				}
-			}
-			Zotero.debug(_queue[type][event]);
-			
-			return true;
+			_logTrigger(event, type, ids, extraData);
 		}
 		
 		var order = _getObserverOrder(type);
@@ -198,6 +151,89 @@ Zotero.Notifier = new function(){
 	});
 	
 	
+	/**
+	 * Queue an event until the end of the current notifier transaction
+	 *
+	 * Takes the same parameters as trigger()
+	 *
+	 * @throws If a notifier transaction isn't currently open
+	 */
+	this.queue = function (event, type, ids, extraData) {
+		if (_disabled){
+			Zotero.debug("Notifications are disabled");
+			return false;
+		}
+		
+		if (_types && _types.indexOf(type) == -1) {
+			throw new Error("Invalid type '" + type + "'");
+		}
+		
+		ids = Zotero.flattenArguments(ids);
+		
+		if (Zotero.Debug.enabled) {
+			_logTrigger(event, type, ids, extraData, true);
+		}
+		
+		if (!_inTransaction) {
+			throw new Error("Can't queue event outside of a transaction");
+		}
+		
+		// Merge with existing queue
+		if (!_queue[type]) {
+			_queue[type] = [];
+		}
+		if (!_queue[type][event]) {
+			_queue[type][event] = {};
+		}
+		if (!_queue[type][event].ids) {
+			_queue[type][event].ids = [];
+			_queue[type][event].data = {};
+		}
+		
+		// Merge ids
+		_queue[type][event].ids = _queue[type][event].ids.concat(ids);
+		
+		// Merge extraData keys
+		if (extraData) {
+			// If just a single id, extra data can be keyed by id or passed directly
+			if (ids.length == 1) {
+				let id = ids[0];
+				_queue[type][event].data[id] = extraData[id] ? extraData[id] : extraData;
+			}
+			// For multiple ids, check for data keyed by the id
+			else {
+				for (let i = 0; i < ids.length; i++) {
+					let id = ids[i];
+					if (extraData[id]) {
+						_queue[type][event].data[id] = extraData[id];
+					}
+				}
+			}
+		}
+		Zotero.debug(_queue[type][event]);
+		
+		return true;
+	}
+	
+	
+	function _logTrigger(event, type, ids, extraData, queueing) {
+		Zotero.debug("Notifier.trigger("
+			+ "'" + event + "', "
+			+ "'" + type + "', "
+			+ "[" + ids.join() + "]"
+			+ (extraData ? ", " + extraData : "")
+			+ ")"
+			+ (queueing
+				? " queued "
+				: " called "
+			+ "[observers: " + _countObserversForType(type) + "]")
+		);
+		if (extraData) {
+			Zotero.debug(extraData);
+		}
+	}
+	
+	
 	/**
 	 * Get order of observer by priority, with lower numbers having higher priority.
 	 * If an observer doesn't have a priority, sort it last.
diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js
index 2b5e4a3f8..d19b8ae2a 100644
--- a/chrome/content/zotero/xpcom/search.js
+++ b/chrome/content/zotero/xpcom/search.js
@@ -172,10 +172,10 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 
 Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
 	if (env.isNew) {
-		Zotero.Notifier.trigger('add', 'search', this.id, env.notifierData);
+		Zotero.Notifier.queue('add', 'search', this.id, env.notifierData);
 	}
 	else if (!env.options.skipNotifier) {
-		Zotero.Notifier.trigger('modify', 'search', this.id, env.notifierData);
+		Zotero.Notifier.queue('modify', 'search', this.id, env.notifierData);
 	}
 	
 	if (env.isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) {
@@ -233,7 +233,7 @@ Zotero.Search.prototype._eraseData = Zotero.Promise.coroutine(function* (env) {
 	yield Zotero.DB.queryAsync(sql, this.id);
 	
 	if (!env.skipNotifier) {
-		Zotero.Notifier.trigger('delete', 'search', this.id, notifierData);
+		Zotero.Notifier.queue('delete', 'search', this.id, notifierData);
 	}
 });
 
diff --git a/chrome/content/zotero/xpcom/storage.js b/chrome/content/zotero/xpcom/storage.js
index a12b0cfcc..fa401904c 100644
--- a/chrome/content/zotero/xpcom/storage.js
+++ b/chrome/content/zotero/xpcom/storage.js
@@ -1220,6 +1220,7 @@ Zotero.Sync.Storage = new function () {
 		var libraryID, key;
 		[libraryID, key] = libraryKey.split("/");
 		var item = Zotero.Items.getByLibraryAndKey(libraryID, key);
+		// TODO: yield or switch to queue
 		Zotero.Notifier.trigger('redraw', 'item', item.id, { column: "hasAttachment" });
 		
 		var parent = item.parentItemKey;
diff --git a/chrome/content/zotero/xpcom/syncedSettings.js b/chrome/content/zotero/xpcom/syncedSettings.js
index 3f41032c8..7d10a042d 100644
--- a/chrome/content/zotero/xpcom/syncedSettings.js
+++ b/chrome/content/zotero/xpcom/syncedSettings.js
@@ -75,7 +75,7 @@ Zotero.SyncedSettings = (function () {
 				var sql = "DELETE FROM syncedSettings WHERE setting=? AND libraryID=?";
 				yield Zotero.DB.queryAsync(sql, [setting, libraryID]);
 				
-				Zotero.Notifier.trigger('delete', 'setting', [id], extraData);
+				yield Zotero.Notifier.trigger('delete', 'setting', [id], extraData);
 				return true;
 			}
 			
@@ -100,7 +100,7 @@ Zotero.SyncedSettings = (function () {
 					+ "(setting, libraryID, value, synced) VALUES (?, ?, ?, ?)";
 				yield Zotero.DB.queryAsync(sql, [setting, libraryID, JSON.stringify(value), synced]);
 			}
-			Zotero.Notifier.trigger(event, 'setting', [id], extraData);
+			yield Zotero.Notifier.trigger(event, 'setting', [id], extraData);
 			return true;
 		})
 	};