From 199619f40e3001fb49734977a2446b7e715daa73 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sat, 12 Sep 2020 16:09:49 -0400 Subject: [PATCH] Remove .noteSchemaVersion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This leaves item.note in place, rather than reverting all the `getNote()` → `.note` changes. We can consider which we want to keep. --- .../content/zotero/xpcom/data/dataObject.js | 2 +- .../zotero/xpcom/data/dataObjectUtilities.js | 5 - chrome/content/zotero/xpcom/data/item.js | 153 +++++++----------- chrome/content/zotero/xpcom/data/items.js | 15 +- chrome/content/zotero/xpcom/data/notes.js | 5 - chrome/content/zotero/xpcom/schema.js | 2 - resource/schema/userdata.sql | 1 - test/content/support.js | 5 +- test/tests/itemTest.js | 48 ------ test/tests/syncLocalTest.js | 23 --- 10 files changed, 60 insertions(+), 199 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index 609d2f9955..524088476c 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -763,7 +763,7 @@ Zotero.DataObject.prototype._getLatestField = function (field) { */ Zotero.DataObject.prototype._markFieldChange = function (field, value) { // New method (changedData) - if (['deleted', 'tags', 'note'].includes(field) || field.startsWith('annotation')) { + if (['deleted', 'tags'].includes(field) || field.startsWith('annotation')) { if (Array.isArray(value)) { this._changedData[field] = [...value]; } diff --git a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js index 72ad50c124..1f3ca4717d 100644 --- a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js +++ b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js @@ -380,11 +380,6 @@ Zotero.DataObjectUtilities = { continue; } - // Ignore noteSchemaVersion: 0 if no local note - if (field == 'noteSchemaVersion' && val === 0) { - continue; - } - changeset.push({ field: field, op: "add", diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 5dedf92ea0..0aa9532897 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -56,18 +56,13 @@ Zotero.Item = function(itemTypeOrID) { // loadItemData this._itemData = null; + this._noteTitle = null; + this._noteText = null; this._displayTitle = null; - this._note = { - data: null, - title: null, - schemaVersion: Zotero.Notes.schemaVersion - }; - // loadChildItems this._attachments = null; this._notes = null; - this._annotations = null; // loadAnnotation this._annotationType = null; @@ -760,7 +755,7 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) { } if (loadIn && this.isNote() && field == Zotero.ItemFields.getID('title')) { - this._note.title = value ? value : ""; + this._noteTitle = value ? value : ""; return true; } @@ -1678,44 +1673,34 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { } // Note - if ((isNew && this.isNote()) || this._hasFieldChanged('note')) { - let note = this._getLatestField('note'); - - // Since we override change detection for new notes and always save data, we have to mark - // the data type as loaded so it gets reloaded by Zotero.DataObject.reload() - if (isNew) { - this._loaded.note = true; - } - else { - if (note.data === null || note.title === null) { - throw new Error("Note marked as changed with cached values not set"); + if ((isNew && this.isNote()) || this._changed.note) { + if (!isNew) { + if (this._noteText === null || this._noteTitle === null) { + throw new Error("Cached note values not set with " + + "this._changed.note set to true"); } } let parent = this.isNote() ? this.parentID : null; - let noteData = note.data || ''; + let noteText = this._noteText ? this._noteText : ''; // Add
wrapper if not present - if (!noteData.match(/^
[\s\S]*<\/div>$/)) { - noteData = Zotero.Notes.notePrefix + noteData + Zotero.Notes.noteSuffix; + if (!noteText.match(/^
[\s\S]*<\/div>$/)) { + noteText = Zotero.Notes.notePrefix + noteText + Zotero.Notes.noteSuffix; } let params = [ parent ? parent : null, - noteData, - note.title || '', - note.schemaVersion + noteText, + this._noteTitle ? this._noteTitle : '' ]; - this._clearChanged('note'); - this._markForReload('note'); - let sql = "SELECT COUNT(*) FROM itemNotes WHERE itemID=?"; if (yield Zotero.DB.valueQueryAsync(sql, itemID)) { - sql = "UPDATE itemNotes SET parentItemID=?, note=?, title=?, schemaVersion=? WHERE itemID=?"; + sql = "UPDATE itemNotes SET parentItemID=?, note=?, title=? WHERE itemID=?"; params.push(itemID); } else { sql = "INSERT INTO itemNotes " - + "(itemID, parentItemID, note, title, schemaVersion) VALUES (?,?,?,?,?)"; + + "(itemID, parentItemID, note, title) VALUES (?,?,?,?)"; params.unshift(itemID); } yield Zotero.DB.queryAsync(sql, params); @@ -2028,16 +2013,10 @@ Zotero.Item.prototype.numNotes = function(includeTrashed, includeEmbedded) { */ Zotero.Item.prototype.getNoteTitle = function() { if (!this.isNote() && !this.isAttachment()) { - throw new Error("getNoteTitle() can only be called on notes and attachments"); + throw ("getNoteTitle() can only be called on notes and attachments"); } - - var note = this._getLatestField('note'); - if (note.title !== null) { - return note.title; - } - if (note.data !== null) { - note.title = Zotero.Notes.noteToTitle(note.data); - return note.title; + if (this._noteTitle !== null) { + return this._noteTitle; } this._requireData('itemData'); return ""; @@ -2068,89 +2047,66 @@ Zotero.Item.prototype.hasNote = Zotero.Promise.coroutine(function* () { Zotero.defineProperty(Zotero.Item.prototype, 'note', { get: function () { - if (!this.isNote() && !this.isAttachment()) { - throw new Error(".note can only be accessed on notes and attachments " - + `(${this.libraryID}/${this.key} is a ${Zotero.ItemTypes.getName(this.itemTypeID)})`); - } - - // Store access time for later garbage collection - this._noteAccessTime = new Date(); - - var note = this._getLatestField('note'); - - if (note.data !== null) { - return note.data; - } - - this._requireData('note'); - return ""; + return this.getNote(); } }); /** - * Get the contents of an item note + * Get the text of an item note **/ Zotero.Item.prototype.getNote = function() { - Zotero.warn("Zotero.Item::getNote() is deprecated -- use .note"); - return this.note; + 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)})`); + } + + // Store access time for later garbage collection + this._noteAccessTime = new Date(); + + if (this._noteText !== null) { + return this._noteText; + } + + this._requireData('note'); + return ""; } -Zotero.defineProperty(Zotero.Item.prototype, 'noteSchemaVersion', { - get: function() { - if (!this.isNote() && !this.isAttachment()) { - return undefined; - } - return this._getLatestField('note').schemaVersion; - } -}); - - /** - * Set an item note - * - * Note: This can only be called on notes and attachments - * - * @param {String} data - Note contents - * @param {Number} [schemaVersion = Zotero.Notes.schemaVersion] - */ -Zotero.Item.prototype.setNote = function (data, schemaVersion) { +* Set an item note +* +* Note: This can only be called on notes and attachments +**/ +Zotero.Item.prototype.setNote = function(text) { if (!this.isNote() && !this.isAttachment()) { throw ("updateNote() can only be called on notes and attachments"); } - if (typeof data != 'string') { - throw new Error("data must be a string (was " + typeof data + ")"); + if (typeof text != 'string') { + throw ("text must be a string in Zotero.Item.setNote() (was " + typeof text + ")"); } - if (schemaVersion === undefined) { - schemaVersion = Zotero.Notes.schemaVersion; - } - if (typeof schemaVersion != 'number' || schemaVersion != parseInt(schemaVersion)) { - throw new Error(`schemaVersion must be an integer (was ${JSON.stringify(schemaVersion)})`); - } - - data = data + text = text // Strip control characters .replace(/[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F]/g, "") .trim(); - var { data: oldData, schemaVersion: oldSchemaVersion } = this._getLatestField('note'); - if (data === oldData && schemaVersion === oldSchemaVersion) { + var oldText = this.getNote(); + if (text === oldText) { Zotero.debug("Note hasn't changed", 4); return false; } - this._hasNote = data !== ''; - var title = Zotero.Notes.noteToTitle(data); - - // This isn't quite correct because the save could fail + this._hasNote = text !== ''; + this._noteText = text; + this._noteTitle = Zotero.Notes.noteToTitle(text); if (this.isNote()) { - this._displayTitle = title; + this._displayTitle = this._noteTitle; } - this._markFieldChange('note', { data, title, schemaVersion }); + this._markFieldChange('note', oldText); + this._changed.note = true; return true; } @@ -4316,7 +4272,7 @@ Zotero.Item.prototype.clone = function (libraryID, options = {}) { newItem.setCreators(this.getCreators()); } else { - newItem.setNote(this.note, this.noteSchemaVersion); + newItem.setNote(this.getNote()); if (sameLibrary) { var parent = this.parentKey; if (parent) { @@ -4604,6 +4560,7 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { case 'key': case 'version': case 'itemType': + case 'note': // Use? case 'md5': case 'mtime': @@ -4612,7 +4569,6 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { // Handled below // case 'note': - case 'noteSchemaVersion': case 'collections': case 'parentItem': case 'deleted': @@ -4878,7 +4834,7 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { if (!this.isAnnotation()) { let note = json.note; - this.setNote(note !== undefined ? note : "", json.noteSchemaVersion); + this.setNote(note !== undefined ? note : ""); } } @@ -4965,10 +4921,9 @@ Zotero.Item.prototype.toJSON = function (options = {}) { // Notes and embedded attachment notes if (this.isAttachment() || this.isNote()) { - let note = this.note; + let note = this.getNote(); if (note !== "" || mode == 'full' || (mode == 'new' && this.isNote())) { obj.note = note; - obj.noteSchemaVersion = this.noteSchemaVersion || 0; } } diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index b1fde27b17..b512e3a7b2 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/chrome/content/zotero/xpcom/data/items.js @@ -404,7 +404,7 @@ Zotero.Items = function() { this._loadNotes = Zotero.Promise.coroutine(function* (libraryID, ids, idSQL) { var notesToUpdate = []; - var sql = "SELECT itemID, note, schemaVersion FROM items " + var sql = "SELECT itemID, note FROM items " + "JOIN itemNotes USING (itemID) " + "WHERE libraryID=?" + idSQL; var params = [libraryID]; @@ -420,7 +420,6 @@ Zotero.Items = function() { throw new Error("Item " + itemID + " not found"); } let note = row.getResultByIndex(1); - let schemaVersion = row.getResultByIndex(2); // Convert non-HTML notes on-the-fly if (note !== "") { @@ -451,10 +450,7 @@ Zotero.Items = function() { } } - item._note.data = note || ''; - item._note.schemaVersion = schemaVersion; - // Lazily loaded - item._note.title = null; + item._noteText = note ? note : ''; item._loaded.note = true; item._clearChanged('note'); }.bind(this) @@ -487,8 +483,7 @@ Zotero.Items = function() { throw new Error("Item " + itemID + " not loaded"); } - item._note.data = ''; - item._note.schemaVersion = 0; + item._noteText = ''; item._loaded.note = true; item._clearChanged('note'); }.bind(this) @@ -747,9 +742,7 @@ Zotero.Items = function() { // Mark all top-level items as having child items loaded sql = "SELECT itemID FROM items I WHERE libraryID=?" + idSQL + " AND itemID NOT IN " - + "(SELECT itemID FROM itemAttachments " - + "UNION SELECT itemID FROM itemNotes " - + "UNION SELECT itemID FROM itemAnnotations)"; + + "(SELECT itemID FROM itemAttachments UNION SELECT itemID FROM itemNotes)"; yield Zotero.DB.queryAsync( sql, params, diff --git a/chrome/content/zotero/xpcom/data/notes.js b/chrome/content/zotero/xpcom/data/notes.js index 2b67959054..ceaf504d09 100644 --- a/chrome/content/zotero/xpcom/data/notes.js +++ b/chrome/content/zotero/xpcom/data/notes.js @@ -32,11 +32,6 @@ Zotero.Notes = new function() { this.__defineGetter__("notePrefix", function () { return '
'; }); this.__defineGetter__("noteSuffix", function () { return '
'; }); - Zotero.defineProperty(this, 'schemaVersion', { - value: 1, - writable: false - }); - /** * Return first line (or first MAX_LENGTH characters) of note content **/ diff --git a/chrome/content/zotero/xpcom/schema.js b/chrome/content/zotero/xpcom/schema.js index 5ba7b92c7b..6811a6e080 100644 --- a/chrome/content/zotero/xpcom/schema.js +++ b/chrome/content/zotero/xpcom/schema.js @@ -3238,8 +3238,6 @@ Zotero.Schema = new function(){ yield Zotero.DB.queryAsync("CREATE TABLE itemAnnotations (\n itemID INTEGER PRIMARY KEY,\n parentItemID INT NOT NULL,\n type INTEGER NOT NULL,\n text TEXT,\n comment TEXT,\n color TEXT,\n pageLabel TEXT,\n sortIndex TEXT NOT NULL,\n position TEXT NOT NULL,\n FOREIGN KEY (itemID) REFERENCES items(itemID) ON DELETE CASCADE,\n FOREIGN KEY (parentItemID) REFERENCES itemAttachments(itemID) ON DELETE CASCADE\n)"); yield Zotero.DB.queryAsync("CREATE INDEX itemAnnotations_parentItemID ON itemAnnotations(parentItemID)"); - - yield Zotero.DB.queryAsync("ALTER TABLE itemNotes ADD COLUMN schemaVersion INT NOT NULL DEFAULT 0"); } // If breaking compatibility or doing anything dangerous, clear minorUpdateFrom diff --git a/resource/schema/userdata.sql b/resource/schema/userdata.sql index 9eef22157e..703e1ec1a2 100644 --- a/resource/schema/userdata.sql +++ b/resource/schema/userdata.sql @@ -88,7 +88,6 @@ CREATE TABLE itemNotes ( parentItemID INT, note TEXT, title TEXT, - schemaVersion INT NOT NULL DEFAULT 0, FOREIGN KEY (itemID) REFERENCES items(itemID) ON DELETE CASCADE, FOREIGN KEY (parentItemID) REFERENCES items(itemID) ON DELETE CASCADE ); diff --git a/test/content/support.js b/test/content/support.js index 5d6c9f0d62..be060bcb2f 100644 --- a/test/content/support.js +++ b/test/content/support.js @@ -468,10 +468,7 @@ function createUnsavedDataObject(objectType, params = {}) { obj.setTags(params.tags); } if (params.note !== undefined) { - obj.setNote(params.note, params.noteSchemaVersion); - } - else if (itemType == 'note') { - obj.setNote(`

${Zotero.Utilities.randomString()}

`, params.noteSchemaVersion); + obj.setNote(params.note); } break; diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index a245610ab9..8ef711cbfb 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -854,30 +854,6 @@ describe("Zotero.Item", function () { }); }) - describe("#noteSchemaVersion", function () { - it("should be set to current schema version", async function () { - var note = await createDataObject('item', { itemType: 'note' }); - assert.equal(note.noteSchemaVersion, Zotero.Notes.schemaVersion); - }); - - it("should be set to an explicit value with setNote()", async function () { - var note = createUnsavedDataObject('item', { itemType: 'note' }); - note.setNote('
Foo
', 2); - await note.saveTx(); - assert.equal(note.noteSchemaVersion, 2); - }); - - it("shouldn't be settable to null", async function () { - var note = createUnsavedDataObject('item', { itemType: 'note' }); - assert.throws(() => note.setNote('
Foo
', null)); - }); - - it("shouldn't be settable to a numeric string", async function () { - var note = createUnsavedDataObject('item', { itemType: 'note' }); - assert.throws(() => note.setNote('
Foo
', "2")); - }); - }); - describe("#attachmentCharset", function () { it("should get and set a value", function* () { var charset = 'utf-8'; @@ -1583,13 +1559,6 @@ describe("Zotero.Item", function () { var newItem = item.clone(); assert.isEmpty(Object.keys(newItem.toJSON().relations)); }); - - it("should preserve noteSchemaVersion when set to a different version from the current version", async function () { - var oldVersion = Zotero.Notes.schemaVersion - 1; - var note = await createDataObject('item', { itemType: 'note', noteSchemaVersion: oldVersion }); - var newNote = note.clone(); - assert.equal(newNote.noteSchemaVersion, oldVersion); - }); }) describe("#moveToLibrary()", function () { @@ -1829,12 +1798,6 @@ describe("Zotero.Item", function () { var json = item.toJSON({ mode: 'full' }); assert.notProperty(json, "inPublications"); }); - - it("should include noteSchemaVersion", function () { - var note = createUnsavedDataObject('item', { itemType: 'note', noteSchemaVersion: 3 }); - var json = note.toJSON(); - assert.propertyVal(json, 'noteSchemaVersion', 3); - }); }) describe("'full' mode", function () { @@ -2380,17 +2343,6 @@ describe("Zotero.Item", function () { assert.equal(item.getField("bookTitle"), "Publication Title"); }); - it("should set note and noteSchemaField", function () { - var item = new Zotero.Item; - item.fromJSON({ - itemType: "note", - note: "

Foo

", - noteSchemaVersion: 3 - }); - assert.equal(item.note, "

Foo

"); - assert.equal(item.noteSchemaVersion, 3); - }); - it("should import annotation fields", async function () { var attachment = await importPDFAttachment(); diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 4b5cac3cd0..674cb526c6 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -2560,29 +2560,6 @@ describe("Zotero.Sync.Data.Local", function() { assert.lengthOf(result.conflicts, 1); }); - it("should ignore noteSchemaVersion=0 if no note", function () { - var json1 = { - key: "AAAAAAAA", - version: 1234, - title: "Link", - dateModified: "2017-04-02 12:34:56" - }; - var json2 = { - key: "AAAAAAAA", - version: 1235, - title: "Link", - note: "", - noteSchemaVersion: 0, - dateModified: "2017-04-02 12:34:56" - }; - var ignoreFields = ['dateAdded', 'dateModified']; - var result = Zotero.Sync.Data.Local._reconcileChangesWithoutCache( - 'item', json1, json2, ignoreFields - ); - assert.lengthOf(result.changes, 0); - assert.lengthOf(result.conflicts, 0); - }); - it("should automatically use remote version for conflicting fields when both sides are in trash", function () { var json1 = { key: "AAAAAAAA",