From ef7da3486a4b517a6beb45ecc6115812e99905a8 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sun, 16 Jul 2017 18:15:12 -0400 Subject: [PATCH] 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. --- .../content/zotero/xpcom/data/dataObject.js | 33 +++++++++++++++-- chrome/content/zotero/xpcom/data/item.js | 35 ++++++++++--------- chrome/content/zotero/xpcom/data/items.js | 1 - test/tests/dataObjectTest.js | 30 +++++++++++++++- 4 files changed, 78 insertions(+), 21 deletions(-) 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;