From 352f71e32e7a5283c7b44e3f096dd253891376d2 Mon Sep 17 00:00:00 2001
From: Abe Jellinek <jellinek@berkeley.edu>
Date: Thu, 29 Feb 2024 02:08:33 -0800
Subject: [PATCH] Fix zotero://open/ and FileHandlers regressions (#3761)

---
 chrome/content/zotero/xpcom/fileHandlers.js |  6 ++--
 components/zotero-protocol-handler.js       | 39 +++++++++++----------
 test/tests/fileHandlersTest.js              | 22 ++++++++++--
 3 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/chrome/content/zotero/xpcom/fileHandlers.js b/chrome/content/zotero/xpcom/fileHandlers.js
index 1acdfa2035..968aba8e2b 100644
--- a/chrome/content/zotero/xpcom/fileHandlers.js
+++ b/chrome/content/zotero/xpcom/fileHandlers.js
@@ -81,7 +81,7 @@ Zotero.FileHandlers = {
 			handlers = this._handlersLinux[readerType];
 		}
 		
-		let page = location?.position?.pageIndex ?? undefined;
+		let page = location?.pageIndex ?? undefined;
 		// Add 1 to page index for external readers
 		if (page !== undefined && parseInt(page) == page) {
 			page = parseInt(page) + 1;
@@ -492,9 +492,7 @@ Zotero.OpenPDF = {
 		await Zotero.FileHandlers.open(pathOrItem, {
 			location: {
 				annotationID: annotationKey,
-				position: {
-					pageIndex: page,
-				}
+				pageIndex: page,
 			}
 		});
 	}
diff --git a/components/zotero-protocol-handler.js b/components/zotero-protocol-handler.js
index be8c21c4ee..d3ebee3afc 100644
--- a/components/zotero-protocol-handler.js
+++ b/components/zotero-protocol-handler.js
@@ -1193,32 +1193,33 @@ function ZoteroProtocolHandler() {
 				return;
 			}
 			
-			var location = null;
+			var location = {};
+			
 			if (page) {
-				location = {
-					position: {
-						pageIndex: page
-					},
-					annotationID: annotation
-				};
+				location.pageIndex = parseInt(page);
 			}
-			else if (cfi) {
-				location = {
-					position: {
-						type: 'FragmentSelector',
-						conformsTo: 'http://www.idpf.org/epub/linking/cfi/epub-cfi.html',
-						value: cfi
-					}
+			if (annotation) {
+				location.annotationID = annotation;
+			}
+			
+			if (cfi) {
+				location.position = {
+					type: 'FragmentSelector',
+					conformsTo: 'http://www.idpf.org/epub/linking/cfi/epub-cfi.html',
+					value: cfi
 				};
 			}
 			else if (sel) {
-				location = {
-					position: {
-						type: 'CssSelector',
-						value: sel
-					}
+				location.position = {
+					type: 'CssSelector',
+					value: sel
 				};
 			}
+			
+			// Don't pass empty location
+			if (!Object.keys(location).length) {
+				location = null;
+			}
 
 			var openInWindow = Zotero.Prefs.get('openReaderInNewWindow');
 
diff --git a/test/tests/fileHandlersTest.js b/test/tests/fileHandlersTest.js
index 0db8b8d056..cf5c9094b7 100644
--- a/test/tests/fileHandlersTest.js
+++ b/test/tests/fileHandlersTest.js
@@ -29,15 +29,31 @@ describe("Zotero.FileHandlers", () => {
 		it("should open a PDF internally when no handler is set", async function () {
 			let pdf = await importFileAttachment('wonderland_short.pdf');
 			await Zotero.FileHandlers.open(pdf, {
-				location: { position: { pageIndex: 2 } }
+				location: { pageIndex: 2 }
 			});
-			assert.ok(Zotero.Reader.getByTabID(win.Zotero_Tabs.selectedID));
+			let reader = Zotero.Reader.getByTabID(win.Zotero_Tabs.selectedID);
+			assert.ok(reader);
+			
+			// Wait for _setState() (is there an easier way to do this?)
+			let stub = sinon.stub(reader, '_setState');
+			let setStatePromise = new Promise((resolve) => {
+				stub.callsFake(async (...args) => {
+					await stub.wrappedMethod.apply(reader, args);
+					resolve();
+				});
+			});
+			await reader._waitForReader();
+			await setStatePromise;
+			
+			// Check that the reader navigated to the correct page
+			assert.equal(pdf.getAttachmentLastPageIndex(), 2);
+			stub.restore();
 		});
 
 		it("should open a PDF in a new window when no handler is set and openInWindow is passed", async function () {
 			let pdf = await importFileAttachment('wonderland_short.pdf');
 			await Zotero.FileHandlers.open(pdf, {
-				location: { position: { pageIndex: 2 } },
+				location: { pageIndex: 2 },
 				openInWindow: true
 			});
 			assert.notOk(Zotero.Reader.getByTabID(win.Zotero_Tabs.selectedID));