Collection/item tree view updates

- Pass .skipSelect option to data object .save() to prevent new objects
  from being selected
- Fix miscellaneous bugs
- Selection-related tests

chrome/content/zotero/xpcom/collectionTreeView.js

@@ -236,8 +236,7 @@ Zotero.CollectionTreeView.prototype.reload = function()
 /*
  *  Called by Zotero.Notifier on any changes to collections in the data layer
  */
-Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function* (action, type, ids)
+Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function* (action, type, ids, extraData) {
-{
 	if ((!ids || ids.length == 0) && action != 'refresh' && action != 'redraw') {
 		return;
 	}
@@ -346,12 +345,12 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function*
 	else if(action == 'add')
 	{
 		// Multiple adds not currently supported
-		ids = ids[0];
+		let id = ids[0];
 		
 		switch (type)
 		{
 			case 'collection':
-				var collection = yield Zotero.Collections.getAsync(ids);
+				var collection = yield Zotero.Collections.getAsync(id);
 				
 				// Open container if creating subcollection
 				var parentID = collection.parentID;
@@ -367,8 +366,10 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function*
 					break;
 				}
 				let row = this._collectionRowMap[collection.id];
-				this._treebox.ensureRowIsVisible(row);
+				if (!extraData[id] || !extraData[id].skipSelect) {
-				this.selection.select(row);
+					this._treebox.ensureRowIsVisible(row);
+					this.selection.select(row);
+				}
 				break;
 				
 			case 'search':
@@ -377,7 +378,9 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function*
 					this.rememberSelection(savedSelection);
 					break;
 				}
-				this.selection.select(this._rowMap['S' + ids]);
+				if (!extraData[id] || !extraData[id].skipSelect) {
+					this.selection.select(this._rowMap['S' + id]);
+				}
 				break;
 			
 			case 'group':

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

@@ -358,10 +358,10 @@ Zotero.Collection.prototype._finalizeSave = Zotero.Promise.coroutine(function* (
 	}
 	
 	if (isNew) {
-		Zotero.Notifier.trigger('add', 'collection', this.id);
+		Zotero.Notifier.trigger('add', 'collection', this.id, env.notifierData);
 	}
-	else {
+	else if (!env.options.skipNotifier) {
-		Zotero.Notifier.trigger('modify', 'collection', this.id, { changed: this._previousData });
+		Zotero.Notifier.trigger('modify', 'collection', this.id, env.notifierData);
 	}
 	
 	// Invalidate cached child collections

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

@@ -544,6 +544,13 @@ Zotero.DataObject.prototype.save = Zotero.Promise.coroutine(function* (options)
 			throw new Error("_finalizeSave not implement for Zotero." + this._ObjectType);
 		}
 		
+		env.notifierData = {};
+		if (env.options.skipSelect) {
+			env.notifierData.skipSelect = true;
+		}
+		if (!env.isNew) {
+			env.changed = this._previousData;
+		}
 		yield this._saveData(env);
 		yield Zotero.DataObject.prototype._finalizeSave.call(this, env);
 		return this._finalizeSave(env);

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

@@ -1238,14 +1238,18 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			itemID = env.id = insertID;
 		}
 		
-		Zotero.Notifier.trigger('add', 'item', itemID);
+		if (!env.options.skipNotifier) {
+			Zotero.Notifier.trigger('add', 'item', itemID, env.notifierData);
+		}
 	}
 	else {
 		var sql = "UPDATE items SET " + sqlColumns.join("=?, ") + "=? WHERE itemID=?";
 		sqlValues.push(parseInt(itemID));
 		yield Zotero.DB.queryAsync(sql, sqlValues);
 		
-		Zotero.Notifier.trigger('modify', 'item', itemID, { changed: this._previousData });
+		if (!env.options.skipNotifier) {
+			Zotero.Notifier.trigger('modify', 'item', itemID, env.notifierData);
+		}
 	}
 	
 	//
@@ -1411,9 +1415,15 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 							changedCollections[i] + '-' + this.id
 						);
 					}
-					yield parentItem.save({
+					let parentOptions = {
 						skipDateModifiedUpdate: true
-					});
+					};
+					// Apply options (e.g., skipNotifier) from outer save
+					for (let o in env.options) {
+						if (!o.startsWith('skip')) continue;
+						parentOptions[o] = env.options[o];
+					}
+					yield parentItem.save(parentOptions);
 				}
 			}
 			

chrome/content/zotero/xpcom/itemTreeView.js

@@ -242,8 +242,6 @@ Zotero.ItemTreeView.prototype.setTree = Zotero.Promise.coroutine(function* (tree
 		
 		yield this.expandMatchParents();
 		
-		yield this._runListeners('load');
-		this._initialized = true;
 		
 		if (this._ownerDocument.defaultView.ZoteroPane_Local) {
 			this._ownerDocument.defaultView.ZoteroPane_Local.clearItemsPaneMessage();
@@ -258,6 +256,9 @@ Zotero.ItemTreeView.prototype.setTree = Zotero.Promise.coroutine(function* (tree
 		
 		delete this._waitAfter;
 		Zotero.debug("Set tree in "+(Date.now()-start)+" ms");
+		
+		this._initialized = true;
+		yield this._runListeners('load');
 	}
 	catch (e) {
 		Components.utils.reportError(e);
@@ -836,9 +837,11 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio
 				this._refreshItemRowMap();
 			}
 			
-			// Reset to Info tab
+			if (!extraData[singleSelect] || !extraData[singleSelect].skipSelect) {
-			this._ownerDocument.getElementById('zotero-view-tabbox').selectedIndex = 0;
+				// Reset to Info tab
-			yield this.selectItem(singleSelect);
+				this._ownerDocument.getElementById('zotero-view-tabbox').selectedIndex = 0;
+				yield this.selectItem(singleSelect);
+			}
 		}
 		// If single item is selected and was modified
 		else if (action == 'modify' && ids.length == 1 &&
@@ -1585,6 +1588,9 @@ Zotero.ItemTreeView.prototype.sort = Zotero.Promise.coroutine(function* (itemID)
  *  Select an item
  */
 Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (id, expand, noRecurse) {
+	var selected = this.getSelectedItems(true);
+	var alreadySelected = selected.length == 1 && selected[0] == id;
+	
 	// Don't change selection if UI updates are disabled (e.g., during sync)
 	if (Zotero.suppressUIUpdates) {
 		Zotero.debug("Sync is running; not selecting item");
@@ -1656,7 +1662,7 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i
 	// itemSelected() isn't waited for and 'yield selectItem(itemID)' continues before the
 	// itembox has been refreshed. To get around this, we make a promise resolver that's
 	// triggered by itemSelected() when it's done.
-	if (!this.selection.selectEventsSuppressed) {
+	if (!alreadySelected && !this.selection.selectEventsSuppressed) {
 		var itemSelectedPromise = new Zotero.Promise(function () {
 			this._itemSelectedPromiseResolver = {
 				resolve: arguments[0],
@@ -1673,7 +1679,7 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i
 	}
 	this.selection.select(row);
 	
-	if (!this.selection.selectEventsSuppressed) {
+	if (!alreadySelected && !this.selection.selectEventsSuppressed) {
 		yield itemSelectedPromise;
 	}
 	

chrome/content/zotero/xpcom/search.js

@@ -245,10 +245,10 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
 	var isNew = env.isNew;
 	if (isNew) {
-		Zotero.Notifier.trigger('add', 'search', this.id);
+		Zotero.Notifier.trigger('add', 'search', this.id, env.notifierData);
 	}
-	else {
+	else if (!env.options.skipNotifier) {
-		Zotero.Notifier.trigger('modify', 'search', this.id, { changed: this._previousData });
+		Zotero.Notifier.trigger('modify', 'search', this.id, env.notifierData);
 	}
 	
 	if (isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) {

chrome/content/zotero/zoteroPane.js

@@ -1090,6 +1090,7 @@ var ZoteroPane = new function()
 	 */
 	this.setTagScope = Zotero.Promise.coroutine(function* () {
 		var collectionTreeRow = self.getCollectionTreeRow();
+		if (!collectionTreeRow) return;
 		var tagSelector = document.getElementById('zotero-tag-selector');
 		if (!tagSelector.getAttribute('collapsed') ||
 				tagSelector.getAttribute('collapsed') == 'false') {
@@ -1109,6 +1110,8 @@ var ZoteroPane = new function()
 	
 	
 	this.onCollectionSelected = Zotero.Promise.coroutine(function* () {
+		yield Zotero.DB.waitForTransaction();
+		
 		var collectionTreeRow = this.getCollectionTreeRow();
 		
 		if (this.itemsView && this.itemsView.collectionTreeRow == collectionTreeRow) {
@@ -1221,6 +1224,11 @@ var ZoteroPane = new function()
 		return Zotero.spawn(function* () {
 			yield Zotero.DB.waitForTransaction();
 			
+			if (!this.itemsView) {
+				Zotero.debug("Items view not available in itemSelected", 2);
+				return;
+			}
+			
 			// Display restore button if items selected in Trash
 			if (this.itemsView.selection.count) {
 				document.getElementById('zotero-item-restore-button').hidden
@@ -1382,13 +1390,13 @@ var ZoteroPane = new function()
 		}, this)
 		.then(function () {
 			// See note in itemTreeView.js::selectItem()
-			if (this.itemsView._itemSelectedPromiseResolver) {
+			if (this.itemsView && this.itemsView._itemSelectedPromiseResolver) {
 				this.itemsView._itemSelectedPromiseResolver.resolve();
 			}
 		}.bind(this))
 		.catch(function (e) {
 			Zotero.debug(e, 1);
-			if (this.itemsView._itemSelectedPromiseResolver) {
+			if (this.itemsView && this.itemsView._itemSelectedPromiseResolver) {
 				this.itemsView._itemSelectedPromiseResolver.reject(e);
 			}
 			throw e;

test/tests/collectionTreeViewTest.js

@@ -0,0 +1,113 @@
+describe("Zotero.CollectionTreeView", function() {
+	var win, collectionsView;
+	
+	// Load Zotero pane and select library
+	before(function* () {
+		win = yield loadZoteroPane();
+		var zp = win.ZoteroPane;
+		var cv = zp.collectionsView;
+		var resolve1, resolve2;
+		var promise1 = new Zotero.Promise(() => resolve1 = arguments[0]);
+		var promise2 = new Zotero.Promise(() => resolve2 = arguments[0]);
+		cv.addEventListener('load', () => resolve1())
+		yield promise1;
+		cv.selection.select(0);
+		zp.addEventListener('itemsLoaded', () => resolve2());
+		yield promise2;
+		collectionsView = zp.collectionsView;
+	});
+	after(function () {
+		if (win) {
+			win.close();
+		}
+	});
+	
+	// Select library
+	// TODO: Add a selectCollection() function and select a collection instead
+	var resetSelection = Zotero.Promise.coroutine(function* () {
+		yield collectionsView.selectLibrary(Zotero.Libraries.userLibraryID);
+		assert.equal(collectionsView.getSelectedLibraryID(), Zotero.Libraries.userLibraryID);
+	});
+	
+	describe("#notify()", function () {
+		it("should select a new collection", function* () {
+			yield resetSelection();
+			
+			// Create collection
+			var collection = new Zotero.Collection;
+			collection.name = "Select new collection";
+			var id = yield collection.save();
+			
+			// New collection should be selected
+			yield Zotero.Promise.delay(100);
+			var selected = collectionsView.getSelectedCollection(true);
+			assert.equal(selected, id);
+		});
+		
+		it("shouldn't select a new collection if skipNotifier is passed", function* () {
+			yield resetSelection();
+			
+			// Create collection with skipNotifier flag
+			var collection = new Zotero.Collection;
+			collection.name = "No select on skipNotifier";
+			var id = yield collection.save({
+				skipNotifier: true
+			});
+			
+			// Library should still be selected
+			assert.equal(collectionsView.getSelectedLibraryID(), Zotero.Libraries.userLibraryID);
+		});
+		
+		it("shouldn't select a new collection if skipSelect is passed", function* () {
+			yield resetSelection();
+			
+			// Create collection with skipSelect flag
+			var collection = new Zotero.Collection;
+			collection.name = "No select on skipSelect";
+			var id = yield collection.save({
+				skipSelect: true
+			});
+			
+			// Library should still be selected
+			assert.equal(collectionsView.getSelectedLibraryID(), Zotero.Libraries.userLibraryID);
+		});
+		
+		it("shouldn't select a modified collection", function* () {
+			// Create collection
+			var collection = new Zotero.Collection;
+			collection.name = "No select on modify";
+			var id = yield collection.save();
+			collection = yield Zotero.Collections.getAsync(id);
+			yield Zotero.Promise.delay(100);
+			
+			yield resetSelection();
+			
+			collection.name = "No select on modify 2";
+			yield collection.save();
+			
+			// Modified collection should not be selected
+			yield Zotero.Promise.delay(100);
+			assert.equal(collectionsView.getSelectedLibraryID(), Zotero.Libraries.userLibraryID);
+		});
+		
+		it("should reselect a selected modified collection", function* () {
+			// Create collection
+			var collection = new Zotero.Collection;
+			collection.name = "Reselect on modify";
+			var id = yield collection.save();
+			collection = yield Zotero.Collections.getAsync(id);
+			yield Zotero.Promise.delay(100);
+			
+			var selected = collectionsView.getSelectedCollection(true);
+			assert.equal(selected, id);
+			
+			collection.name = "Reselect on modify 2";
+			yield collection.save();
+			
+			// Modified collection should still be selected
+			yield Zotero.Promise.delay(100);
+			selected = collectionsView.getSelectedCollection(true);
+			assert.equal(selected, id);
+		});
+	})
+})

test/tests/itemTreeViewTest.js

@@ -0,0 +1,141 @@
+describe("Zotero.ItemTreeView", function() {
+	var win, itemsView, existingItemID;
+	
+	// Load Zotero pane and select library
+	before(function* () {
+		win = yield loadZoteroPane();
+		var zp = win.ZoteroPane;
+		var cv = zp.collectionsView;
+		var resolve1, resolve2;
+		var promise1 = new Zotero.Promise(() => resolve1 = arguments[0]);
+		var promise2 = new Zotero.Promise(() => resolve2 = arguments[0]);
+		cv.addEventListener('load', () => resolve1())
+		yield promise1;
+		cv.selection.select(0);
+		zp.addEventListener('itemsLoaded', () => resolve2());
+		yield promise2;
+		itemsView = zp.itemsView;
+		
+		var item = new Zotero.Item('book');
+		existingItemID = yield item.save();
+		yield Zotero.Promise.delay(100);
+	});
+	after(function () {
+		if (win) {
+			win.close();
+		}
+	});
+	
+	describe("#selectItem()", function () {
+		/**
+		 * Make sure that selectItem() doesn't hang if the pane's item-select handler is never
+		 * triggered due to the item already being selected
+		 */
+		it("should return if item is already selected", function* () {
+			yield itemsView.selectItem(existingItemID);
+			var selected = itemsView.getSelectedItems();
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0].id, existingItemID);
+			yield itemsView.selectItem(existingItemID);
+			selected = itemsView.getSelectedItems();
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0].id, existingItemID);
+		});
+	})
+	
+	describe("#notify()", function () {
+		it("should select a new item", function* () {
+			itemsView.selection.clearSelection();
+			assert.lengthOf(itemsView.getSelectedItems(), 0);
+			
+			// Create item
+			var item = new Zotero.Item('book');
+			var id = yield item.save();
+			
+			// New item should be selected
+			yield Zotero.Promise.delay(100);
+			var selected = itemsView.getSelectedItems();
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0].id, id);
+		});
+		
+		it("shouldn't select a new item if skipNotifier is passed", function* () {
+			// Select existing item
+			yield itemsView.selectItem(existingItemID);
+			var selected = itemsView.getSelectedItems();
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0].id, existingItemID);
+			
+			// Create item with skipNotifier flag
+			var item = new Zotero.Item('book');
+			var id = yield item.save({
+				skipNotifier: true
+			});
+			
+			// Existing item should still be selected
+			selected = itemsView.getSelectedItems();
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0].id, existingItemID);
+		});
+		
+		it("shouldn't select a new item if skipSelect is passed", function* () {
+			// Select existing item
+			yield itemsView.selectItem(existingItemID);
+			var selected = itemsView.getSelectedItems();
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0].id, existingItemID);
+			
+			// Create item with skipSelect flag
+			var item = new Zotero.Item('book');
+			var id = yield item.save({
+				skipSelect: true
+			});
+			
+			// Existing item should still be selected
+			yield Zotero.Promise.delay(100);
+			selected = itemsView.getSelectedItems(true);
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0], existingItemID);
+		});
+		
+		it("shouldn't select a modified item", function* () {
+			// Create item
+			var item = new Zotero.Item('book');
+			var id = yield item.save();
+			item = yield Zotero.Items.getAsync(id);
+			yield Zotero.Promise.delay(100);
+			
+			itemsView.selection.clearSelection();
+			assert.lengthOf(itemsView.getSelectedItems(), 0);
+			
+			item.setField('title', 'no select on modify');
+			yield item.save();
+			
+			// Modified item should not be selected
+			yield Zotero.Promise.delay(100);
+			assert.lengthOf(itemsView.getSelectedItems(), 0);
+		});
+		
+		it("should reselect a selected modified item", function* () {
+			// Create item
+			var item = new Zotero.Item('book');
+			var id = yield item.save();
+			item = yield Zotero.Items.getAsync(id);
+			yield Zotero.Promise.delay(100);
+			
+			yield itemsView.selectItem(id);
+			var selected = itemsView.getSelectedItems(true);
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0], id);
+			
+			item.setField('title', 'reselect on modify');
+			yield item.save();
+			
+			// Modified item should still be selected
+			yield Zotero.Promise.delay(100);
+			selected = itemsView.getSelectedItems(true);
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0], id);
+		});
+	})
+})