New state-handling approach for item tag changes

If this works out I think we'll want to use this approach for
all data layer changes.

Previously, an unsaved change on an object would update its state
immediately, which was fine for synchronous code but breaks down if a
save involves multiple asynchronous calls, because modifying state after
the relevant data has been saved to the DB but before the `_changed`
object has been cleared would mean that new changes would be lost. Now,
changes are written to _changedData, and a get for the data first checks
_changedData before checking the state property (e.g., _tags) directly.
The changedData property is cleared as it's written, and once the object
is saved, the reload updates the state property with the new data.

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

@@ -633,10 +633,11 @@ Zotero.DataObject.prototype.reload = Zotero.Promise.coroutine(function* (dataTyp
 		for (let i=0; i<dataTypes.length; i++) {
 			let dataType = dataTypes[i];
 			if (!this._loaded[dataType] || this._skipDataTypeLoad[dataType]
-					|| (!reloadUnchanged && !this._changed[dataType])) {
+					|| (!reloadUnchanged && !this._changed[dataType] && !this._dataTypesToReload.has(dataType))) {
 				continue;
 			}
 			yield this.loadDataType(dataType, true);
+			this._dataTypesToReload.delete(dataType);
 		}
 	}
 });
@@ -701,6 +702,17 @@ Zotero.DataObject.prototype._markAllDataTypeLoadStates = function (loaded) {
  * @param {} oldValue
  */
 Zotero.DataObject.prototype._markFieldChange = function (field, oldValue) {
+	// New method (changedData)
+	if (field == 'tags') {
+		if (Array.isArray(oldValue)) {
+			this._changedData[field] = [...oldValue];
+		}
+		else {
+			this._changedData[field] = oldValue;
+		}
+		return;
+	}
+	
 	// Only save if object already exists and field not already changed
 	if (!this.id || this._previousData[field] !== undefined) {
 		return;
@@ -716,7 +728,10 @@ Zotero.DataObject.prototype._markFieldChange = function (field, oldValue) {
 
 
 Zotero.DataObject.prototype.hasChanged = function() {
-	var changed = Object.keys(this._changed).filter(dataType => this._changed[dataType]);
+	var changed = Object.keys(this._changed).filter(dataType => this._changed[dataType])
+		.concat(
+			Object.keys(this._changedData).filter(dataType => this._changedData[dataType])
+		);
 	if (changed.length == 1
 			&& changed[0] == 'primaryData'
 			&& Object.keys(this._changed.primaryData).length == 1
@@ -736,10 +751,13 @@ Zotero.DataObject.prototype._clearChanged = function (dataType) {
 	if (dataType) {
 		delete this._changed[dataType];
 		delete this._previousData[dataType];
+		delete this._changedData[dataType];
 	}
 	else {
 		this._changed = {};
 		this._previousData = {};
+		this._changedData = {};
+		this._dataTypesToReload = new Set();
 	}
 }
 
@@ -749,6 +767,17 @@ Zotero.DataObject.prototype._clearChanged = function (dataType) {
  */
 Zotero.DataObject.prototype._clearFieldChange = function (field) {
 	delete this._previousData[field];
+	delete this._changedData[field];
+}
+
+
+/**
+ * Mark a data type as requiring a reload when the current save finishes. The changed state is cleared
+ * before the new data is saved to the database (so that further updates during the save process don't
+ * get lost), so we need to separately keep track of what changed.
+ */
+Zotero.DataObject.prototype._markForReload = function (dataType) {
+	this._dataTypesToReload.add(dataType);
 }
 
 

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

@@ -1748,13 +1748,16 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 	}
 	
 	// Tags
-	if (this._changed.tags) {
+	if (this._changedData.tags) {
-		let oldTags = this._previousData.tags || [];
+		let oldTags = this._tags;
-		let newTags = this._tags;
+		let newTags = this._changedData.tags;
+		this._clearChanged('tags');
+		this._markForReload('tags');
 		
 		// Convert to individual JSON objects, diff, and convert back
 		let oldTagsJSON = oldTags.map(x => JSON.stringify(x));
 		let newTagsJSON = newTags.map(x => JSON.stringify(x));
+		
 		let toAdd = Zotero.Utilities.arrayDiff(newTagsJSON, oldTagsJSON).map(x => JSON.parse(x));
 		let toRemove = Zotero.Utilities.arrayDiff(oldTagsJSON, newTagsJSON).map(x => JSON.parse(x));
 		
@@ -1825,7 +1828,6 @@ Zotero.Item.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
 		if (env.isNew) {
 			this._markAllDataTypeLoadStates(true);
 		}
-		this._clearChanged();
 	}
 	
 	return env.isNew ? this.id : true;
@@ -1988,7 +1990,7 @@ Zotero.Item.prototype.hasNote = Zotero.Promise.coroutine(function* () {
 Zotero.Item.prototype.getNote = function() {
 	if (!this.isNote() && !this.isAttachment()) {
 		throw new Error("getNote() can only be called on notes and attachments "
-			+ `(${this.libraryID}/${this.key} is a {Zotero.ItemTypes.getName(this.itemTypeID)})`);
+			+ `(${this.libraryID}/${this.key} is a ${Zotero.ItemTypes.getName(this.itemTypeID)})`);
 	}
 	
 	// Store access time for later garbage collection
@@ -3336,7 +3338,7 @@ Zotero.Item.prototype.clearBestAttachmentState = function () {
 Zotero.Item.prototype.getTags = function () {
 	this._requireData('tags');
 	// BETTER DEEP COPY?
-	return JSON.parse(JSON.stringify(this._tags));
+	return JSON.parse(JSON.stringify(this._changedData.tags || this._tags));
 };
 
 
@@ -3348,7 +3350,8 @@ Zotero.Item.prototype.getTags = function () {
  */
 Zotero.Item.prototype.hasTag = function (tagName) {
 	this._requireData('tags');
-	return this._tags.some(tagData => tagData.tag == tagName);
+	var tags = this._changedData.tags || this._tags;
+	return tags.some(tagData => tagData.tag == tagName);
 }
 
 
@@ -3357,8 +3360,8 @@ Zotero.Item.prototype.hasTag = function (tagName) {
  */
 Zotero.Item.prototype.getTagType = function (tagName) {
 	this._requireData('tags');
-	for (let i=0; i<this._tags.length; i++) {
+	var tags = this._changedData.tags || this._tags;
-		let tag = this._tags[i];
+	for (let tag of tags) {
 		if (tag.tag === tagName) {
 			return tag.type ? tag.type : 0;
 		}
@@ -3376,7 +3379,8 @@ Zotero.Item.prototype.getTagType = function (tagName) {
  *                                   (e.g., [{tag: 'tag', type: 1}])
  */
 Zotero.Item.prototype.setTags = function (tags) {
-	var oldTags = this.getTags();
+	this._requireData('tags');
+	var oldTags = this._changedData.tags || this._tags;
 	var newTags = tags.concat()
 		// Allow array of strings
 		.map(tag => typeof tag == 'string' ? { tag } : tag);
@@ -3401,9 +3405,7 @@ Zotero.Item.prototype.setTags = function (tags) {
 		return;
 	}
 	
-	this._markFieldChange('tags', this._tags);
+	this._markFieldChange('tags', newTags);
-	this._changed.tags = true;
-	this._tags = newTags;
 }
 
 
@@ -3456,8 +3458,6 @@ Zotero.Item.prototype.replaceTag = function (oldTag, newTag) {
 	var tags = this.getTags();
 	newTag = newTag.trim();
 	
-	Zotero.debug("REPLACING TAG " + oldTag + " " + newTag);
-	
 	if (newTag === "") {
 		Zotero.debug('Not replacing with empty tag', 2);
 		return false;
@@ -3488,8 +3488,9 @@ Zotero.Item.prototype.replaceTag = function (oldTag, newTag) {
  */
 Zotero.Item.prototype.removeTag = function(tagName) {
 	this._requireData('tags');
-	var newTags = this._tags.filter(tagData => tagData.tag !== tagName);
+	var oldTags = this._changedData.tags || this._tags;
-	if (newTags.length == this._tags.length) {
+	var newTags = oldTags.filter(tagData => tagData.tag !== tagName);
+	if (newTags.length == oldTags.length) {
 		Zotero.debug('Cannot remove missing tag ' + tagName + ' from item ' + this.libraryKey);
 		return;
 	}

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

@@ -664,7 +664,6 @@ Zotero.Items = function() {
 			}
 			
 			item._loaded.tags = true;
-			item._clearChanged('tags');
 		}.bind(this);
 		
 		yield Zotero.DB.queryAsync(

test/tests/dataObjectTest.js

@@ -241,7 +241,8 @@ describe("Zotero.DataObject", function() {
 				assert.isTrue(obj.hasChanged());
 			}
 		})
-	})
+	});
+	
 	
 	describe("#save()", function () {
 		it("should add new identifiers to cache", function* () {
@@ -264,6 +265,33 @@ describe("Zotero.DataObject", function() {
 			}
 		})
 		
+		it("should handle additional tag change in the middle of a save", function* () {
+			var item = yield createDataObject('item');
+			item.setTags(['a']);
+			
+			var deferred = new Zotero.Promise.defer();
+			var origFunc = Zotero.Notifier.queue.bind(Zotero.Notifier);
+			sinon.stub(Zotero.Notifier, "queue").callsFake(function (event, type, ids, extraData) {
+				// Add a new tag after the first one has been added to the DB and before the save is
+				// finished. The changed state should've cleared before saving to the DB the first
+				// time, so the second setTags() should mark the item as changed and allow the new tag
+				// to be saved in the second saveTx().
+				if (event == 'add' && type == 'item-tag') {
+					item.setTags(['a', 'b']);
+					Zotero.Notifier.queue.restore();
+					deferred.resolve(item.saveTx());
+				}
+				origFunc(...arguments);
+			});
+			
+			yield Zotero.Promise.all([item.saveTx(), deferred.promise]);
+			assert.sameMembers(item.getTags().map(o => o.tag), ['a', 'b']);
+			var tags = yield Zotero.DB.columnQueryAsync(
+				"SELECT name FROM tags JOIN itemTags USING (tagID) WHERE itemID=?", item.id
+			);
+			assert.sameMembers(tags, ['a', 'b']);
+		});
+		
 		describe("Edit Check", function () {
 			var group;