From e909fcdba8f0ae0302e68ef108dcf74c5b8424c6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 14 Nov 2021 12:20:04 +0100 Subject: [PATCH 1/2] Only show the `loadingIcon`-spinner on visible pages (issue 14242) This patch preserves the old behaviour of appending a `loadingIcon`-div to all pages that are not yet loaded/rendered. However, the actual `loadingIcon`-spinner (i.e. the `loading-icon.gif` image) will only be displayed on *visible* pages to improve performance. To avoid having to iterate through all pages in the document, which doesn't seem like a good idea for a PDF document with thousands of pages, we use a combination of the currently visible *and* cached pages to toggle the `loadingIcon`-spinner. --- web/base_viewer.js | 23 +++++++++++++++++++++++ web/pdf_page_view.js | 12 +++++++++++- web/pdf_viewer.css | 3 +++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 9ae9952f8..c7bf2f607 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -158,6 +158,10 @@ class PDFPageViewBuffer { return this.#buf.has(view); } + [Symbol.iterator]() { + return this.#buf.keys(); + } + #destroyFirstView() { const firstView = this.#buf.keys().next().value; @@ -1402,6 +1406,23 @@ class BaseViewer { return this.scroll.down; } + /** + * Only show the `loadingIcon`-spinner on visible pages (see issue 14242). + */ + #toggleLoadingIconSpinner(visibleIds) { + for (const id of visibleIds) { + const pageView = this._pages[id - 1]; + pageView?.toggleLoadingIconSpinner(/* viewVisible = */ true); + } + for (const pageView of this.#buffer) { + if (visibleIds.has(pageView.id)) { + // Handled above, since the "buffer" may not contain all visible pages. + continue; + } + pageView.toggleLoadingIconSpinner(/* viewVisible = */ false); + } + } + forceRendering(currentlyVisiblePages) { const visiblePages = currentlyVisiblePages || this._getVisiblePages(); const scrollAhead = this.#getScrollAhead(visiblePages); @@ -1415,6 +1436,8 @@ class BaseViewer { scrollAhead, preRenderExtra ); + this.#toggleLoadingIconSpinner(visiblePages.ids); + if (pageView) { this._ensurePdfPageLoaded(pageView).then(() => { this.renderingQueue.renderView(pageView); diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index ca87ab4d8..0f8171f0d 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -289,7 +289,10 @@ class PDFPageView { } this.loadingIconDiv = document.createElement("div"); - this.loadingIconDiv.className = "loadingIcon"; + this.loadingIconDiv.className = "loadingIcon notVisible"; + if (this._isStandalone) { + this.toggleLoadingIconSpinner(/* viewVisible = */ true); + } this.loadingIconDiv.setAttribute("role", "img"); this.l10n.get("loading").then(msg => { this.loadingIconDiv?.setAttribute("aria-label", msg); @@ -523,6 +526,13 @@ class PDFPageView { return this.viewport.convertToPdfPoint(x, y); } + /** + * @ignore + */ + toggleLoadingIconSpinner(viewVisible = false) { + this.loadingIconDiv?.classList.toggle("notVisible", !viewVisible); + } + draw() { if (this.renderingState !== RenderingStates.INITIAL) { console.error("Must be in new state before drawing"); diff --git a/web/pdf_viewer.css b/web/pdf_viewer.css index b9e640832..dc0feda97 100644 --- a/web/pdf_viewer.css +++ b/web/pdf_viewer.css @@ -129,6 +129,9 @@ bottom: 0; background: url("images/loading-icon.gif") center no-repeat; } +.pdfViewer .page .loadingIcon.notVisible { + background: none; +} .pdfPresentationMode .pdfViewer { margin-left: 0; From 7d4c37e988fba7547703cad5383a94289cb27a87 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 15 Nov 2021 13:26:50 +0100 Subject: [PATCH 2/2] Use the new iterator in the `PDFPageViewBuffer` unit-tests The previous patch introduced an iterator in the `PDFPageViewBuffer`-class, hence the test-only `_buffer`-getter is no longer necessary. --- test/unit/base_viewer_spec.js | 16 ++++++++-------- web/base_viewer.js | 11 ----------- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/test/unit/base_viewer_spec.js b/test/unit/base_viewer_spec.js index 8131f9a2d..f59ef75cd 100644 --- a/test/unit/base_viewer_spec.js +++ b/test/unit/base_viewer_spec.js @@ -40,7 +40,7 @@ describe("BaseViewer", function () { buffer.push(view); } // Ensure that the correct views are inserted. - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(1), viewsMap.get(2), viewsMap.get(3), @@ -51,7 +51,7 @@ describe("BaseViewer", function () { buffer.push(view); } // Ensure that the correct views are evicted. - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(3), viewsMap.get(4), viewsMap.get(5), @@ -71,7 +71,7 @@ describe("BaseViewer", function () { // Ensure that keeping the size constant won't evict any views. buffer.resize(5); - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(1), viewsMap.get(2), viewsMap.get(3), @@ -82,7 +82,7 @@ describe("BaseViewer", function () { // Ensure that increasing the size won't evict any views. buffer.resize(10); - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(1), viewsMap.get(2), viewsMap.get(3), @@ -93,7 +93,7 @@ describe("BaseViewer", function () { // Ensure that decreasing the size will evict the correct views. buffer.resize(3); - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(3), viewsMap.get(4), viewsMap.get(5), @@ -114,7 +114,7 @@ describe("BaseViewer", function () { // while re-ordering them correctly. buffer.resize(5, new Set([1, 2])); - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(3), viewsMap.get(4), viewsMap.get(5), @@ -126,7 +126,7 @@ describe("BaseViewer", function () { // while re-ordering them correctly. buffer.resize(10, new Set([3, 4, 5])); - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(1), viewsMap.get(2), viewsMap.get(3), @@ -138,7 +138,7 @@ describe("BaseViewer", function () { // while re-ordering the remaining ones correctly. buffer.resize(3, new Set([1, 2, 5])); - expect(buffer._buffer).toEqual([ + expect([...buffer]).toEqual([ viewsMap.get(1), viewsMap.get(2), viewsMap.get(5), diff --git a/web/base_viewer.js b/web/base_viewer.js index c7bf2f607..af590e454 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -99,17 +99,6 @@ class PDFPageViewBuffer { constructor(size) { this.#size = size; - - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("!PRODUCTION || TESTING") - ) { - Object.defineProperty(this, "_buffer", { - get() { - return [...this.#buf]; - }, - }); - } } push(view) {