diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index 8b3400371..4783d1d68 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/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 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); } diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 2b2b72d37..cab8d1c48 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/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) { - let oldTags = this._previousData.tags || []; - let newTags = this._tags; + if (this._changedData.tags) { + let oldTags = 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 typeof tag == 'string' ? { tag } : tag); @@ -3401,9 +3405,7 @@ Zotero.Item.prototype.setTags = function (tags) { return; } - this._markFieldChange('tags', this._tags); - this._changed.tags = true; - this._tags = newTags; + this._markFieldChange('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); - if (newTags.length == this._tags.length) { + var oldTags = this._changedData.tags || this._tags; + 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; } diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index 537f08107..bc46eafa0 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/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( diff --git a/test/tests/dataObjectTest.js b/test/tests/dataObjectTest.js index eab9324ab..008211b6e 100644 --- a/test/tests/dataObjectTest.js +++ b/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;