More data layer changes

- Moved ::_get() and _set() from Collection/Search into DataObject, and
  disabled in Item
- Don't disable new items after save. We now put new objects into the
  DataObjects cache from save() so that changes made post-save are
  picked up by other code using .get().
- Added 'skipCache' save() option to avoid reloading data on new objects
  and adding them to the cache. (This will be used in syncing, where
  objects might be in another library where they're not needed right
  away.) Objects created with this option are instead disabled to
  prevent reuse.
- Modified some tests to try to make sure we're reloading everything properly
  after a save.
- Documented save() options

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

@@ -82,40 +82,6 @@ Zotero.defineProperty(Zotero.Collection.prototype, 'parent', {
 	}
 });
 
-Zotero.Collection.prototype._set = function (field, value) {
-	if (field == 'id' || field == 'libraryID' || field == 'key') {
-		return this._setIdentifier(field, value);
-	}
-	
-	this._requireData('primaryData');
-	
-	switch (field) {
-		case 'name':
-			value = value.trim().normalize();
-			break;
-		
-		case 'version':
-			value = parseInt(value);
-			break;
-		
-		case 'synced':
-			value = !!value;
-			break;
-	}
-	
-	if (this['_' + field] != value) {
-		this._markFieldChange(field, this['_' + field]);
-		if (!this._changed.primaryData) {
-			this._changed.primaryData = {};
-		}
-		this._changed.primaryData[field] = true;
-		
-		switch (field) {
-			default:
-				this['_' + field] = value;
-		}
-	}
-}
 
 Zotero.Collection.prototype.getID = function() {
 	Zotero.debug('Collection.getID() deprecated -- use Collection.id');
@@ -340,15 +306,14 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 });
 
 Zotero.Collection.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
-	var isNew = env.isNew;
+	if (env.isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) {
-	if (isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) {
 		var groupID = Zotero.Groups.getGroupIDFromLibraryID(this.libraryID);
 		var group = Zotero.Groups.get(groupID);
 		group.clearCollectionCache();
 	}
 	
 	if (!env.options.skipNotifier) {
-		if (isNew) {
+		if (env.isNew) {
 			Zotero.Notifier.trigger('add', 'collection', this.id, env.notifierData);
 		}
 		else  {
@@ -361,17 +326,12 @@ Zotero.Collection.prototype._finalizeSave = Zotero.Promise.coroutine(function* (
 		this.ObjectsClass.refreshChildCollections(env.parentIDs);
 	}
 	
-	// New collections have to be reloaded via Zotero.Collections.get(), so mark them as disabled
+	if (!env.skipCache) {
-	if (isNew) {
-		var id = this.id;
-		this._disabled = true;
-		return id;
-	}
-	
 		yield this.reload();
 		this._clearChanged();
+	}
 	
-	return true;
+	return env.isNew ? this.id : true;
 });
 
 

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

@@ -50,6 +50,9 @@ Zotero.DataObject = function () {
 	this._parentID = null;
 	this._parentKey = null;
 	
+	// Set in dataObjects.js
+	this._inCache = false;
+	
 	this._loaded = {};
 	this._skipDataTypeLoad = {};
 	this._markAllDataTypeLoadStates(false);
@@ -90,6 +93,8 @@ Zotero.defineProperty(Zotero.DataObject.prototype, 'ObjectsClass', {
 
 
 Zotero.DataObject.prototype._get = function (field) {
+	if (field != 'id') this._disabledCheck();
+	
 	if (this['_' + field] !== null) {
 		return this['_' + field];
 	}
@@ -98,6 +103,44 @@ Zotero.DataObject.prototype._get = function (field) {
 }
 
 
+Zotero.DataObject.prototype._set = function (field, value) {
+	this._disabledCheck();
+	
+	if (field == 'id' || field == 'libraryID' || field == 'key') {
+		return this._setIdentifier(field, value);
+	}
+	
+	this._requireData('primaryData');
+	
+	switch (field) {
+		case 'name':
+			value = value.trim().normalize();
+			break;
+		
+		case 'version':
+			value = parseInt(value);
+			break;
+		
+		case 'synced':
+			value = !!value;
+			break;
+	}
+	
+	if (this['_' + field] != value) {
+		this._markFieldChange(field, this['_' + field]);
+		if (!this._changed.primaryData) {
+			this._changed.primaryData = {};
+		}
+		this._changed.primaryData[field] = true;
+		
+		switch (field) {
+			default:
+				this['_' + field] = value;
+		}
+	}
+}
+
+
 Zotero.DataObject.prototype._setIdentifier = function (field, value) {
 	// If primary data is loaded, the only allowed identifier change is libraryID
 	// (allowed mainly for the library switcher in the advanced search window),
@@ -544,6 +587,14 @@ Zotero.DataObject.prototype.editCheck = function () {
 /**
  * Save changes to database
  *
+ * @params {Object} [options]
+ * @params {Boolean} [options.skipCache] - Don't save add new object to the cache; if set, object
+ *                                         is disabled after save
+ * @params {Boolean} [options.skipDateModifiedUpdate]
+ * @params {Boolean} [options.skipClientDateModifiedUpdate]
+ * @params {Boolean} [options.skipNotifier] - Don't trigger Zotero.Notifier events
+ * @params {Boolean} [options.skipSelect] - Don't select object automatically in trees
+ * @params {Boolean} [options.skipSyncedUpdate] - Don't automatically set 'synced' to false
  * @return {Promise<Integer|Boolean>}  Promise for itemID of new item,
  *                                     TRUE on item update, or FALSE if item was unchanged
  */
@@ -650,7 +701,7 @@ Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env)
 		return false;
 	}
 	
-	// Undo registerIdentifiers() on failure
+	// Undo registerObject() on failure
 	var func = function () {
 		this.ObjectsClass.unload(env.id);
 	}.bind(this);
@@ -699,9 +750,18 @@ Zotero.DataObject.prototype._saveData = function (env) {
 };
 
 Zotero.DataObject.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
-	// Register this object's identifiers in Zotero.DataObjects
 	if (env.isNew) {
-		this.ObjectsClass.registerIdentifiers(env.id, env.libraryID, env.key);
+		if (!env.skipCache) {
+			// Register this object's identifiers in Zotero.DataObjects
+			this.ObjectsClass.registerObject(this);
+		}
+		// If object isn't being reloaded, disable it, since its data may be out of date
+		else {
+			this._disabled = true;
+		}
+	}
+	else if (env.skipCache) {
+		Zotero.logError("skipCache is only for new objects");
 	}
 });
 
@@ -778,3 +838,10 @@ Zotero.DataObject.prototype._erasePostCommit = function(env) {
 Zotero.DataObject.prototype._generateKey = function () {
 	return Zotero.Utilities.generateObjectKey();
 }
+
+Zotero.DataObject.prototype._disabledCheck = function () {
+	if (this._disabled) {
+		Zotero.logError(this._ObjectType + " is disabled -- "
+			+ "use Zotero." + this._ObjectTypePlural  + ".getAsync()");
+	}
+}

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

@@ -395,13 +395,19 @@ Zotero.DataObjects.prototype.reloadAll = function (libraryID) {
 }
 
 
-Zotero.DataObjects.prototype.registerIdentifiers = function (id, libraryID, key) {
+Zotero.DataObjects.prototype.registerObject = function (obj) {
+	var id = obj.id;
+	var libraryID = obj.libraryID;
+	var key = obj.key;
+	
 	Zotero.debug("Registering " + this._ZDO_object + " " + id + " as " + libraryID + "/" + key);
 	if (!this._objectIDs[libraryID]) {
 		this._objectIDs[libraryID] = {};
 	}
 	this._objectIDs[libraryID][key] = id;
 	this._objectKeys[id] = [libraryID, key];
+	this._objectCache[id] = obj;
+	obj._inCache = true;
 }
 
 
@@ -521,6 +527,7 @@ Zotero.DataObjects.prototype._load = Zotero.Promise.coroutine(function* (library
 					obj.loadFromRow(rowObj, true);
 					if (!options || !options.noCache) {
 						this._objectCache[id] = obj;
+						obj._inCache = true;
 					}
 				}
 				loaded[id] = obj;

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

@@ -178,10 +178,14 @@ Zotero.Item.prototype.isPrimaryField = function (fieldName) {
 	return this.ObjectsClass.isPrimaryField(fieldName);
 }
 
-Zotero.Item.prototype._get = function (fieldName) {
+Zotero.Item.prototype._get = function () {
 	throw new Error("_get is not valid for items");
 }
 
+Zotero.Item.prototype._set = function () {
+	throw new Error("_set is not valid for items");
+}
+
 Zotero.Item.prototype._setParentKey = function() {
 	if (!this.isNote() && !this.isAttachment()) {
 		throw new Error("_setParentKey() can only be called on items of type 'note' or 'attachment'");
@@ -207,11 +211,7 @@ Zotero.Item.prototype._setParentKey = function() {
  * @return {String} Value as string or empty string if value is not present
  */
 Zotero.Item.prototype.getField = function(field, unformatted, includeBaseMapped) {
-	// We don't allow access after saving to force use of the centrally cached
+	if (field != 'id') this._disabledCheck();
-	// object, but we make an exception for the id
-	if (field != 'id') {
-		this._disabledCheck();
-	}
 	
 	//Zotero.debug('Requesting field ' + field + ' for item ' + this._id, 4);
 	
@@ -694,12 +694,12 @@ Zotero.Item.prototype.getFieldsNotInType = function (itemTypeID, allowBaseConver
  * Field can be passed as fieldID or fieldName
  */
 Zotero.Item.prototype.setField = function(field, value, loadIn) {
+	this._disabledCheck();
+	
 	if (typeof value == 'string') {
 		value = value.trim().normalize();
 	}
 	
-	this._disabledCheck();
-	
 	//Zotero.debug("Setting field '" + field + "' to '" + value + "' (loadIn: " + (loadIn ? 'true' : 'false') + ") for item " + this.id + " ");
 	
 	if (!field) {
@@ -1730,21 +1730,16 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 });
 
 Zotero.Item.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
-	// New items have to be reloaded via Zotero.Items.get(), so mark them as disabled
+	if (!env.skipCache) {
-	if (env.isNew) {
-		var id = this.id;
-		this._disabled = true;
-		return id;
-	}
-	
 		// Always reload primary data. DataObject.reload() only reloads changed data types, so
 		// it won't reload, say, dateModified and firstCreator if only creator data was changed
 		// and not primaryData.
 		yield this.loadPrimaryData(true);
 		yield this.reload();
 		this._clearChanged();
+	}
 	
-	return true;
+	return env.isNew ? this.id : true;
 });
 
 /**
@@ -4807,12 +4802,3 @@ Zotero.Item.prototype._getOldCreators = function () {
 	}
 	return oldCreators;
 }
-
-
-Zotero.Item.prototype._disabledCheck = function () {
-	if (this._disabled) {
-		var msg = "New Zotero.Item objects shouldn't be accessed after save -- use Zotero.Items.get()";
-		Zotero.debug(msg, 2, true);
-		Components.utils.reportError(msg);
-	}
-}

chrome/content/zotero/xpcom/search.js

@@ -87,40 +87,6 @@ Zotero.defineProperty(Zotero.Search.prototype, 'conditions', {
 	get: function() this.getConditions()
 });
 
-Zotero.Search.prototype._set = function (field, value) {
-	if (field == 'id' || field == 'libraryID' || field == 'key') {
-		return this._setIdentifier(field, value);
-	}
-	
-	this._requireData('primaryData');
-	
-	switch (field) {
-		case 'name':
-			value = value.trim().normalize();
-			break;
-		
-		case 'version':
-			value = parseInt(value);
-			break;
-		
-		case 'synced':
-			value = !!value;
-			break;
-	}
-	
-	if (this['_' + field] != value) {
-		this._markFieldChange(field, this['_' + field]);
-		if (!this._changed.primaryData) {
-			this._changed.primaryData = {};
-		}
-		this._changed.primaryData[field] = true;
-		
-		switch (field) {
-			default:
-				this['_' + field] = value;
-		}
-	}
-}
 
 Zotero.Search.prototype.loadFromRow = function (row) {
 	this._id = row.savedSearchID;
@@ -205,31 +171,26 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 });
 
 Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
-	var isNew = env.isNew;
+	if (env.isNew) {
-	if (isNew) {
 		Zotero.Notifier.trigger('add', 'search', this.id, env.notifierData);
 	}
 	else if (!env.options.skipNotifier) {
 		Zotero.Notifier.trigger('modify', 'search', this.id, env.notifierData);
 	}
 	
-	if (isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) {
+	if (env.isNew && Zotero.Libraries.isGroupLibrary(this.libraryID)) {
 		var groupID = Zotero.Groups.getGroupIDFromLibraryID(this.libraryID);
 		var group = yield Zotero.Groups.get(groupID);
 		group.clearSearchCache();
 	}
 	
-	if (isNew) {
+	if (!env.skipCache) {
-		var id = this.id;
-		this._disabled = true;
-		return id;
-	}
-	
 		yield this.loadPrimaryData(true);
 		yield this.reload();
 		this._clearChanged();
+	}
 	
-	return isNew ? this.id : true;
+	return env.isNew ? this.id : true;
 });
 
 

test/tests/collectionTest.js

@@ -7,6 +7,7 @@ describe("Zotero.Collection", function() {
 			var collection = new Zotero.Collection;
 			collection.name = name;
 			var id = yield collection.saveTx();
+			assert.equal(collection.name, name);
 			collection = yield Zotero.Collections.getAsync(id);
 			assert.equal(collection.name, name);
 		});
@@ -19,6 +20,7 @@ describe("Zotero.Collection", function() {
 			collection.version = version;
 			collection.name = "Test";
 			var id = yield collection.saveTx();
+			assert.equal(collection.version, version);
 			collection = yield Zotero.Collections.getAsync(id);
 			assert.equal(collection.version, version);
 		});
@@ -35,6 +37,7 @@ describe("Zotero.Collection", function() {
 			col.name = "Child";
 			col.parentKey = parentKey;
 			var id = yield col.saveTx();
+			assert.equal(col.parentKey, parentKey);
 			col = yield Zotero.Collections.getAsync(id);
 			assert.equal(col.parentKey, parentKey);
 		});
@@ -51,7 +54,6 @@ describe("Zotero.Collection", function() {
 			col.name = "Child";
 			col.parentKey = parentKey;
 			var id = yield col.saveTx();
-			col = yield Zotero.Collections.getAsync(id);
 			
 			// Create new parent collection
 			var newParentCol = new Zotero.Collection
@@ -62,6 +64,7 @@ describe("Zotero.Collection", function() {
 			// Change parent collection
 			col.parentKey = newParentKey;
 			yield col.saveTx();
+			assert.equal(col.parentKey, newParentKey);
 			col = yield Zotero.Collections.getAsync(id);
 			assert.equal(col.parentKey, newParentKey);
 		});
@@ -78,7 +81,6 @@ describe("Zotero.Collection", function() {
 			col.name = "Child";
 			col.parentKey = parentKey;
 			var id = yield col.saveTx();
-			col = yield Zotero.Collections.getAsync(id);
 			
 			// Set to existing parent
 			col.parentKey = parentKey;
@@ -89,7 +91,6 @@ describe("Zotero.Collection", function() {
 			var col = new Zotero.Collection
 			col.name = "Test";
 			var id = yield col.saveTx();
-			col = yield Zotero.Collections.getAsync(id);
 			
 			col.parentKey = false;
 			var ret = yield col.saveTx();

test/tests/collectionTreeViewTest.js

@@ -66,7 +66,6 @@ describe("Zotero.CollectionTreeView", function() {
 			var collection = new Zotero.Collection;
 			collection.name = "No select on modify";
 			var id = yield collection.saveTx();
-			collection = yield Zotero.Collections.getAsync(id);
 			
 			resetSelection();
 			
@@ -82,7 +81,6 @@ describe("Zotero.CollectionTreeView", function() {
 			var collection = new Zotero.Collection;
 			collection.name = "Reselect on modify";
 			var id = yield collection.saveTx();
-			collection = yield Zotero.Collections.getAsync(id);
 			
 			var selected = collectionsView.getSelectedCollection(true);
 			assert.equal(selected, id);

test/tests/dataObjectTest.js

@@ -7,7 +7,6 @@ describe("Zotero.DataObject", function() {
 		it("should load data on a regular item", function* () {
 			var item = new Zotero.Item('book');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			yield item.loadAllData();
 			assert.throws(item.getNote.bind(item), 'getNote() can only be called on notes and attachments');
 		})
@@ -15,7 +14,6 @@ describe("Zotero.DataObject", function() {
 		it("should load data on an attachment item", function* () {
 			var item = new Zotero.Item('attachment');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			yield item.loadAllData();
 			assert.equal(item.getNote(), '');
 		})
@@ -23,7 +21,6 @@ describe("Zotero.DataObject", function() {
 		it("should load data on a note item", function* () {
 			var item = new Zotero.Item('note');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			yield item.loadAllData();
 			assert.equal(item.getNote(), '');
 		})
@@ -52,7 +49,6 @@ describe("Zotero.DataObject", function() {
 		it("should be set to false after creating item", function* () {
 			var item = new Zotero.Item("book");
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			assert.isFalse(item.synced);
 			item.eraseTx();
 		});
@@ -60,7 +56,6 @@ describe("Zotero.DataObject", function() {
 		it("should be set to true when changed", function* () {
 			var item = new Zotero.Item("book");
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			
 			item.synced = 1;
 			yield item.saveTx();
@@ -72,7 +67,6 @@ describe("Zotero.DataObject", function() {
 		it("should be set to false after modifying item", function* () {
 			var item = new Zotero.Item("book");
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			
 			item.synced = 1;
 			yield item.saveTx();
@@ -88,7 +82,6 @@ describe("Zotero.DataObject", function() {
 		it("should be unchanged if skipSyncedUpdate passed", function* () {
 			var item = new Zotero.Item("book");
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			
 			item.synced = 1;
 			yield item.saveTx();

test/tests/dataObjectUtilitiesTest.js

@@ -13,12 +13,10 @@ describe("Zotero.DataObjectUtilities", function() {
 				yield Zotero.DB.executeTransaction(function* () {
 					var item = new Zotero.Item('book');
 					id1 = yield item.save();
-					item = yield Zotero.Items.getAsync(id1);
 					json1 = yield item.toJSON();
 					
 					var item = new Zotero.Item('book');
 					id2 = yield item.save();
-					item = yield Zotero.Items.getAsync(id2);
 					json2 = yield item.toJSON();
 				});
 				

test/tests/itemPaneTest.js

@@ -14,7 +14,6 @@ describe("Item pane", function () {
 		it("should refresh on item update", function* () {
 			var item = new Zotero.Item('book');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			
 			var itemBox = doc.getElementById('zotero-editpane-item-box');
 			var label = doc.getAnonymousNodes(itemBox)[0].getElementsByAttribute('fieldname', 'title')[1];
@@ -34,8 +33,6 @@ describe("Item pane", function () {
 		it("should refresh on note update", function* () {
 			var item = new Zotero.Item('note');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
-			
 			
 			// Wait for the editor
 			var noteBox = doc.getElementById('zotero-note-editor');

test/tests/itemTest.js

@@ -109,6 +109,7 @@ describe("Zotero.Item", function () {
 			var item = new Zotero.Item('book');
 			item.dateModified = dateModified;
 			var id = yield item.saveTx();
+			assert.equal(item.dateModified, dateModified);
 			item = yield Zotero.Items.getAsync(id);
 			assert.equal(item.dateModified, dateModified);
 		})
@@ -120,6 +121,7 @@ describe("Zotero.Item", function () {
 			var id = yield item.saveTx({
 				skipDateModifiedUpdate: true
 			});
+			assert.equal(item.dateModified, dateModified);
 			item = yield Zotero.Items.getAsync(id);
 			assert.equal(item.dateModified, dateModified);
 		})

test/tests/itemTreeViewTest.js

@@ -115,7 +115,6 @@ describe("Zotero.ItemTreeView", function() {
 			// Create item
 			var item = new Zotero.Item('book');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			
 			itemsView.selection.clearSelection();
 			assert.lengthOf(itemsView.getSelectedItems(), 0);
@@ -139,7 +138,6 @@ describe("Zotero.ItemTreeView", function() {
 			// Create item
 			var item = new Zotero.Item('book');
 			var id = yield item.saveTx();
-			item = yield Zotero.Items.getAsync(id);
 			
 			yield itemsView.selectItem(id);
 			var selected = itemsView.getSelectedItems(true);