Fix updating of cached child collections/items, and make more efficient

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,
+ * @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 = [];
+		// Add this item to the parent's cached item lists after commit,
-		if (this.id && this._previousData.parentKey) {
+		// if the parent was loaded
-			parentIDs.push(this.ObjectsClass.getIDFromLibraryAndKey(
-				this.libraryID, this._previousData.parentKey
-			));
-		}
 		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
+ * Add a collection to the cached child collections list if loaded
- *
- * Note: This is called by Zotero.Collections.refreshChildCollections()
- *
- * @private
- * @return {Promise}
  */
-Zotero.Collection.prototype._refreshChildCollections = Zotero.Promise.coroutine(function* () {
+Zotero.Collection.prototype._registerChildCollection = function (collectionID) {
-	yield this.reloadHasChildCollections();
 	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
+ * Remove a collection from the cached child collections list if loaded
- *
- * Note: This is called by Zotero.Collections.refreshChildItems()
- *
- * @private
  */
-Zotero.Collection.prototype._refreshChildItems = Zotero.Promise.coroutine(function* () {
+Zotero.Collection.prototype._unregisterChildCollection = function (collectionID) {
-	yield this.reloadHasChildItems();
+	if (this._loaded.childCollections) {
-	if (this._loaded.childItems) {
+		for (let i = 0; i < this._childCollections.length; i++) {
-		return this.loadChildItems(true);
+			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;
+	}
+}

chrome/content/zotero/xpcom/data/collections.js

@@ -155,38 +155,32 @@ Zotero.Collections = function() {
 	}
 	
 	
-	/**
+	this.registerChildCollection = function (collectionID, childCollectionID) {
-	 * Invalidate child collection cache in specified collections, skipping any that aren't loaded
+		if (this._objectCache[collectionID]) {
-	 *
+			this._objectCache[collectionID]._registerChildCollection(childCollectionID);
-	 * @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.unregisterChildCollection = function (collectionID, childCollectionID) {
-	 * Invalidate child item cache in specified collections, skipping any that aren't loaded
+		if (this._objectCache[collectionID]) {
-	 *
+			this._objectCache[collectionID]._unregisterChildCollection(childCollectionID);
-	 * @param	{Integer|Integer[]}	ids		One or more itemIDs
+		}
-	 */
+	}
-	this.refreshChildItems = Zotero.Promise.coroutine(function* (ids) {
+	
-		ids = Zotero.flattenArguments(ids);
+	
-		
+	this.registerChildItem = function (collectionID, itemID) {
-		for (let i=0; i<ids.length; i++) {
+		if (this._objectCache[collectionID]) {
-			let id = ids[i];
+			this._objectCache[collectionID]._registerChildItem(itemID);
-			if (this._objectCache[id]) {
+		}
-				yield this._objectCache[id]._refreshChildItems();
+	}
+	
+	
+	this.unregisterChildItem = function (collectionID, itemID) {
+		if (this._objectCache[collectionID]) {
+			this._objectCache[collectionID]._unregisterChildItem(itemID);
 		}
 	}
-	});
 	
 	
 	this.erase = function(ids) {

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,6 +1622,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		let toAdd = Zotero.Utilities.arrayDiff(newCollections, oldCollections);
 		let toRemove = Zotero.Utilities.arrayDiff(oldCollections, newCollections);
 		
+		if (toAdd.length) {
 			for (let i=0; i<toAdd.length; i++) {
 				let collectionID = toAdd[i];
 				
@@ -1633,10 +1634,17 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 					+ "(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) {
 			let sql = "DELETE FROM collectionItems WHERE itemID=? AND collectionID IN ("
 				+ toRemove.join(',')
@@ -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));
 		}
 	}
 	

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);
+		})
+	})
 })