From 2536edb6abf97d10db09f8c1d862b2938f8c9cb8 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 10 Sep 2020 06:13:51 -0400 Subject: [PATCH] Fix annotation 'position' handling and update additional sortIndex tests Item.position has to be a string. It still gets passed to/from the PDF reader as an object. --- chrome/content/zotero/xpcom/annotations.js | 2 +- chrome/content/zotero/xpcom/data/item.js | 13 +++++---- test/content/support.js | 6 ++-- test/tests/annotationsTest.js | 32 +++++++++++++++------- test/tests/itemTest.js | 28 +++++++++---------- 5 files changed, 46 insertions(+), 35 deletions(-) diff --git a/chrome/content/zotero/xpcom/annotations.js b/chrome/content/zotero/xpcom/annotations.js index 0254cc521e..edc1b2f32e 100644 --- a/chrome/content/zotero/xpcom/annotations.js +++ b/chrome/content/zotero/xpcom/annotations.js @@ -118,7 +118,7 @@ Zotero.Annotations = new function () { item.annotationColor = json.color; item.annotationPageLabel = json.pageLabel; item.annotationSortIndex = json.sortIndex; - item.annotationPosition = Object.assign({}, json.position); + item.annotationPosition = JSON.stringify(Object.assign({}, json.position)); // TODO: Can colors be set? item.setTags((json.tags || []).map(t => ({ tag: t.name }))); diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 7853622a54..5dedf92ea0 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -1801,10 +1801,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { let pageLabel = this._getLatestField('annotationPageLabel'); let sortIndex = this._getLatestField('annotationSortIndex'); let position = this._getLatestField('annotationPosition'); - // This gets stringified, so make sure it's not null - if (!position) { - throw new Error("Annotation position not set"); - } let sql = "REPLACE INTO itemAnnotations " + "(itemID, parentItemID, type, text, comment, color, pageLabel, sortIndex, position) " @@ -1820,7 +1816,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { color || null, pageLabel || null, sortIndex, - JSON.stringify(position) + position ] ); @@ -3565,6 +3561,11 @@ for (let name of ['position']) { }, set: function (value) { this._requireData('annotationDeferred'); + + if (typeof value != 'string') { + throw new Error(`${field} must be a string`); + } + if (this._getLatestField(field) === value) { return; } @@ -4981,7 +4982,7 @@ Zotero.Item.prototype.toJSON = function (options = {}) { obj.annotationColor = this.annotationColor || ''; obj.annotationPageLabel = this.annotationPageLabel || ''; obj.annotationSortIndex = this.annotationSortIndex || ''; - obj.annotationPosition = JSON.stringify(this.annotationPosition) || ''; + obj.annotationPosition = this.annotationPosition || ''; } } diff --git a/test/content/support.js b/test/content/support.js index 0ad8ffe460..5d6c9f0d62 100644 --- a/test/content/support.js +++ b/test/content/support.js @@ -948,13 +948,13 @@ async function createAnnotation(type, parentItem, options = {}) { annotation.annotationComment = Zotero.Utilities.randomString(); var page = Zotero.Utilities.rand(1, 100).toString().padStart(5, '0'); var pos = Zotero.Utilities.rand(1, 10000).toString().padStart(6, '0'); - annotation.annotationSortIndex = `${page}|${pos}|00000.000`; - annotation.annotationPosition = { + annotation.annotationSortIndex = `${page}|${pos}|00000`; + annotation.annotationPosition = JSON.stringify({ pageIndex: 123, rects: [ [314.4, 412.8, 556.2, 609.6] ] - }; + }); if (options.tags) { annotation.setTags(options.tags); } diff --git a/test/tests/annotationsTest.js b/test/tests/annotationsTest.js index ceb9c7923b..f436021d91 100644 --- a/test/tests/annotationsTest.js +++ b/test/tests/annotationsTest.js @@ -29,6 +29,7 @@ describe("Zotero.Annotations", function() { ], "dateModified": "2019-05-14 06:50:40" }; + var exampleHighlightAlt = jsonPositionToString(exampleHighlight); var exampleNote = { "key": "5TKU34XX", @@ -46,6 +47,7 @@ describe("Zotero.Annotations", function() { }, "dateModified": "2019-05-14 06:50:54" }; + var exampleNoteAlt = jsonPositionToString(exampleNote); var exampleImage = { "key": "QD32MQJF", @@ -66,6 +68,7 @@ describe("Zotero.Annotations", function() { }, "dateModified": "2019-05-14 06:51:22" }; + var exampleImageAlt = jsonPositionToString(exampleImage); var exampleGroupHighlight = { "key": "PE57YAYH", @@ -89,6 +92,15 @@ describe("Zotero.Annotations", function() { }, "dateModified": "2019-05-14 06:50:40" }; + var exampleGroupHighlightAlt = jsonPositionToString(exampleGroupHighlight); + + // Item.position is a string, so when using the annotation JSON as input or when comparing we + // have to use a version where 'position' has been stringified + function jsonPositionToString(json) { + var o = Object.assign({}, json); + o.position = JSON.stringify(o.position); + return o; + } var item; var attachment; @@ -118,7 +130,7 @@ describe("Zotero.Annotations", function() { annotation.annotationType = 'highlight'; for (let prop of ['text', 'comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - annotation[itemProp] = exampleHighlight[prop]; + annotation[itemProp] = exampleHighlightAlt[prop]; } annotation.addTag("math"); annotation.addTag("chemistry"); @@ -131,7 +143,7 @@ describe("Zotero.Annotations", function() { if (prop == 'dateModified') { continue; } - assert.deepEqual(json[prop], exampleHighlight[prop], `'${prop}' doesn't match`); + assert.deepEqual(json[prop], exampleHighlightAlt[prop], `'${prop}' doesn't match`); } await annotation.eraseTx(); @@ -146,7 +158,7 @@ describe("Zotero.Annotations", function() { annotation.annotationType = 'note'; for (let prop of ['comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - annotation[itemProp] = exampleNote[prop]; + annotation[itemProp] = exampleNoteAlt[prop]; } await annotation.saveTx(); var json = Zotero.Annotations.toJSON(annotation); @@ -156,7 +168,7 @@ describe("Zotero.Annotations", function() { if (prop == 'dateModified') { continue; } - assert.deepEqual(json[prop], exampleNote[prop], `'${prop}' doesn't match`); + assert.deepEqual(json[prop], exampleNoteAlt[prop], `'${prop}' doesn't match`); } await annotation.eraseTx(); @@ -171,7 +183,7 @@ describe("Zotero.Annotations", function() { annotation.annotationType = 'image'; for (let prop of ['comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - annotation[itemProp] = exampleImage[prop]; + annotation[itemProp] = exampleImageAlt[prop]; } await annotation.saveTx(); @@ -195,7 +207,7 @@ describe("Zotero.Annotations", function() { || prop == 'dateModified') { continue; } - assert.deepEqual(json[prop], exampleImage[prop], `'${prop}' doesn't match`); + assert.deepEqual(json[prop], exampleImageAlt[prop], `'${prop}' doesn't match`); } assert.equal(json.imageURL, `zotero://attachment/library/items/${imageAttachment.key}`); @@ -214,7 +226,7 @@ describe("Zotero.Annotations", function() { annotation.annotationType = 'highlight'; for (let prop of ['text', 'comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - annotation[itemProp] = exampleGroupHighlight[prop]; + annotation[itemProp] = exampleGroupHighlightAlt[prop]; } await annotation.saveTx(); var json = Zotero.Annotations.toJSON(annotation); @@ -234,7 +246,7 @@ describe("Zotero.Annotations", function() { assert.equal(annotation.key, exampleHighlight.key); for (let prop of ['text', 'comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - assert.deepEqual(annotation[itemProp], exampleHighlight[prop], `'${prop}' doesn't match`); + assert.deepEqual(annotation[itemProp], exampleHighlightAlt[prop], `'${prop}' doesn't match`); } var itemTags = annotation.getTags().map(t => t.tag); var jsonTags = exampleHighlight.tags.map(t => t.name); @@ -247,7 +259,7 @@ describe("Zotero.Annotations", function() { assert.equal(annotation.key, exampleNote.key); for (let prop of ['comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - assert.deepEqual(annotation[itemProp], exampleNote[prop], `'${prop}' doesn't match`); + assert.deepEqual(annotation[itemProp], exampleNoteAlt[prop], `'${prop}' doesn't match`); } }); @@ -259,7 +271,7 @@ describe("Zotero.Annotations", function() { assert.equal(annotation.key, exampleImage.key); for (let prop of ['comment', 'color', 'pageLabel', 'sortIndex', 'position']) { let itemProp = 'annotation' + prop[0].toUpperCase() + prop.substr(1); - assert.deepEqual(annotation[itemProp], exampleImage[prop], `'${prop}' doesn't match`); + assert.deepEqual(annotation[itemProp], exampleImageAlt[prop], `'${prop}' doesn't match`); } }); }); diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index ddfda28404..a245610ab9 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1223,13 +1223,13 @@ describe("Zotero.Item", function () { annotation.parentID = attachment.id; annotation.annotationType = 'highlight'; annotation.annotationText = "This is highlighted text."; - annotation.annotationSortIndex = '00015|002431|00000.000'; - annotation.annotationPosition = { + annotation.annotationSortIndex = '00015|002431|00000'; + annotation.annotationPosition = JSON.stringify({ pageIndex: 123, rects: [ [314.4, 412.8, 556.2, 609.6] ] - }; + }); await annotation.saveTx(); }); @@ -1238,13 +1238,13 @@ describe("Zotero.Item", function () { annotation.parentID = attachment.id; annotation.annotationType = 'note'; annotation.annotationComment = "This is a comment."; - annotation.annotationSortIndex = '00015|002431|00000.000'; - annotation.annotationPosition = { + annotation.annotationSortIndex = '00015|002431|00000'; + annotation.annotationPosition = JSON.stringify({ pageIndex: 123, rects: [ [314.4, 412.8, 556.2, 609.6] ] - }; + }); await annotation.saveTx(); }); @@ -1260,15 +1260,15 @@ describe("Zotero.Item", function () { var annotation = new Zotero.Item('annotation'); annotation.parentID = attachment.id; annotation.annotationType = 'image'; - annotation.annotationSortIndex = '00015|002431|00000.000'; - annotation.annotationPosition = { + annotation.annotationSortIndex = '00015|002431|00000'; + annotation.annotationPosition = JSON.stringify({ pageIndex: 123, rects: [ [314.4, 412.8, 556.2, 609.6] ], width: 1, height: 1 - }; + }); await annotation.saveTx(); await Zotero.Attachments.importEmbeddedImage({ @@ -1781,8 +1781,8 @@ describe("Zotero.Item", function () { item.annotationComment = "This is a comment with rich-text\nAnd a new line"; item.annotationColor = "#ffec00"; item.annotationPageLabel = "15"; - item.annotationSortIndex = "00015|002431|00000.000"; - item.annotationPosition = { + item.annotationSortIndex = "00015|002431|00000"; + item.annotationPosition = JSON.stringify({ "pageIndex": 1, "rects": [ [231.284, 402.126, 293.107, 410.142], @@ -1791,16 +1791,14 @@ describe("Zotero.Item", function () { [54.222, 372.238, 293.107, 380.254], [54.222, 362.276, 273.955, 370.292] ] - }; + }); var json = item.toJSON(); - Zotero.debug(json); - for (let prop of ['Type', 'Text', 'Comment', 'Color', 'PageLabel', 'SortIndex']) { let name = 'annotation' + prop; assert.propertyVal(json, name, item[name]); } - assert.deepEqual(JSON.parse(json.annotationPosition), item.annotationPosition); + assert.deepEqual(json.annotationPosition, item.annotationPosition); assert.notProperty(json, 'collections'); assert.notProperty(json, 'relations'); });