Fix note tab reopen logic (#5706)

fix: #5704
Distinguish note editor CE and instance in var name
Add note editor CE _initPromise
Await CE init in various places
Add tests for note tab

chrome/content/zotero/elements/noteEditor.js

@@ -36,6 +36,10 @@
 			this._parentItem = null;
 			this._iframe = null;
 			this._initialized = false;
+			this._initPromise = new Promise((resolve, reject) => {
+				this._resolveInitPromise = resolve;
+				this._rejectInitPromise = reject;
+			});
 			this._editorInstance = null;
 			this._destroyed = false;
 			this._bottomPlaceholder = null;
@@ -154,6 +158,9 @@
 				this._onInitCallback();
 			}
 			requestIdleCallback(() => this.setToggleContextPaneButtonMode());
+
+			await this._editorInstance._initPromise;
+			this._resolveInitPromise();
 		};
 
 		onInit = (callback) => {

chrome/content/zotero/tabs.js

@@ -110,14 +110,14 @@ var Zotero_Tabs = new function () {
 				await reader._initPromise;
 			},
 			note: async (tab, tabIndex, options) => {
-				let noteEditor = await Zotero.Notes.open(tab.data.itemID, options && options.location, {
+				let editorInstance = await Zotero.Notes.open(tab.data.itemID, options && options.location, {
 					tabID: tab.id,
 					title: tab.title,
 					tabIndex,
 					allowDuplicate: true,
 					preventJumpback: true
 				});
-				await noteEditor;
+				await editorInstance._initPromise;
 			}
 		},
 		focusFirst: {

chrome/content/zotero/xpcom/data/notes.js

@@ -35,6 +35,14 @@ Zotero.Notes = new function () {
 	this._editorInstances = [];
 	this._downloadInProgressPromise = null;
 
+	/**
+	 * Open note in tab or window
+	 * @param {number} itemID
+	 * @param {Object} location - Not implemented yet
+	 * @param {Object} options
+	 * @returns {Promise<Zotero.EditorInstance | null>} Instance of Zotero.EditorInstance for the note.
+	 * If the note tab is opened in background (unloaded), returns null.
+	 */
 	this.open = async function (itemID, location, { title, tabIndex, tabID, openInBackground, openInWindow, allowDuplicate, preventJumpback, parentItemKey } = {}) {
 		let { libraryID } = Zotero.Items.getLibraryAndKeyFromID(itemID);
 		let library = Zotero.Libraries.get(libraryID);
@@ -44,6 +52,11 @@ Zotero.Notes = new function () {
 			openInWindow = true;
 		}
 
+		// Never duplicate when opening in window
+		if (openInWindow) {
+			allowDuplicate = false;
+		}
+
 		await library.waitForDataLoad('item');
 
 		let item = Zotero.Items.get(itemID);
@@ -51,37 +64,52 @@ Zotero.Notes = new function () {
 			throw new Error('Item does not exist');
 		}
 
+		// Instance of EditorInstance
+		let editorInstance;
+		// Instance of note-editor CE
 		let noteEditor;
-		if (!openInWindow && !allowDuplicate && !this._editorInstances.find(r => r.itemID === itemID)) {
-			if (win) {
-				let existingTabID = win.Zotero_Tabs.getTabIDByItemID(itemID);
-				if (existingTabID) {
-					win.Zotero_Tabs.select(existingTabID, false, { location });
-					return win.Zotero_Tabs.getTabContent(existingTabID).querySelector('note-editor.note-tab');
+
+		if (!allowDuplicate) {
+			if (openInWindow) {
+				editorInstance = this._editorInstances.find(r => r.itemID === itemID && r.viewMode === 'window');
+			}
+			else {
+				editorInstance = this._editorInstances.find(r => r.itemID === itemID && r.viewMode === 'tab');
+			}
+		}
+
+		if (win && !openInWindow && !allowDuplicate && !editorInstance) {
+			let existingTabID = win.Zotero_Tabs.getTabIDByItemID(itemID);
+			if (existingTabID) {
+				win.Zotero_Tabs.select(existingTabID, false, { location });
+				// Wait for the note editor to load
+				let timeout = 3000;
+				for (let i = 0; i < timeout; i += 100) {
+					editorInstance = this._editorInstances.find(r => r.itemID === itemID && r.viewMode === 'tab');
+					if (editorInstance) {
+						return editorInstance;
+					}
+					await new Promise(resolve => setTimeout(resolve, 100));
+				}
+				if (!editorInstance) {
+					throw new Error('Timed out waiting for note editor to load');
 				}
 			}
 		}
 
-		if (openInWindow) {
-			noteEditor = this._editorInstances.find(r => r.itemID === itemID && r.viewMode === 'window');
-		}
-		else if (!allowDuplicate) {
-			noteEditor = this._editorInstances.find(r => r.itemID === itemID && r.viewMode === 'tab');
-		}
-
-		if (noteEditor) {
-			if (noteEditor.viewMode === 'tab') {
-				win.Zotero_Tabs.select(noteEditor.tabID, true);
+		if (editorInstance) {
+			if (openInWindow) {
+				editorInstance.focus();
 			}
 			else {
-				noteEditor.focus();
+				win.Zotero_Tabs.select(editorInstance.tabID, true);
 			}
 			
 			if (location) {
 				// TODO: implement this
-				noteEditor.navigate(location);
+				editorInstance.navigate(location);
 			}
-			return noteEditor;
+			return editorInstance;
 		}
 
 		if (openInWindow) {
@@ -129,7 +157,11 @@ Zotero.Notes = new function () {
 				}));
 			}
 
-			if (!noteEditor && !openInBackground) {
+			if (openInBackground) {
+				return null;
+			}
+
+			if (!noteEditor) {
 				noteEditor = win.document.createXULElement('note-editor');
 				noteEditor.classList.add('note-tab');
 				container.appendChild(noteEditor);
@@ -160,7 +192,8 @@ Zotero.Notes = new function () {
 				noteEditor.focus();
 			}
 		}
-		return noteEditor;
+		await noteEditor._initPromise;
+		return noteEditor._editorInstance;
 	};
 
 	this.setBottomPlaceholderHeight = function (noteEditor, height) {
@@ -191,9 +224,9 @@ Zotero.Notes = new function () {
 		if (!tabID) {
 			return null;
 		}
-		let noteEditor = this._editorInstances.find(x => x._tabID === tabID);
-		if (noteEditor) {
-			return noteEditor;
+		let editorInstance = this._editorInstances.find(x => x.tabID === tabID);
+		if (editorInstance) {
+			return editorInstance;
 		}
 		return null;
 	};

chrome/content/zotero/xpcom/editorInstance.js

@@ -61,6 +61,10 @@ class EditorInstance {
 		this._viewMode = mode;
 	}
 
+	get tabID() {
+		return this._tabID;
+	}
+
 	async init(options) {
 		Zotero.Notes.registerEditorInstance(this);
 		this.onNavigate = options.onNavigate;

test/tests/noteTabTest.js

@@ -25,31 +25,73 @@ describe("Note Tab", function () {
 			item.setNote('This is a test note.');
 			await item.saveTx();
 
-			let noteEditor = await Zotero.Notes.open(item.id);
+			let editorInstance = await Zotero.Notes.open(item.id);
 
-			assert.isNotNull(noteEditor, "Note editor should be opened");
-			assert.equal(noteEditor.item.id, item.id, "Note editor should be associated with the correct item");
+			assert.isNotNull(editorInstance, "Note editor should be opened");
+			assert.equal(editorInstance.itemID, item.id, "Note editor should be associated with the correct item");
 
 			let sameNoteEditor = await Zotero.Notes.open(item.id, undefined, {
 				tabID: Zotero_Tabs.selectedID,
 			});
-			assert.equal(noteEditor, sameNoteEditor, "Opening the same note should return the existing editor");
+			assert.equal(editorInstance, sameNoteEditor, "Opening the same note should return the existing editor");
 
-			let duplicateNoteEditor = await Zotero.Notes.open(item.id, undefined, {
+			let duplicateEditorInstance = await Zotero.Notes.open(item.id, undefined, {
 				allowDuplicate: true,
 			});
 
-			assert.isNotNull(duplicateNoteEditor, "Duplicate note editor should be opened");
-			assert.notEqual(noteEditor, duplicateNoteEditor, "Duplicate note editor should be a new instance");
-			assert.equal(duplicateNoteEditor.item.id, item.id, "Duplicate note editor should be associated with the correct item");
+			assert.isNotNull(duplicateEditorInstance, "Duplicate note editor should be opened");
+			assert.notEqual(editorInstance, duplicateEditorInstance, "Duplicate note editor should be a new instance");
+			assert.equal(duplicateEditorInstance.itemID, item.id, "Duplicate note editor should be associated with the correct item");
 
 			Zotero_Tabs.closeAll();
 
+			await waitForCallback(
+				() => !Zotero.Notes._editorInstances.find(e => e.tabID),
+				100, 10);
+
 			await Zotero.Notes.open(item.id, undefined, {
 				openInBackground: true,
 			});
 
 			assert.equal(Zotero_Tabs.selectedType, 'library', "Tab should be opened in background");
 		});
+
+		it("should open unloaded note tab", async function () {
+			// https://forums.zotero.org/discussion/128954/
+			let item = new Zotero.Item("note");
+			item.setNote("This is a test note.");
+			await item.saveTx();
+
+			let editorInstance = await Zotero.Notes.open(item.id);
+			let tabID = editorInstance.tabID;
+			Zotero_Tabs.unload(tabID);
+
+			let editor2 = await ZoteroPane.openNote(item.id);
+
+			assert.equal(editorInstance, editor2, "Unloaded note tab should be reloaded");
+		});
+
+		it("should select opened note tab", async function () {
+			// https://forums.zotero.org/discussion/128917/
+			let item = new Zotero.Item("note");
+			item.setNote("This is a test note.");
+			await item.saveTx();
+
+			let editorInstance = await Zotero.Notes.open(item.id);
+			let tabID = editorInstance.tabID;
+
+			let promise = waitForNotifierEvent("select", "tab");
+			
+			Zotero_Tabs.select("zotero-pane");
+			await promise;
+
+			promise = waitForNotifierEvent("select", "tab");
+
+			let editor2 = await ZoteroPane.openNote(item.id);
+			await promise;
+
+			assert.equal(Zotero_Tabs.selectedID, tabID, "Should select the opened note tab");
+			assert.equal(editor2, editorInstance, "Should return the same editor instance");
+		});
 	});
 });