From 69958d43569de1cb53200df2b9064ca85e5a7451 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Sat, 26 Dec 2020 02:32:11 -0500
Subject: [PATCH] Return Zotero.Item objects from getAnnotations() instead of
 ids

Returning ids mirrors getAttachments() and getNotes(), but I think we
want to move towards a world where those return actual objects, even if
we need async load calls on the returned objects.
---
 chrome/content/zotero/xpcom/data/item.js        |  4 ++--
 .../content/zotero/xpcom/pdfWorker/manager.js   | 10 ++++------
 chrome/content/zotero/xpcom/reader.js           | 17 ++++++++---------
 test/tests/itemTest.js                          | 12 ++++++------
 4 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 4256d45386..923961113b 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -3670,7 +3670,7 @@ Zotero.Item.prototype.getAnnotations = function (includeTrashed) {
 	var cacheKey = 'with' + (includeTrashed ? '' : 'out') + 'Trashed';
 	
 	if (this._annotations[cacheKey]) {
-		return [...this._annotations[cacheKey]];
+		return Zotero.Items.get([...this._annotations[cacheKey]]);
 	}
 	
 	var rows = this._annotations.rows.concat();
@@ -3680,7 +3680,7 @@ Zotero.Item.prototype.getAnnotations = function (includeTrashed) {
 	}
 	var ids = rows.map(row => row.itemID);
 	this._annotations[cacheKey] = ids;
-	return ids;
+	return Zotero.Items.get(ids);
 };
 
 
diff --git a/chrome/content/zotero/xpcom/pdfWorker/manager.js b/chrome/content/zotero/xpcom/pdfWorker/manager.js
index 6e1535963a..520044f1b2 100644
--- a/chrome/content/zotero/xpcom/pdfWorker/manager.js
+++ b/chrome/content/zotero/xpcom/pdfWorker/manager.js
@@ -157,10 +157,9 @@ class PDFWorker {
 			if (!this.isPDFAttachment(attachment)) {
 				throw new Error('not a valid attachment');
 			}
-			let ids = attachment.getAnnotations();
+			let items = attachment.getAnnotations();
 			let annotations = [];
-			for (let id of ids) {
-				let item = await Zotero.Items.getAsync(id);
+			for (let item of items) {
 				annotations.push({
 					id: item.key,
 					type: item.annotationType,
@@ -222,10 +221,9 @@ class PDFWorker {
 			}
 			// TODO: Remove when fixed
 			attachment._loaded.childItems = true;
-			let ids = attachment.getAnnotations();
+			let items = attachment.getAnnotations();
 			let existingAnnotations = [];
-			for (let id of ids) {
-				let item = await Zotero.Items.getAsync(id);
+			for (let item of items) {
 				existingAnnotations.push({
 					id: item.key,
 					type: item.annotationType,
diff --git a/chrome/content/zotero/xpcom/reader.js b/chrome/content/zotero/xpcom/reader.js
index eabec4cee0..c0d5184322 100644
--- a/chrome/content/zotero/xpcom/reader.js
+++ b/chrome/content/zotero/xpcom/reader.js
@@ -37,9 +37,9 @@ class ReaderInstance {
 		buf = new Uint8Array(buf).buffer;
 		// TODO: Remove when fixed
 		item._loaded.childItems = true;
-		let ids = item.getAnnotations();
-		let annotations = (await Promise.all(ids.map(id => this._getAnnotation(id)))).filter(x => x);
-		this.annotationItemIDs = ids;
+		let annotationItems = item.getAnnotations();
+		let annotations = (await Promise.all(annotationItems.map(x => this._getAnnotation(x)))).filter(x => x);
+		this.annotationItemIDs = annotationItems.map(x => x.id);
 		state = state || await this._loadState();
 		this._state = state;
 		this._postMessage({
@@ -439,9 +439,8 @@ class ReaderInstance {
 	 * @param itemID
 	 * @returns {Object|null}
 	 */
-	async _getAnnotation(itemID) {
+	async _getAnnotation(item) {
 		try {
-			let item = Zotero.Items.get(itemID);
 			if (!item || !item.isAnnotation()) {
 				return null;
 			}
@@ -704,10 +703,10 @@ class Reader {
 				let item = Zotero.Items.get(readerWindow._itemID);
 				// TODO: Remove when fixed
 				item._loaded.childItems = true;
-				let annotationItemIDs = item.getAnnotations();
-				readerWindow.annotationItemIDs = annotationItemIDs;
-				let affectedAnnotationIds = annotationItemIDs.filter(annotationID => {
-					let annotation = Zotero.Items.get(annotationID);
+				let annotationItems = item.getAnnotations();
+				readerWindow.annotationItemIDs = annotationItems.map(x => x.id);
+				let affectedAnnotationIds = annotationItems.filter(annotation => {
+					let annotationID = annotation.id;
 					let imageAttachmentID = null;
 					annotation._loaded.childItems = true;
 					let annotationAttachments = annotation.getAttachments();
diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js
index be9f01f746..a584c58695 100644
--- a/test/tests/itemTest.js
+++ b/test/tests/itemTest.js
@@ -1341,14 +1341,14 @@ describe("Zotero.Item", function () {
 				await annotation2.saveTx();
 			});
 			
-			it("should return ids of annotations not in trash", async function () {
-				var ids = attachment.getAnnotations();
-				assert.sameMembers(ids, [annotation1.id]);
+			it("should return annotations not in trash", async function () {
+				var items = attachment.getAnnotations();
+				assert.sameMembers(items, [annotation1]);
 			});
 			
-			it("should return ids of annotations in trash if includeTrashed=true", async function () {
-				var ids = attachment.getAnnotations(true);
-				assert.sameMembers(ids, [annotation1.id, annotation2.id]);
+			it("should return annotations in trash if includeTrashed=true", async function () {
+				var items = attachment.getAnnotations(true);
+				assert.sameMembers(items, [annotation1, annotation2]);
 			});
 		});
 	});