From ca9508ebce17c73a8341ab9ea950b97d91d4fbbe Mon Sep 17 00:00:00 2001 From: windingwind <33902321+windingwind@users.noreply.github.com> Date: Thu, 15 Aug 2024 14:22:24 +0800 Subject: [PATCH] Lazy load attachment preview (#4568) --- .../content/zotero/elements/attachmentBox.js | 60 +++++- .../content/zotero/elements/attachmentsBox.js | 56 +++++- chrome/content/zotero/elements/itemDetails.js | 11 +- test/tests/itemPaneTest.js | 172 +++++++++++++++--- 4 files changed, 254 insertions(+), 45 deletions(-) diff --git a/chrome/content/zotero/elements/attachmentBox.js b/chrome/content/zotero/elements/attachmentBox.js index 182add6fbb..c166d33e1a 100644 --- a/chrome/content/zotero/elements/attachmentBox.js +++ b/chrome/content/zotero/elements/attachmentBox.js @@ -32,7 +32,6 @@ content = MozXULElement.parseXULToFragment(` - `); + _body = null; + + _preview = null; + + _lastPreviewRenderId = ""; + + _discardPreviewTimeout = 60000; + + _previewDiscarded = false; + constructor() { super(); @@ -91,7 +100,6 @@ this._item = null; this._section = null; - this._preview = null; this._asyncRendering = false; @@ -212,17 +220,25 @@ if (val.isAttachment()) { this._item = val; this.hidden = false; - this._preview.disableResize = false; } else { this.hidden = true; - this._preview.disableResize = true; } + if (this._preview) this._preview.disableResize = !!this.hidden; + } + + get previewElem() { + if (!this._preview) { + this._initPreview(); + } + return this._preview; } init() { this.initCollapsibleSection(); + this._body = this.querySelector('.body'); + this._id('url').addEventListener('contextmenu', (event) => { this._id('url-menu').openPopupAtScreen(event.screenX, event.screenY, true); }); @@ -236,8 +252,6 @@ this._isEditingFilename = false; }); - this._preview = this._id("attachment-preview"); - let noteButton = this._id('note-button'); noteButton.addEventListener("command", () => { this.convertAttachmentNote(); @@ -301,7 +315,14 @@ if (!this.item) return; if (this._asyncRendering) return; if (!this._section.open) return; - if (this._isAlreadyRendered("async")) return; + if (this._isAlreadyRendered("async")) { + if (this._previewDiscarded) { + this._previewDiscarded = false; + this.previewElem.render(); + } + this._lastPreviewRenderTime = Date.now(); + return; + } Zotero.debug('Refreshing attachment box'); this._asyncRendering = true; @@ -450,11 +471,24 @@ } if (this.usePreview) { - this._preview.item = this.item; - await this._preview.render(); + this.previewElem.item = this.item; + await this.previewElem.render(); } this._asyncRendering = false; + + this._lastPreviewRenderTime = `${Date.now()}-${Math.random()}`; + } + + discard() { + if (!this._preview) return; + let lastRenderTime = this._lastPreviewRenderId; + setTimeout(() => { + if (!this._asyncRendering && this._lastPreviewRenderId === lastRenderTime) { + this._preview?.discard(); + this._previewDiscarded = true; + } + }, this._discardPreviewTimeout); } onViewClick(event) { @@ -676,6 +710,14 @@ if (finished) resolve(); else reject(new Error("AttachmentBox#_waitForRender timeout")); } + + _initPreview() { + this._preview = document.createXULElement('attachment-preview'); + this._preview.setAttribute('tabindex', '0'); + this._preview.setAttribute('data-l10n-id', 'attachment-preview'); + this._body.prepend(this._preview); + this._preview.disableResize = !!this.hidden; + } } customElements.define("attachment-box", AttachmentBox); diff --git a/chrome/content/zotero/elements/attachmentsBox.js b/chrome/content/zotero/elements/attachmentsBox.js index 12cb12af07..dad27cb107 100644 --- a/chrome/content/zotero/elements/attachmentsBox.js +++ b/chrome/content/zotero/elements/attachmentsBox.js @@ -30,7 +30,6 @@ content = MozXULElement.parseXULToFragment(` - @@ -45,8 +44,16 @@ _attachmentIDs = []; + _body = null; + _preview = null; + _lastPreviewRenderTime = ""; + + _discardPreviewTimeout = 60000; + + _previewDiscarded = false; + get item() { return this._item; } @@ -59,7 +66,7 @@ super.item = item; let hidden = !item?.isRegularItem() || item?.isFeedItem; this.hidden = hidden; - this._preview.disableResize = !!hidden; + if (this._preview) this._preview.disableResize = !!hidden; } get inTrash() { @@ -81,10 +88,19 @@ } } + get previewElem() { + if (!this._preview) { + this._initPreview(); + } + return this._preview; + } + init() { this.initCollapsibleSection(); this._section.addEventListener('add', this._handleAdd); + this._body = this.querySelector('.body'); + this._attachments = this.querySelector('.attachments-container'); this._addPopup = this.querySelector('.add-popup'); @@ -106,7 +122,6 @@ }); this.usePreview = Zotero.Prefs.get('showAttachmentPreview'); - this._preview = this.querySelector('attachment-preview'); this._notifierID = Zotero.Notifier.registerObserver(this, ['item'], 'attachmentsBox'); @@ -169,7 +184,14 @@ async asyncRender() { if (!this._item) return; - if (this._isAlreadyRendered("async")) return; + if (this._isAlreadyRendered("async")) { + if (this._previewDiscarded) { + this._previewDiscarded = false; + this.previewElem.render(); + } + this._lastPreviewRenderTime = `${Date.now()}-${Math.random()}`; + return; + } this._renderStage = "final"; this._asyncRendering = true; @@ -184,6 +206,17 @@ await this.updatePreview(); this._asyncRendering = false; } + + discard() { + if (!this._preview) return; + let lastRenderTime = this._lastPreviewRenderTime; + setTimeout(() => { + if (!this._asyncRendering && this._lastPreviewRenderTime === lastRenderTime) { + this._preview?.discard(); + this._previewDiscarded = true; + } + }, this._discardPreviewTimeout); + } updateCount() { if (!this._item?.isRegularItem()) { @@ -211,8 +244,9 @@ || (this._attachmentIDs.length && !this._section.open)) { return; } - this._preview.item = attachment; - await this._preview.render(); + this.previewElem.item = attachment; + await this.previewElem.render(); + this._lastPreviewRenderTime = `${Date.now()}-${Math.random()}`; } async _getPreviewAttachment() { @@ -225,6 +259,14 @@ return attachment; } + _initPreview() { + this._preview = document.createXULElement('attachment-preview'); + this._preview.setAttribute('tabindex', '0'); + this._preview.setAttribute('data-l10n-id', 'attachment-preview'); + this._body.prepend(this._preview); + this._preview.disableResize = !!this.hidden; + } + _handleAdd = (event) => { this._section.open = true; this._addPopup.openPopup(event.detail.button, 'after_end'); @@ -238,7 +280,7 @@ menu.dataset.l10nArgs = `{ "type": "${this.usePreview ? "open" : "collapsed"}" }`; if (toOpen) { - this._preview.render(); + this.previewElem.render(); } }; diff --git a/chrome/content/zotero/elements/itemDetails.js b/chrome/content/zotero/elements/itemDetails.js index 4c03a0fb71..def19a1dc0 100644 --- a/chrome/content/zotero/elements/itemDetails.js +++ b/chrome/content/zotero/elements/itemDetails.js @@ -275,11 +275,13 @@ box.tabType = this.tabType; box.item = item; box.collectionTreeRow = this.collectionTreeRow; + // Discard hidden panes + if (box.hidden && box.discard) { + box.discard(); + } // Execute sync render immediately if (!box.hidden && box.render) { - if (box.render) { - box.render(); - } + box.render(); } } @@ -604,7 +606,8 @@ let needsDiscard = []; entries.forEach((entry) => { let targetPaneElem = entry.target; - if (entry.isIntersecting && targetPaneElem.render) { + if (entry.isIntersecting + && (targetPaneElem.render || targetPaneElem.asyncRender)) { needsRefresh.push(targetPaneElem); } else if (targetPaneElem.discard) { diff --git a/test/tests/itemPaneTest.js b/test/tests/itemPaneTest.js index dd2de97ec0..0457ce0198 100644 --- a/test/tests/itemPaneTest.js +++ b/test/tests/itemPaneTest.js @@ -12,7 +12,8 @@ describe("Item pane", function () { } async function waitForPreviewBoxReader(box, itemID) { - let preview = box._preview; + let preview = await getBoxPreview(box); + if (!preview) return false; await waitForPreviewBoxRender(box); let res = await waitForCallback( () => preview._reader?.itemID == itemID @@ -25,9 +26,26 @@ describe("Item pane", function () { return true; } - function isPreviewDisplayed(box) { - return !!(box._preview.hasPreview - && win.getComputedStyle(box._preview).display !== "none"); + async function isPreviewDisplayed(box) { + let preview = await getBoxPreview(box); + if (!preview) return false; + return !!(preview.hasPreview + && win.getComputedStyle(preview).display !== "none"); + } + + async function getBoxPreview(box) { + try { + // Since we are lazy loading the preview, should wait for the preview to be initialized + await waitForCallback( + () => !!box._preview + , 10, 0.5); + } + catch (e) { + Zotero.logError(e); + // Return false if waitForCallback fails + return false; + } + return box._preview; } before(function* () { @@ -497,13 +515,14 @@ describe("Item pane", function () { }); await ZoteroPane.viewItems([attachment]); let tabID = Zotero_Tabs.selectedID; + ZoteroContextPane.splitter.setAttribute("state", "open"); let itemDetails = ZoteroContextPane.context._getItemContext(tabID); let attachmentsBox = itemDetails.getPane(paneID); assert.isFalse(attachmentsBox.hidden); await waitForScrollToPane(itemDetails, paneID); - assert.isFalse(isPreviewDisplayed(attachmentsBox)); + assert.isFalse(await isPreviewDisplayed(attachmentsBox)); }); it("should not show attachments pane in reader standalone attachment item", async function () { @@ -541,7 +560,7 @@ describe("Item pane", function () { await waitForScrollToPane(itemDetails, paneID); await waitForPreviewBoxRender(attachmentsBox); - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); }); it("should not render attachments pane preview when show preview is disabled", async function () { @@ -556,7 +575,7 @@ describe("Item pane", function () { await waitForScrollToPane(itemDetails, paneID); - assert.isFalse(isPreviewDisplayed(attachmentsBox)); + assert.isFalse(await isPreviewDisplayed(attachmentsBox)); }); it("should only render after attachments pane becomes visible", async function () { @@ -566,7 +585,7 @@ describe("Item pane", function () { let itemDetails = ZoteroPane.itemPane._itemDetails; let attachmentsBox = itemDetails.getPane(paneID); - let preview = attachmentsBox._preview; + let preview = attachmentsBox.previewElem; // Force discard previous preview await preview.discard(true); @@ -584,7 +603,7 @@ describe("Item pane", function () { // Do not use _isAlreadyRendered, since that changes the render flag state assert.equal(attachmentsBox._syncRenderItemID, item.id); assert.notEqual(attachmentsBox._asyncRenderItemID, item.id); - assert.isFalse(isPreviewDisplayed(attachmentsBox)); + assert.isFalse(await isPreviewDisplayed(attachmentsBox)); await waitForScrollToPane(itemDetails, paneID); await waitForPreviewBoxRender(attachmentsBox); @@ -594,7 +613,7 @@ describe("Item pane", function () { assert.equal(attachmentsBox._syncRenderItemID, item.id); assert.equal(attachmentsBox._asyncRenderItemID, item.id); - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); assert.isTrue(preview.hasPreview); win.resizeTo(null, height); }); @@ -604,7 +623,7 @@ describe("Item pane", function () { let itemDetails = ZoteroPane.itemPane._itemDetails; let attachmentsBox = itemDetails.getPane(paneID); - let preview = attachmentsBox._preview; + let preview = attachmentsBox.previewElem; // Force discard previous preview await preview.discard(true); @@ -617,7 +636,7 @@ describe("Item pane", function () { await ZoteroPane.selectItem(item.id); assert.isTrue(await waitForPreviewBoxRender(attachmentsBox)); // No preview - assert.isFalse(isPreviewDisplayed(attachmentsBox)); + assert.isFalse(await isPreviewDisplayed(attachmentsBox)); // No row assert.equal(attachmentsBox.querySelectorAll("attachment-row").length, 0); @@ -632,7 +651,7 @@ describe("Item pane", function () { await itemDetails._renderPromise; await waitForPreviewBoxRender(attachmentsBox); // Image preview for item with image attachment - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); assert.equal(preview.previewType, "image"); // 1 row assert.equal(attachmentsBox.querySelectorAll("attachment-row").length, 1); @@ -647,7 +666,7 @@ describe("Item pane", function () { await waitForPreviewBoxReader(attachmentsBox, attachment2.id); await Zotero.Promise.delay(100); // PDF preview - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); assert.equal(preview.previewType, "pdf"); // 2 rows assert.equal(attachmentsBox.querySelectorAll("attachment-row").length, 2); @@ -690,7 +709,7 @@ describe("Item pane", function () { await attachment2.eraseTx(); await Zotero.Promise.delay(100); // Image preview for item with image attachment - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); assert.equal(preview.previewType, "image"); // 1 row assert.equal(attachmentsBox.querySelectorAll("attachment-row").length, 1); @@ -727,7 +746,7 @@ describe("Item pane", function () { assert.isFalse(attachmentsBox.hidden); await waitForScrollToPane(itemDetails, paneID); - assert.isFalse(isPreviewDisplayed(attachmentsBox)); + assert.isFalse(await isPreviewDisplayed(attachmentsBox)); // Select library tab Zotero_Tabs.select("zotero-pane"); @@ -746,7 +765,7 @@ describe("Item pane", function () { Zotero_Tabs.select(tabID); // Make sure the preview status is not changed in reader - assert.isFalse(isPreviewDisplayed(attachmentsBox)); + assert.isFalse(await isPreviewDisplayed(attachmentsBox)); }); /** @@ -758,7 +777,7 @@ describe("Item pane", 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; + let preview = attachmentsBox.previewElem; // Pin the pane to avoid always scrolling to the section itemDetails.pinnedPane = paneID; @@ -898,7 +917,7 @@ describe("Item pane", function () { it("should open attachment on double-clicking attachments pane preview", async function () { let itemDetails = ZoteroPane.itemPane._itemDetails; let attachmentsBox = itemDetails.getPane(paneID); - let preview = attachmentsBox._preview; + let preview = attachmentsBox.previewElem; let item = new Zotero.Item('book'); await item.saveTx(); @@ -928,7 +947,7 @@ describe("Item pane", function () { 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; + let preview = attachmentsBox.previewElem; // Pin the pane to avoid always scrolling to the section itemDetails.pinnedPane = paneID; @@ -970,14 +989,77 @@ describe("Item pane", function () { // Should be able to render the correct preview await ZoteroPane.selectItem(item1.id); await waitForPreviewBoxReader(attachmentsBox, attachment1.id); - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); await ZoteroPane.selectItem(item2.id); await waitForPreviewBoxReader(attachmentsBox, attachment2.id); - assert.isTrue(isPreviewDisplayed(attachmentsBox)); + assert.isTrue(await isPreviewDisplayed(attachmentsBox)); itemDetails.pinnedPane = ""; }); + + it("should not load preview iframe before becoming visible", async function () { + let itemDetails = ZoteroPane.itemPane._itemDetails; + let attachmentsBox = itemDetails.getPane(paneID); + + // Resize to very small height to ensure the attachment box is not in view + let height = doc.documentElement.clientHeight; + win.resizeTo(null, 100); + + // Remove any existing preview to ensure the test is valid + attachmentsBox._preview?.remove(); + attachmentsBox._preview = null; + + let item = await createDataObject('item'); + await importFileAttachment('test.pdf', { parentID: item.id }); + + await ZoteroPane.selectItem(item.id); + + assert.notExists(attachmentsBox._preview); + assert.notExists(attachmentsBox.querySelector("#preview")); + + await waitForScrollToPane(itemDetails, paneID); + await waitForPreviewBoxRender(attachmentsBox); + + assert.exists(await getBoxPreview(attachmentsBox)); + + win.resizeTo(null, height); + }); + + it("should discard attachments pane preview after becoming invisible", async function () { + let itemDetails = ZoteroPane.itemPane._itemDetails; + let attachmentsBox = itemDetails.getPane(paneID); + + // Resize to very small height to ensure the attachment box is not in view + let height = doc.documentElement.clientHeight; + win.resizeTo(null, 100); + + const discardTimeout = 50; + + // Temporarily set discard timeout to 100ms for testing + let currentDiscardTimeout = attachmentsBox._discardPreviewTimeout; + attachmentsBox._discardPreviewTimeout = discardTimeout; + + let item = await createDataObject('item'); + await importFileAttachment('test.pdf', { parentID: item.id }); + + await ZoteroPane.selectItem(item.id); + await waitForScrollToPane(itemDetails, paneID); + await waitForPreviewBoxRender(attachmentsBox); + + assert.isTrue(attachmentsBox._preview._isReaderInitialized); + + // Scroll the attachments pane out of view + await waitForScrollToPane(itemDetails, 'info'); + + // Wait a bit for the preview to be discarded + await Zotero.Promise.delay(discardTimeout + 100); + + assert.isFalse(attachmentsBox._preview._isReaderInitialized); + + win.resizeTo(null, height); + attachmentsBox._discardPreviewTimeout = currentDiscardTimeout; + }); }); @@ -1147,6 +1229,11 @@ describe("Item pane", function () { await item.saveTx(); await promise; + + // Wait for section to finish rendering + let box = ZoteroPane.itemPane._itemDetails.getPane(paneID); + await waitForPreviewBoxRender(box); + assert.equal(label.value, newTitle); }); @@ -1154,6 +1241,11 @@ describe("Item pane", function () { // Regular item: hide let itemDetails = ZoteroPane.itemPane._itemDetails; let box = itemDetails.getPane(paneID); + + // TEMP: Force abort any pending renders + box._preview?.remove(); + box._preview = null; + let item = new Zotero.Item('book'); await item.saveTx(); await ZoteroPane.selectItem(item.id); @@ -1172,7 +1264,7 @@ describe("Item pane", function () { await waitForPreviewBoxReader(box, attachment.id); assert.isFalse(box.hidden); await Zotero.Promise.delay(100); - assert.isTrue(isPreviewDisplayed(box)); + assert.isTrue(await isPreviewDisplayed(box)); // Standalone attachment: show let attachment1 = await importFileAttachment('test.pdf'); @@ -1181,7 +1273,7 @@ describe("Item pane", function () { await waitForPreviewBoxReader(box, attachment1.id); assert.isFalse(box.hidden); await Zotero.Promise.delay(100); - assert.isTrue(isPreviewDisplayed(box)); + assert.isTrue(await isPreviewDisplayed(box)); }); it("should show attachment pane without preview in reader for standalone attachment item", async function () { @@ -1210,7 +1302,7 @@ describe("Item pane", function () { await waitForScrollToPane(itemDetails, paneID); // No preview - assert.isFalse(isPreviewDisplayed(box)); + assert.isFalse(await isPreviewDisplayed(box)); }); it("should only show attachment note container when exists", async function () { @@ -1238,6 +1330,36 @@ describe("Item pane", function () { // Should be readonly assert.equal(noteEditor.mode, "view"); }); + + it("should discard attachment pane preview after becoming invisible", async function () { + let itemDetails = ZoteroPane.itemPane._itemDetails; + let attachmentBox = itemDetails.getPane(paneID); + + const discardTimeout = 50; + + // Temporarily set discard timeout to 100ms for testing + let currentDiscardTimeout = attachmentBox._discardPreviewTimeout; + attachmentBox._discardPreviewTimeout = discardTimeout; + + let item = await createDataObject('item'); + let attachment = await importFileAttachment('test.pdf', { parentID: item.id }); + + await ZoteroPane.selectItem(attachment.id); + await waitForScrollToPane(itemDetails, paneID); + await waitForPreviewBoxRender(attachmentBox); + + assert.isTrue(attachmentBox._preview._isReaderInitialized); + + // Select a regular item to hide the attachment pane + await ZoteroPane.selectItem(item.id); + + // Wait a bit for the preview to be discarded + await Zotero.Promise.delay(discardTimeout + 100); + + assert.isFalse(attachmentBox._preview._isReaderInitialized); + + attachmentBox._discardPreviewTimeout = currentDiscardTimeout; + }); });