Refactor AttachmentPreview render and discard (#4161)

chrome/content/zotero/elements/attachmentBox.js

@@ -291,15 +291,10 @@
 		}
 
 		notify(event, _type, ids, _extraData) {
-			if (event != 'modify' || !this.item || !this.item.id) return;
+			if (event != 'modify' || !this.item?.id || !ids.includes(this.item.id)) return;
-			for (let id of ids) {
+			
-				if (id != this.item.id) {
+			// Wait for the render finish and then refresh
-					continue;
+			this._waitForRender(this._forceRenderAll.bind(this));
-				}
-				
-				this._forceRenderAll();
-				break;
-			}
 		}
 
 		async asyncRender() {
@@ -312,11 +307,6 @@
 			this._asyncRendering = true;
 			// Cancel editing filename when refreshing
 			this._isEditingFilename = false;
-
-			if (this.usePreview) {
-				this._preview.item = this.item;
-				this._preview.render();
-			}
 			
 			let fileNameRow = this._id('fileNameRow');
 			let urlField = this._id('url');
@@ -457,6 +447,12 @@
 			else {
 				selectButton.hidden = true;
 			}
+
+			if (this.usePreview) {
+				this._preview.item = this.item;
+				await this._preview.render();
+			}
+
 			this._asyncRendering = false;
 		}
 
@@ -658,6 +654,27 @@
 		_id(id) {
 			return this.querySelector(`#${id}`);
 		}
+
+		async _waitForRender(callback) {
+			let resolve, reject;
+			Promise.race([new Promise(((res, rej) => {
+				resolve = res;
+				reject = rej;
+			})), Zotero.Promise.delay(3000)]).then(() => callback());
+			let i = 0;
+			let finished = false;
+			// Wait for render to finish
+			while (i < 100) {
+				if (!this._asyncRendering) {
+					finished = true;
+					break;
+				}
+				await Zotero.Promise.delay(10);
+				i++;
+			}
+			if (finished) resolve();
+			else reject(new Error("AttachmentBox#_waitForRender timeout"));
+		}
 	}
 
 	customElements.define("attachment-box", AttachmentBox);

chrome/content/zotero/elements/attachmentPreview.js

@@ -24,6 +24,12 @@
 */
 
 {
+	class PreviewRenderAbortError extends Error {
+		constructor() {
+			super("AttachmentPreview render aborted");
+		}
+	}
+
 	class AttachmentPreview extends ItemPaneSectionElementBase {
 		static fileTypeMap = {
 			// TODO: support video and audio
@@ -42,11 +48,52 @@
 			this._previewInitializePromise = Zotero.Promise.defer();
 			this._nextPreviewInitializePromise = Zotero.Promise.defer();
 
-			this._renderingItemID = null;
+			/**
+			 * The most recent task to be processed
+			 * @type {Object}
+			 * @property {string} type
+			 * @property {Object} data
+			 * @property {number} data.itemID
+			 * @property {string} data.previewType
+			 */
+			this._lastTask = null;
 
-			this._isDiscardPlanned = false;
+			/**
+			 * The ID of the last item that was rendered
+			 * @type {number}
+			 */
+			this._lastRenderID = null;
+
+			/**
+			 * Whether a task is currently awaiting to be processed
+			 * @type {boolean}
+			 */
+			this._isWaitingForTask = false;
+
+			/**
+			 * Whether a task is currently being processed
+			 * @type {boolean}
+			 */
+			this._isProcessingTask = false;
+
+			/**
+			 * Whether a render task is currently being processed
+			 * @type {boolean}
+			 */
+			this._isRendering = false;
+
+			/**
+			 * Whether a discard task is currently being processed
+			 * @type {boolean}
+			 */
 			this._isDiscarding = false;
-			this._failedCount = 0;
+
+			/**
+			 * Whether the current preview reader is initialized by `Zotero.Reader.openPreview`.
+			 * When the previous reader rendering task is aborted before initialization,
+			 * reuse the reader; otherwise must discard the old reader first.
+			 */
+			this._isReaderInitialized = false;
 
 			this._resizeOb = new ResizeObserver(this._handleResize.bind(this));
 		}
@@ -207,59 +254,182 @@
 			}
 			if (this.isMediaType) {
 				if (["refresh", "modify"].includes(event) && ids.includes(this.item.id)) {
-					this.discard().then(() => this.render());
+					this.render();
 				}
 			}
 		}
 
+		/**
+		 * Queue a render task
+		 * Immediately update the `_lastTask` property and wait for the current task to finish
+		 * before processing the new task. This is to prevent multiple tasks from being processed
+		 * at the same time. Only the most recent task will be processed.
+		 * @returns {Promise<void>}
+		 */
 		async render() {
-			let itemID = this._item?.id;
+			this._lastTask = {
-			if (!this.initialized && itemID === this._renderingItemID) {
+				type: "render",
+				uid: `${Date.now()}-${Math.random()}`,
+				data: {
+					itemID: this._item?.id,
+					previewType: this.previewType
+				}
+			};
+			this._debug(`Queue render task, itemID: ${this._item?.id}, previewType: ${this.previewType}`);
+			await this._processTask();
+		}
+
+		/**
+		 * Queue a discard task
+		 */
+		async discard() {
+			this._lastTask = {
+				type: "discard",
+				uid: `${Date.now()}-${Math.random()}`,
+			};
+			this._debug(`Queue discard task`);
+			await this._processTask();
+		}
+
+		/**
+		 * Process the most recent task
+		 * @returns {Promise<void>}
+		 */
+		async _processTask() {
+			if (!this.initialized || !this._lastTask || this._isWaitingForTask) {
+				this._debug("No task to process or already waiting for a processing task");
 				return;
 			}
-			// For tests
-			let resolve;
-			if (Zotero.test) {
-				this._renderPromise = new Promise(r => resolve = r);
-				// Expose `resolve` for `this.discard`
-				this._renderPromise.resolve = resolve;
-			}
 
-			this._renderingItemID = itemID;
+			this._isWaitingForTask = true;
-			let success = false;
+
-			if (this.isValidType && await this._item.fileExists()) {
+			// Wait for the current render/discard to finish
-				if (this.isReaderType) {
+			let i = 0;
-					success = await this._renderReader();
+			while (i < 300 && (this._isRendering || this._isDiscarding || this._isProcessingTask)) {
-				}
+				await Zotero.Promise.delay(10);
-				else if (this.isMediaType) {
+				i++;
-					success = await this._renderMedia();
-				}
 			}
-			if (itemID !== this._item?.id) {
+
+			this._debug("Current task finished, processing new task");
+
+			let task = this._lastTask;
+			if (!task) {
+				this._debug("No task to process");
+				this._isWaitingForTask = false;
 				return;
 			}
-			this._updateWidthHeightRatio();
+
-			this.setAttribute("data-preview-type", this.previewType);
+			let uid = task.uid;
-			this.setPreviewStatus(success ? "success" : "fail");
+			
-			if (this._renderingItemID === itemID) {
+			this._isWaitingForTask = false;
-				this._renderingItemID = null;
+			this._isProcessingTask = true;
+
+			// If no new task was queued while processing, clear the last task
+			if (this._lastTask.uid === uid) {
+				this._debug("Clear last task");
+				this._lastTask = null;
 			}
-			if (Zotero.test) {
+
-				resolve();
+			this._debug(`Processing task ${task.type} (${uid})`);
+
+			switch (task.type) {
+				case "render":
+					await Promise.race([this._processRender(task.data), Zotero.Promise.delay(3000)]);
+					break;
+				case "discard":
+					await Promise.race([this._processDiscard(task.data), Zotero.Promise.delay(3000)]);
+					break;
+			}
+			
+			this._isProcessingTask = false;
+			// Force reset flag anyway to avoid blocking following tasks
+			this._isRendering = false;
+			this._isDiscarding = false;
+
+			this._debug(`Task ${task.type} (${uid}) processed`);
+		}
+
+		/**
+		 * Render the preview for the given item
+		 * First discard the current preview and then render the new preview
+		 * The render task will be aborted if the item changes before the task is finished
+		 * @returns {Promise<void>}
+		 */
+		async _processRender({ itemID, previewType }) {
+			if (this._lastRenderID === itemID && this.hasPreview) {
+				this._debug(`Item ${itemID} already rendered`);
+				return;
+			}
+
+			this._debug(`Rendering item ${itemID}, previewType: ${previewType}`);
+
+			this._isRendering = true;
+			let success = false;
+
+			try {
+				// Discard the current preview.
+				await this._processDiscard();
+
+				this._debug(`Discard finished, rendering item ${itemID}`);
+	
+				this._tryAbortRender(itemID);
+	
+				let item = Zotero.Items.get(itemID);
+				if (previewType !== "file" && await item.fileExists()) {
+					if (this.isReaderType) {
+						success = await this._renderReader(itemID);
+					}
+					else if (this.isMediaType) {
+						success = await this._renderMedia(itemID);
+					}
+				}
+				
+				this._tryAbortRender(itemID);
+	
+				this._updateWidthHeightRatio();
+				this.setAttribute("data-preview-type", this.previewType);
+	
+				this._lastRenderID = itemID;
+
+				this._debug(`Render not aborted, item ${itemID}`);
+			}
+			catch (e) {
+				if (!(e instanceof PreviewRenderAbortError)) {
+					this.setPreviewStatus("fail");
+					this._debug(`Render failed: item ${itemID}, ${e}`);
+					throw e;
+				}
+			}
+			finally {
+				this.setPreviewStatus(success ? "success" : "fail");
+				this._isRendering = false;
+
+				this._debug(`Render processed, item ${itemID} ${success ? "succeeded" : "failed"}`);
 			}
 		}
 
-		async discard(force = false) {
+		/**
-			if (!this.initialized) {
+		 * @throws {PreviewRenderAbortError}
-				return;
+		 * @param {number} itemID
-			}
+		 */
-			this._isDiscardPlanned = false;
+		_tryAbortRender(itemID) {
-			if (this._isDiscarding) {
+			if (itemID !== this._item?.id) {
-				return;
+				throw new PreviewRenderAbortError();
-			}
+			}
-			if (!force && (this.isVisible || !this._reader)) {
+		}
+
+		/**
+		 * Discard the current preview if it exists and is initialized
+		 * @returns {Promise<void>}
+		 */
+		async _processDiscard() {
+			if (!this._isReaderInitialized && !this._lastRenderID) {
+				this._debug("No preview to discard");
 				return;
 			}
+
+			this._debug("Discard preview");
+
 			this._isDiscarding = true;
 			if (this._reader) {
 				let _reader = this._reader;
@@ -275,13 +445,23 @@
 			if (nextPreview) {
 				nextPreview.id = "preview";
 			}
+			this._debug("Preview discarded");
+
 			// Preload a new next-preview
 			await this._nextPreviewInitializePromise.promise;
 			this._nextPreviewInitializePromise = Zotero.Promise.defer();
+
+			this._debug("Next preview initialized");
+
 			this._id("preview")?.after(this.nextPreview);
 			this.setPreviewStatus("loading");
+
+			// Clean up after discarding
 			this._isDiscarding = false;
-			this._renderPromise?.resolve();
+			this._lastRenderID = null;
+			this._isReaderInitialized = false;
+
+			this._debug("Discard processed");
 		}
 
 		async openAttachment(event) {
@@ -352,43 +532,45 @@
 			}
 		}
 
-		async _renderReader() {
+		/**
+		 * Render the reader for the given item
+		 * @throws {PreviewRenderAbortError}
+		 * @param {number} itemID
+		 */
+		async _renderReader(itemID) {
 			this.setPreviewStatus("loading");
 			// This only need to be awaited during first load
 			await this._previewInitializePromise.promise;
 			// This should be awaited in the following refreshes
 			await this._nextPreviewInitializePromise.promise;
+
+			this._tryAbortRender(itemID);
+
 			let prev = this._id("prev");
 			let next = this._id("next");
 			prev && (prev.disabled = true);
 			next && (next.disabled = true);
 			let success = false;
-			if (this._reader?._item?.id !== this._item?.id) {
+			let preview = this._id("preview");
-				await this.discard(true);
+
-				this._reader = await Zotero.Reader.openPreview(this._item.id, this._id("preview"));
+			this._debug(`Loading preview render for item id ${itemID}, iframe is ${preview}`);
-				success = await this._reader._open({});
+
-				// Retry 3 times if failed
+			// The reader will be initialized if the operation is not aborted before this point
-				if (!success && this._failedCount < 3) {
+			// and we'll need to discard the reader even if the operation is not finished
-					this._nextPreviewInitializePromise.resolve();
+			this._isReaderInitialized = true;
-					this._failedCount++;
+			this._reader = await Zotero.Reader.openPreview(itemID, preview);
-					// If failed on half-way of initialization, discard it
+
-					this.discard(true);
+			this._tryAbortRender(itemID);
-					setTimeout(() => {
+
-						// Try to re-render later
+			success = await this._reader._open({});
-						this.render();
+			
-					}, 500);
-				}
-			}
-			else {
-				success = true;
-			}
-			if (success) this._failedCount = 0;
 			prev && (prev.disabled = true);
 			next && (next.disabled = false);
 			return success;
 		}
 
 		async _renderMedia() {
+			this.setPreviewStatus("loading");
 			let mediaLoadPromise = new Zotero.Promise.defer();
 			let mediaID = `${this.previewType}-preview`;
 			let media = this._id(mediaID);
@@ -465,6 +647,11 @@
 		_id(id) {
 			return this.querySelector(`#${id}`);
 		}
+
+		_debug(message, ...args) {
+			if (!Zotero.test) return;
+			Zotero.debug(`[AttachmentPreview] ${message}`, ...args);
+		}
 	}
 
 	customElements.define("attachment-preview", AttachmentPreview);

chrome/content/zotero/elements/attachmentsBox.js

@@ -192,7 +192,7 @@
 		async updatePreview() {
 			// Skip if asyncRender is not finished/executed, which means the box is invisible
 			// The box will be rendered when it becomes visible
-			if (this._renderStage !== "final") {
+			if (!this.initialized || this._renderStage !== "final") {
 				return;
 			}
 			let attachment = await this._getPreviewAttachment();

test/tests/itemPaneTest.js

@@ -2,24 +2,27 @@ describe("Item pane", function () {
 	var win, doc, ZoteroPane, Zotero_Tabs, ZoteroContextPane, itemsView;
 
 	async function waitForPreviewBoxRender(box) {
-		let success = await waitForCallback(
+		let res = await waitForCallback(
-			() => box._asyncRenderItemID && !box._asyncRendering);
+			() => box._asyncRenderItemID && !box._asyncRendering,
-		if (!success) {
+			100, 3);
-			throw new Error("Wait for box render time out");
+		if (res instanceof Error) {
+			throw res;
 		}
-		await box._preview._renderPromise;
+		return true;
-		return success;
 	}
 
 	async function waitForPreviewBoxReader(box, itemID) {
+		let preview = box._preview;
 		await waitForPreviewBoxRender(box);
-		let success = await waitForCallback(
+		let res = await waitForCallback(
-			() => box._preview._reader?.itemID == itemID, 100, 3000);
+			() => preview._reader?.itemID == itemID
-		if (!success) {
+				&& !preview._isProcessingTask && !preview._lastTask
-			throw new Error("Wait for box preview reader time out");
+			, 100, 3);
+		if (res instanceof Error) {
+			throw res;
 		}
-		await box._preview._reader._initPromise;
+		await preview._reader._initPromise;
-		return success;
+		return true;
 	}
 
 	function isPreviewDisplayed(box) {
@@ -386,6 +389,7 @@ describe("Item pane", function () {
 
 		afterEach(function () {
 			Zotero_Tabs.select("zotero-pane");
+			Zotero_Tabs.closeAll();
 		});
 
 		it("should show attachments pane in library for regular item", async function () {
@@ -676,7 +680,7 @@ describe("Item pane", function () {
 		 * AttachmentsBox serves as a good example since it involves both sync and async rendering.
 		 * If this test fails, it is not recommended to add timeouts as a quick fix.
 		 */
-		it.skip("should keep attachments pane status after changing selection", async function () {
+		it("should keep attachments pane status after changing selection", async function () {
 			let itemDetails = ZoteroPane.itemPane._itemDetails;
 			let attachmentsBox = itemDetails.getPane(paneID);
 			let preview = attachmentsBox._preview;
@@ -845,6 +849,60 @@ describe("Item pane", function () {
 			// Should open attachment
 			assert.equal(reader.itemID, attachment.id);
 		});
+
+		it("should render preview robustly after making dense calls to render and discard", async function () {
+			let itemDetails = ZoteroPane.itemPane._itemDetails;
+			let attachmentsBox = itemDetails.getPane(paneID);
+			let preview = attachmentsBox._preview;
+
+			// Pin the pane to avoid always scrolling to the section
+			itemDetails.pinnedPane = paneID;
+
+			// item with attachment
+			let item1 = new Zotero.Item('book');
+			await item1.saveTx();
+			let file1 = getTestDataDirectory();
+			file1.append('test.pdf');
+			let attachment1 = await Zotero.Attachments.importFromFile({
+				file: file1,
+				parentItemID: item1.id
+			});
+
+			let item2 = new Zotero.Item('book');
+			await item2.saveTx();
+			let file2 = getTestDataDirectory();
+			file2.append('test.pdf');
+			let attachment2 = await Zotero.Attachments.importFromFile({
+				file: file2,
+				parentItemID: item2.id
+			});
+
+			let selectionMap = [item1.id, item2.id];
+			// Repeat render/discard multiple times
+			for (let i = 0; i < 10; i++) {
+				await ZoteroPane.selectItem(selectionMap[i % 2]);
+
+				// No await, since the render/discard may be triggered at any time in actual usage
+				preview.discard();
+				preview.render();
+			}
+
+			// Wait for the last render/discard task to finish
+			await waitForCallback(() => !preview._isRendering && !preview._isDiscarding
+				&& !preview._isProcessingTask && !preview._isWaitingForTask
+				&& !preview._lastTask);
+
+			// Should be able to render the correct preview
+			await ZoteroPane.selectItem(item1.id);
+			await waitForPreviewBoxReader(attachmentsBox, attachment1.id);
+			assert.isTrue(isPreviewDisplayed(attachmentsBox));
+
+			await ZoteroPane.selectItem(item2.id);
+			await waitForPreviewBoxReader(attachmentsBox, attachment2.id);
+			assert.isTrue(isPreviewDisplayed(attachmentsBox));
+
+			itemDetails.pinnedPane = "";
+		});
 	});
 	
 	
@@ -972,6 +1030,7 @@ describe("Item pane", function () {
 
 		afterEach(function () {
 			Zotero_Tabs.select("zotero-pane");
+			Zotero_Tabs.closeAll();
 		});
 
 		it("should refresh on file rename", async function () {
@@ -1002,9 +1061,12 @@ describe("Item pane", function () {
 
 			let paneHeader = doc.getElementById('zotero-item-pane-header');
 			let label = paneHeader.titleField;
-			let promise = waitForDOMAttributes(label, 'value', (newValue) => {
+			let promise = Promise.all([
-				return newValue === newTitle;
+				waitForDOMAttributes(label, 'value', (newValue) => {
-			});
+					return newValue === newTitle;
+				}),
+				waitForItemEvent('modify')
+			]);
 
 			item.setField('title', newTitle);
 			await item.saveTx();
@@ -1013,7 +1075,7 @@ describe("Item pane", function () {
 			assert.equal(label.value, newTitle);
 		});
 
-		it.skip("should show attachment pane in library for attachment item", async function () {
+		it("should show attachment pane in library for attachment item", async function () {
 			// Regular item: hide
 			let itemDetails = ZoteroPane.itemPane._itemDetails;
 			let box = itemDetails.getPane(paneID);