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

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

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 || '';
 		}
 	}
 	

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.annotationSortIndex = `${page}|${pos}|00000`;
-	annotation.annotationPosition = {
+	annotation.annotationPosition = JSON.stringify({
 		pageIndex: 123,
 		rects: [
 			[314.4, 412.8, 556.2, 609.6]
 		]
-	};
+	});
 	if (options.tags) {
 		annotation.setTags(options.tags);
 	}

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

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.annotationSortIndex = '00015|002431|00000';
-				annotation.annotationPosition = {
+				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.annotationSortIndex = '00015|002431|00000';
-				annotation.annotationPosition = {
+				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.annotationSortIndex = '00015|002431|00000';
-				annotation.annotationPosition = {
+				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 <i>rich-text</i>\nAnd a new line";
 					item.annotationColor = "#ffec00";
 					item.annotationPageLabel = "15";
-					item.annotationSortIndex = "00015|002431|00000.000";
+					item.annotationSortIndex = "00015|002431|00000";
-					item.annotationPosition = {
+					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');
 				});