From 1a0849e489b4da10ae85e44c3f01de23e604ec85 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sat, 2 Feb 2013 05:31:14 -0500 Subject: [PATCH] Fix and change extraData for item change notifications - Some item changes were putting data in the wrong form into extraData, which was keeping it from being passed through in notifications. - For item modifications, set a 'changed' object, keyed by itemID, with just the fields that changed as keys and their old values. For deletes, keep the 'old' object for now, since sync relies on it. - Remove item.serialize() for all item changes except deletions, which should speed up writes (and which will leave extraData empty for some changes). - Currently only item fields, creators, related items ('related'), and 'parentItem' are added to 'changed'. Closes #220 --- chrome/content/zotero/xpcom/data/item.js | 189 ++++++++++++++--------- chrome/content/zotero/xpcom/notifier.js | 8 +- 2 files changed, 118 insertions(+), 79 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index b4ca880bd3..c30398ae5e 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -64,7 +64,7 @@ Zotero.Item.prototype._init = function () { this._itemDataLoaded = false; this._relatedItemsLoaded = false; - this._changed = false; + this._changed = {}; this._changedPrimaryData = false; this._changedItemData = false; this._changedCreators = false; @@ -74,7 +74,7 @@ Zotero.Item.prototype._init = function () { this._changedAttachmentData = false; this._skipModTimeUpdate = false; - this._previousData = null; + this._previousData = {}; this._deleted = null; this._noteTitle = null; @@ -412,7 +412,7 @@ Zotero.Item.prototype.loadFromRow = function(row, reload) { * Check if any data fields have changed since last save */ Zotero.Item.prototype.hasChanged = function() { - return !!(this._changed + return !!(Object.keys(this._changed).length || this._changedPrimaryData || this._changedItemData || this._changedCreators @@ -554,7 +554,7 @@ Zotero.Item.prototype.setType = function(itemTypeID, loadIn) { if (copiedFields) { for each(var f in copiedFields) { - this.setField(f[0], f[1]); + this.setField(f[0], f[1], true); } } @@ -562,6 +562,7 @@ Zotero.Item.prototype.setType = function(itemTypeID, loadIn) { this._itemDataLoaded = false; } else { + this._markFieldChange('itemType', Zotero.ItemTypes.getName(oldItemTypeID)); if (!this._changedPrimaryData) { this._changedPrimaryData = {}; } @@ -720,10 +721,9 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) { if (this['_' + field] != value) { Zotero.debug("Field '" + field + "' has changed from '" + this['_' + field] + "' to '" + value + "'", 4); - // Save a copy of the object before modifying - if (this.id && this.exists() && !this._previousData) { - this._previousData = this.serialize(); - } + // Save a copy of the field before modifying + this._markFieldChange(field, this['_' + field]); + if (field == 'itemTypeID') { this.setType(value, loadIn); } @@ -808,10 +808,10 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) { return false; } - // Save a copy of the object before modifying - if (this.id && this.exists() && !this._previousData) { - this._previousData = this.serialize(); - } + // Save a copy of the field before modifying + this._markFieldChange( + Zotero.ItemFields.getName(field), this._itemData[fieldID] + ); } this._itemData[fieldID] = value; @@ -1052,15 +1052,20 @@ Zotero.Item.prototype.setCreator = function(orderIndex, creator, creatorTypeIDOr return false; } + // Save copy of old creators for notifier + if (!this._changedCreators) { + this._changedCreators = {}; + + var oldCreators = this._getOldCreators() + this._markFieldChange('creators', oldCreators); + } + this._changedCreators[orderIndex] = true; + this._creators[orderIndex] = { ref: creator, creatorTypeID: creatorTypeID }; - if (!this._changedCreators) { - this._changedCreators = {}; - } - this._changedCreators[orderIndex] = true; return true; } @@ -1083,6 +1088,14 @@ Zotero.Item.prototype.removeCreator = function(orderIndex) { Zotero.Prefs.set('purge.creators', true); } + // Save copy of old creators for notifier + if (!this._changedCreators) { + this._changedCreators = {}; + + var oldCreators = this._getOldCreators(); + this._markFieldChange('creators', oldCreators); + } + // Shift creator orderIndexes down, going to length+1 so we clear the last one for (var i=orderIndex, max=this._creators.length+1; i 0 || oldIDs.length != currentIDs.length) { - this._prepFieldChange('relatedItems'); + this._markFieldChange('related', currentIDs); + this._changed.relatedItems = true } else { Zotero.debug('Related items not changed in Zotero.Item._setRelatedItems()', 4); @@ -4769,17 +4763,58 @@ Zotero.Item.prototype._setRelatedItems = function (itemIDs) { } -// TODO: use for stuff other than related items -Zotero.Item.prototype._prepFieldChange = function (field) { - if (!this._changed) { - this._changed = {}; +/** + * The creator has already been changed in itembox.xml before being passed + * to setCreator()/removeCreator(), so we have to reach in and get its + * previousData, and ideally try to detect when this private data structure + * has changed, which it almost certainly will. I am so sorry. + */ +Zotero.Item.prototype._getOldCreators = function () { + var oldCreators = []; + for (var i in this._creators) { + if (this._creators[i].ref._changed) { + if (!this._creators[i].ref._previousData + && !this._creators[i].ref._previousData.fields) { + Components.utils.reportError("Previous creator data not available in expected form"); + oldCreators.push(false); + continue; + } + var c = this._creators[i].ref._previousData.fields; + } + else { + var c = this._creators[i].ref; + } + + var old = { + // Convert creatorTypeIDs to text + creatorType: Zotero.CreatorTypes.getName( + this._creators[i].creatorTypeID + ) + }; + + if (c.fieldMode) { + // In 'fields' there's just 'name' for single-field mode + old.name = typeof c.name == 'undefined' ? c.lastName : c.name; + } + else { + old.firstName = c.firstName; + old.lastName = c.lastName; + } + oldCreators.push(old); } - this._changed[field] = true; - - // Save a copy of the data before changing - if (this.id && this.exists() && !this._previousData) { - this._previousData = this.serialize(); + return oldCreators; +} + + +/** + * Save old version of data that's being changed, to pass to the notifier + */ +Zotero.Item.prototype._markFieldChange = function (field, oldValue) { + // Only save if item already exists and field not already changed + if (!this.id || !this.exists() || this._previousData[field]) { + return; } + this._previousData[field] = oldValue; } diff --git a/chrome/content/zotero/xpcom/notifier.js b/chrome/content/zotero/xpcom/notifier.js index 01cda49652..27b3bfa8ae 100644 --- a/chrome/content/zotero/xpcom/notifier.js +++ b/chrome/content/zotero/xpcom/notifier.js @@ -136,7 +136,9 @@ Zotero.Notifier = new function(){ // Merge extraData keys if (extraData) { for (var dataID in extraData) { - _queue[type][event].data[dataID] = extraData[dataID]; + if (extraData[dataID]) { + _queue[type][event].data[dataID] = extraData[dataID]; + } } } @@ -271,7 +273,9 @@ Zotero.Notifier = new function(){ if (runQueue[type][event].ids.indexOf(id) == -1) { runQueue[type][event].ids.push(id); - runQueue[type][event].data[id] = data; + if (data) { + runQueue[type][event].data[id] = data; + } } }