mirrors/zotero
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions

New state-handling approach for item tag changes

Browse source 
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.
This commit is contained in:
Dan Stillman 2017-07-16 18:15:12 -04:00
parent 09a859d7e3
commit ef7da3486a
4 changed files with 78 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
chrome/content/zotero/xpcom/data/dataObject.js
View file

@ -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);
}

35
chrome/content/zotero/xpcom/data/item.js
View file

@ -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<this._tags.length; i++) {
let tag = this._tags[i];
var tags = this._changedData.tags || this._tags;
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._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;
}

1
chrome/content/zotero/xpcom/data/items.js
View file

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

30
test/tests/dataObjectTest.js
View file

@ -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;