Honor .synced on data objects in all cases

Previously, if .synced was already true, setting it to true and saving
would result in .synced == false unless skipSyncedUpdate was passed. Now
the value assigned to .synced is always used on the next save. If the
value hasn't changed and no other values have changed, a save will be a
no-op.

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

@@ -128,7 +128,7 @@ Zotero.DataObject.prototype._set = function (field, value) {
 			break;
 	}
 	
-	if (this['_' + field] != value) {
+	if (this['_' + field] != value || field == 'synced') {
 		this._markFieldChange(field, this['_' + field]);
 		if (!this._changed.primaryData) {
 			this._changed.primaryData = {};
@@ -905,8 +905,14 @@ Zotero.DataObject.prototype.saveTx = function (options) {
 
 
 Zotero.DataObject.prototype.hasChanged = function() {
-	Zotero.debug(this._changed);
+	var changed = Object.keys(this._changed).filter(dataType => this._changed[dataType]);
-	return !!Object.keys(this._changed).filter(dataType => this._changed[dataType]).length
+	if (changed.length == 1
+			&& changed[0] == 'primaryData'
+			&& this._changed.primaryData.synced
+			&& this._previousData.synced == this._synced) {
+		return false;
+	}
+	return !!changed.length;
 }
 
 Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env) {

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

@@ -685,7 +685,7 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) {
 		*/
 		
 		// If field value has changed
-		if (this['_' + field] != value) {
+		if (this['_' + field] != value || field == 'synced') {
 			Zotero.debug("Field '" + field + "' has changed from '" + this['_' + field] + "' to '" + value + "'", 4);
 			
 			// Save a copy of the field before modifying

test/tests/dataObjectTest.js

@@ -45,54 +45,131 @@ describe("Zotero.DataObject", function() {
 	})
 	
 	describe("#synced", function () {
-		it("should be set to false after creating item", function* () {
+		it("should be set to false after creating object", function* () {
-			var item = new Zotero.Item("book");
+			for (let type of types) {
-			var id = yield item.saveTx();
+				var obj = createUnsavedDataObject(type);
-			assert.isFalse(item.synced);
+				var id = yield obj.saveTx();
-			yield item.eraseTx();
+				assert.isFalse(obj.synced);
+				yield obj.eraseTx();
+			}
 		});
 		
-		it("should be set to true when changed", function* () {
+		it("should be set to false after modifying object", function* () {
-			var item = new Zotero.Item("book");
+			for (let type of types) {
-			var id = yield item.saveTx();
+				var obj = createUnsavedDataObject(type);
-			
+				var id = yield obj.saveTx();
-			item.synced = 1;
+				
-			yield item.saveTx();
+				obj.synced = true;
-			assert.ok(item.synced);
+				yield obj.saveTx();
-			
+				
-			yield item.eraseTx();
+				if (type == 'item') {
+					yield obj.loadItemData();
+					obj.setField('title', Zotero.Utilities.randomString());
+				}
+				else {
+					obj.name = Zotero.Utilities.randomString();
+				}
+				yield obj.saveTx();
+				assert.isFalse(obj.synced);
+				
+				yield obj.eraseTx();
+			}
 		});
 		
-		it("should be set to false after modifying item", function* () {
+		it("should be changed to true explicitly with no other changes", function* () {
-			var item = new Zotero.Item("book");
+			for (let type of types) {
-			var id = yield item.saveTx();
+				var obj = createUnsavedDataObject(type);
-			
+				var id = yield obj.saveTx();
-			item.synced = 1;
+				
-			yield item.saveTx();
+				obj.synced = true;
-			
+				yield obj.saveTx();
-			yield item.loadItemData();
+				assert.isTrue(obj.synced);
-			item.setField('title', 'test');
+				
-			yield item.saveTx();
+				yield obj.eraseTx();
-			assert.isFalse(item.synced);
+			}
-			
-			yield item.eraseTx();
 		});
 		
+		it("should be changed to true explicitly with other field changes", function* () {
+			for (let type of types) {
+				var obj = createUnsavedDataObject(type);
+				var id = yield obj.saveTx();
+				
+				if (type == 'item') {
+					obj.setField('title', Zotero.Utilities.randomString());
+				}
+				else {
+					obj.name = Zotero.Utilities.randomString();
+				}
+				obj.synced = true;
+				yield obj.saveTx();
+				assert.isTrue(obj.synced);
+				
+				yield obj.eraseTx();
+			}
+		});
+		
+		it("should remain at true if set explicitly", function* () {
+			for (let type of types) {
+				var obj = createUnsavedDataObject(type);
+				obj.synced = true;
+				var id = yield obj.saveTx();
+				assert.isTrue(obj.synced);
+				
+				if (type == 'item') {
+					obj.setField('title', 'test');
+				}
+				else {
+					obj.name = Zotero.Utilities.randomString();
+				}
+				obj.synced = true;
+				yield obj.saveTx();
+				assert.isTrue(obj.synced);
+				
+				yield obj.eraseTx();
+			}
+		});
+		
+		it("shouldn't cause a save if unchanged and nothing else changed", function* () {
+			for (let type of types) {
+				var obj = createUnsavedDataObject(type);
+				obj.synced = true;
+				var id = yield obj.saveTx();
+				assert.isTrue(obj.synced);
+				
+				obj.synced = true;
+				assert.isFalse(obj.hasChanged(), type + " shouldn't be changed");
+				
+				var obj = createUnsavedDataObject(type);
+				obj.synced = false;
+				var id = yield obj.saveTx();
+				assert.isFalse(obj.synced);
+				obj.synced = false;
+				assert.isFalse(obj.hasChanged(), type + " shouldn't be changed");
+			}
+		})
+		
 		it("should be unchanged if skipSyncedUpdate passed", function* () {
-			var item = new Zotero.Item("book");
+			for (let type of types) {
-			var id = yield item.saveTx();
+				var obj = createUnsavedDataObject(type);
-			
+				var id = yield obj.saveTx();
-			item.synced = 1;
+				
-			yield item.saveTx();
+				obj.synced = 1;
-			
+				yield obj.saveTx();
-			yield item.loadItemData();
+				
-			item.setField('title', 'test');
+				if (type == 'item') {
-			yield item.saveTx({
+					yield obj.loadItemData();
-				skipSyncedUpdate: true
+					obj.setField('title', Zotero.Utilities.randomString());
-			});
+				}
-			assert.ok(item.synced);
+				else {
-			
+					obj.name = Zotero.Utilities.randomString();
-			yield item.eraseTx();
+				}
+				yield obj.saveTx({
+					skipSyncedUpdate: true
+				});
+				assert.ok(obj.synced);
+				
+				yield obj.eraseTx();
+			}
 		});
 	})