From ed1c0a4637c00c2681b6edbf981f152a696d737e Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Sat, 30 May 2015 18:35:43 -0400
Subject: [PATCH] Fix updating of cached child collections/items, and make more
 efficient

---
 .../content/zotero/xpcom/data/collection.js   | 109 ++++++++++++------
 .../content/zotero/xpcom/data/collections.js  |  50 ++++----
 chrome/content/zotero/xpcom/data/item.js      |  43 ++++---
 test/tests/collectionTest.js                  |  61 ++++++++++
 4 files changed, 183 insertions(+), 80 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js
index 82a07bbd7..a6d9160f2 100644
--- a/chrome/content/zotero/xpcom/data/collection.js
+++ b/chrome/content/zotero/xpcom/data/collection.js
@@ -173,14 +173,14 @@ Zotero.Collection.prototype.getChildCollections = function (asIDs) {
  *
  * @param	{Boolean}	asIDs			Return as itemIDs
  * @param	{Boolean}	includeDeleted	Include items in Trash
- * @return	{Zotero.Item[]|Integer[]|FALSE}		Array of Zotero.Item instances or itemIDs,
- *												or FALSE if none
+ * @return {Zotero.Item[]|Integer[]} - Array of Zotero.Item instances or itemIDs,
+ *                                     or FALSE if none
  */
 Zotero.Collection.prototype.getChildItems = function (asIDs, includeDeleted) {
 	this._requireData('childItems');
 	
 	if (this._childItems.length == 0) {
-		return false;
+		return [];
 	}
 	
 	// Remove deleted items if necessary
@@ -280,21 +280,30 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 	}
 	
 	if (this._changed.parentKey) {
-		var parentIDs = [];
-		if (this.id && this._previousData.parentKey) {
-			parentIDs.push(this.ObjectsClass.getIDFromLibraryAndKey(
-				this.libraryID, this._previousData.parentKey
-			));
-		}
+		// Add this item to the parent's cached item lists after commit,
+		// if the parent was loaded
 		if (this.parentKey) {
-			parentIDs.push(this.ObjectsClass.getIDFromLibraryAndKey(
+			let parentCollectionID = this.ObjectsClass.getIDFromLibraryAndKey(
 				this.libraryID, this.parentKey
-			));
+			);
+			Zotero.DB.addCurrentCallback("commit", function () {
+				this.ObjectsClass.registerChildCollection(parentCollectionID, this.id);
+			}.bind(this));
 		}
+		// Remove this from the previous parent's cached collection lists after commit,
+		// if the parent was loaded
+		else if (!isNew && this._previousData.parentKey) {
+			let parentCollectionID = this.ObjectsClass.getIDFromLibraryAndKey(
+				this.libraryID, this._previousData.parentKey
+			);
+			Zotero.DB.addCurrentCallback("commit", function () {
+				this.ObjectsClass.unregisterChildCollection(parentCollectionID, this.id);
+			}.bind(this));
+		}
+		
 		if (!isNew) {
 			Zotero.Notifier.queue('move', 'collection', this.id);
 		}
-		env.parentIDs = parentIDs;
 	}
 });
 
@@ -314,11 +323,6 @@ Zotero.Collection.prototype._finalizeSave = Zotero.Promise.coroutine(function* (
 		}
 	}
 	
-	// Invalidate cached child collections
-	if (env.parentIDs) {
-		this.ObjectsClass.refreshChildCollections(env.parentIDs);
-	}
-	
 	if (!env.skipCache) {
 		yield this.reload();
 		// If new, there's no other data we don't have, so we can mark everything as loaded
@@ -866,7 +870,7 @@ Zotero.Collection.prototype.loadChildItems = Zotero.Promise.coroutine(function*
 	this._childItems = [];
 	
 	if (ids) {
-		var items = yield this.ChildObjects.getAsync(ids)
+		var items = yield this.ChildObjects.getAsync(ids);
 		if (items) {
 			this._childItems = items;
 		}
@@ -878,31 +882,60 @@ Zotero.Collection.prototype.loadChildItems = Zotero.Promise.coroutine(function*
 
 
 /**
- * Invalidate child collection cache, if collections are loaded
- *
- * Note: This is called by Zotero.Collections.refreshChildCollections()
- *
- * @private
- * @return {Promise}
+ * Add a collection to the cached child collections list if loaded
  */
-Zotero.Collection.prototype._refreshChildCollections = Zotero.Promise.coroutine(function* () {
-	yield this.reloadHasChildCollections();
+Zotero.Collection.prototype._registerChildCollection = function (collectionID) {
 	if (this._loaded.childCollections) {
-		return this.loadChildCollections(true);
+		let collection = this.ObjectsClass.get(collectionID);
+		if (collection) {
+			this._hasChildCollections = true;
+			this._childCollections.push(collection);
+		}
 	}
-});;
+}
 
 
 /**
- * Invalidate child item cache, if items are loaded
- *
- * Note: This is called by Zotero.Collections.refreshChildItems()
- *
- * @private
+ * Remove a collection from the cached child collections list if loaded
  */
-Zotero.Collection.prototype._refreshChildItems = Zotero.Promise.coroutine(function* () {
-	yield this.reloadHasChildItems();
-	if (this._loaded.childItems) {
-		return this.loadChildItems(true);
+Zotero.Collection.prototype._unregisterChildCollection = function (collectionID) {
+	if (this._loaded.childCollections) {
+		for (let i = 0; i < this._childCollections.length; i++) {
+			if (this._childCollections[i].id == collectionID) {
+				this._childCollections.splice(i, 1);
+				break;
+			}
+		}
+		this._hasChildCollections = this._childCollections.length > 0;
 	}
-});
+}
+
+
+/**
+ * Add an item to the cached child items list if loaded
+ */
+Zotero.Collection.prototype._registerChildItem = function (itemID) {
+	if (this._loaded.childItems) {
+		let item = this.ChildObjects.get(itemID);
+		if (item) {
+			this._hasChildItems = true;
+			this._childItems.push(item);
+		}
+	}
+}
+
+
+/**
+ * Remove an item from the cached child items list if loaded
+ */
+Zotero.Collection.prototype._unregisterChildItem = function (itemID) {
+	if (this._loaded.childItems) {
+		for (let i = 0; i < this._childItems.length; i++) {
+			if (this._childItems[i].id == itemID) {
+				this._childItems.splice(i, 1);
+				break;
+			}
+		}
+		this._hasChildItems = this._childItems.length > 0;
+	}
+}
diff --git a/chrome/content/zotero/xpcom/data/collections.js b/chrome/content/zotero/xpcom/data/collections.js
index 8f7776bd9..8b3a6a57c 100644
--- a/chrome/content/zotero/xpcom/data/collections.js
+++ b/chrome/content/zotero/xpcom/data/collections.js
@@ -155,38 +155,32 @@ Zotero.Collections = function() {
 	}
 	
 	
-	/**
-	 * Invalidate child collection cache in specified collections, skipping any that aren't loaded
-	 *
-	 * @param	{Integer|Integer[]}	ids		One or more collectionIDs
-	 */
-	this.refreshChildCollections = Zotero.Promise.coroutine(function* (ids) {
-		ids = Zotero.flattenArguments(ids);
-		
-		for (let i=0; i<ids.length; i++) {
-			let id = ids[i];
-			if (this._objectCache[id]) {
-				yield this._objectCache[id]._refreshChildCollections();
-			}
+	this.registerChildCollection = function (collectionID, childCollectionID) {
+		if (this._objectCache[collectionID]) {
+			this._objectCache[collectionID]._registerChildCollection(childCollectionID);
 		}
-	});
+	}
 	
 	
-	/**
-	 * Invalidate child item cache in specified collections, skipping any that aren't loaded
-	 *
-	 * @param	{Integer|Integer[]}	ids		One or more itemIDs
-	 */
-	this.refreshChildItems = Zotero.Promise.coroutine(function* (ids) {
-		ids = Zotero.flattenArguments(ids);
-		
-		for (let i=0; i<ids.length; i++) {
-			let id = ids[i];
-			if (this._objectCache[id]) {
-				yield this._objectCache[id]._refreshChildItems();
-			}
+	this.unregisterChildCollection = function (collectionID, childCollectionID) {
+		if (this._objectCache[collectionID]) {
+			this._objectCache[collectionID]._unregisterChildCollection(childCollectionID);
 		}
-	});
+	}
+	
+	
+	this.registerChildItem = function (collectionID, itemID) {
+		if (this._objectCache[collectionID]) {
+			this._objectCache[collectionID]._registerChildItem(itemID);
+		}
+	}
+	
+	
+	this.unregisterChildItem = function (collectionID, itemID) {
+		if (this._objectCache[collectionID]) {
+			this._objectCache[collectionID]._unregisterChildItem(itemID);
+		}
+	}
 	
 	
 	this.erase = function(ids) {
diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 74dc75abe..e9e261118 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -1399,7 +1399,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 				if (changedCollections.length) {
 					let parentItem = yield this.ObjectsClass.getByLibraryAndKeyAsync(
 						this.libraryID, parentItemKey
-					)
+					);
 					for (let i=0; i<changedCollections.length; i++) {
 						yield parentItem.loadCollections();
 						parentItem.addToCollection(changedCollections[i]);
@@ -1622,19 +1622,27 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		let toAdd = Zotero.Utilities.arrayDiff(newCollections, oldCollections);
 		let toRemove = Zotero.Utilities.arrayDiff(oldCollections, newCollections);
 		
-		for (let i=0; i<toAdd.length; i++) {
-			let collectionID = toAdd[i];
+		if (toAdd.length) {
+			for (let i=0; i<toAdd.length; i++) {
+				let collectionID = toAdd[i];
+				
+				let sql = "SELECT IFNULL(MAX(orderIndex)+1, 0) FROM collectionItems "
+					+ "WHERE collectionID=?";
+				let orderIndex = yield Zotero.DB.valueQueryAsync(sql, collectionID);
+				
+				sql = "INSERT OR IGNORE INTO collectionItems "
+					+ "(collectionID, itemID, orderIndex) VALUES (?, ?, ?)";
+				yield Zotero.DB.queryAsync(sql, [collectionID, this.id, orderIndex]);
+				
+				Zotero.Notifier.queue('add', 'collection-item', collectionID + '-' + this.id);
+			}
 			
-			let sql = "SELECT IFNULL(MAX(orderIndex)+1, 0) FROM collectionItems "
-				+ "WHERE collectionID=?";
-			let orderIndex = yield Zotero.DB.valueQueryAsync(sql, collectionID);
-			
-			sql = "INSERT OR IGNORE INTO collectionItems "
-				+ "(collectionID, itemID, orderIndex) VALUES (?, ?, ?)";
-			yield Zotero.DB.queryAsync(sql, [collectionID, this.id, orderIndex]);
-			
-			yield this.ContainerObjectsClass.refreshChildItems(collectionID);
-			Zotero.Notifier.queue('add', 'collection-item', collectionID + '-' + this.id);
+			// Add this item to any loaded collections' cached item lists after commit
+			Zotero.DB.addCurrentCallback("commit", function () {
+				for (let i = 0; i < toAdd.length; i++) {
+					this.ContainerObjectsClass.registerChildItem(toAdd[i], this.id);
+				}
+			}.bind(this));
 		}
 		
 		if (toRemove.length) {
@@ -1645,9 +1653,16 @@ 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.queue('remove', 'collection-item', collectionID + '-' + this.id);
 			}
+			
+			// Remove this item from any loaded collections' cached item lists after commit
+			Zotero.DB.addCurrentCallback("commit", function () {
+				for (let i = 0; i < toRemove.length; i++) {
+					this.ContainerObjectsClass.unregisterChildItem(toRemove[i], this.id);
+				}
+			}.bind(this));
 		}
 	}
 	
diff --git a/test/tests/collectionTest.js b/test/tests/collectionTest.js
index 2a4847e58..a4ea2cde8 100644
--- a/test/tests/collectionTest.js
+++ b/test/tests/collectionTest.js
@@ -97,4 +97,65 @@ describe("Zotero.Collection", function() {
 			assert.isFalse(ret);
 		});
 	})
+	
+	describe("#getChildCollections()", function () {
+		it("should include child collections", function* () {
+			var collection1 = yield createDataObject('collection');
+			var collection2 = yield createDataObject('collection', { parentID: collection1.id });
+			yield collection1.saveTx();
+			
+			yield collection1.loadChildCollections();
+			var childCollections = collection1.getChildCollections();
+			assert.lengthOf(childCollections, 1);
+			assert.equal(childCollections[0].id, collection2.id);
+		})
+		
+		it("should not include collections that have been removed", function* () {
+			var collection1 = yield createDataObject('collection');
+			var collection2 = yield createDataObject('collection', { parentID: collection1.id });
+			yield collection1.saveTx();
+			
+			yield collection1.loadChildCollections();
+			
+			collection2.parentID = false;
+			yield collection2.save()
+			
+			var childCollections = collection1.getChildCollections();
+			assert.lengthOf(childCollections, 0);
+		})
+	})
+	
+	describe("#getChildItems()", function () {
+		it("should include child items", function* () {
+			var collection = yield createDataObject('collection');
+			var item = createUnsavedDataObject('item');
+			item.addToCollection(collection.key);
+			yield item.saveTx();
+			
+			yield collection.loadChildItems();
+			assert.lengthOf(collection.getChildItems(), 1);
+		})
+		
+		it("should not include items in trash by default", function* () {
+			var collection = yield createDataObject('collection');
+			var item = createUnsavedDataObject('item');
+			item.deleted = true;
+			item.addToCollection(collection.key);
+			yield item.saveTx();
+			
+			yield collection.loadChildItems();
+			assert.lengthOf(collection.getChildItems(), 0);
+		})
+		
+		it("should include items in trash if includeTrashed=true", function* () {
+			var collection = yield createDataObject('collection');
+			var item = createUnsavedDataObject('item');
+			item.deleted = true;
+			item.addToCollection(collection.key);
+			yield item.saveTx();
+			
+			yield collection.loadChildItems();
+			assert.lengthOf(collection.getChildItems(false, true), 1);
+		})
+	})
 })