From a578571f59fa167712ee9717e4149f09b727fd8f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 26 Dec 2022 15:00:22 +0100 Subject: [PATCH 1/2] Only display the page-loadingIcon when rendering is currently running Given that we only render one page at a time, this will lead to only *one* page-loadingIcon being displayed at a time even if multiple pages are visible in the viewer. However, this will make it clearer which page is the currently parsing/rendering one. To simplify toggling of the page-loadingIcon visibility, the existing `PDFPageView.renderingState` is changed into a getter/setter-pair with the latter also handling the page-loadingIcon state. An additional benefit of these changes is that the `PDFViewer` no longer needs to handling toggling of page-loadingIcon visibility during rendering, since there can only ever be *one* page rendering. Finally, this may also simplify future changes w.r.t. page-loadingIcon visibility toggling (using e.g. a show-timeout). --- web/pdf_page_view.js | 55 +++++++++++++++++++++----------------------- web/pdf_viewer.js | 18 --------------- 2 files changed, 26 insertions(+), 47 deletions(-) diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index ad410af4a..f0f2b6f7b 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -120,6 +120,8 @@ class PDFPageView { #previousRotation = null; + #renderingState = RenderingStates.INITIAL; + #useThumbnailCanvas = { initialOptionalContent: true, regularAnnotations: true, @@ -167,7 +169,6 @@ class PDFPageView { this.paintTask = null; this.paintedViewportMap = new WeakMap(); - this.renderingState = RenderingStates.INITIAL; this.resume = null; this._renderError = null; if ( @@ -227,6 +228,30 @@ class PDFPageView { } } + get renderingState() { + return this.#renderingState; + } + + set renderingState(state) { + this.#renderingState = state; + + switch (state) { + case RenderingStates.INITIAL: + case RenderingStates.PAUSED: + this.loadingIconDiv?.classList.add("notVisible"); + break; + case RenderingStates.RUNNING: + this.loadingIconDiv?.classList.remove("notVisible"); + break; + case RenderingStates.FINISHED: + if (this.loadingIconDiv) { + this.loadingIconDiv.remove(); + delete this.loadingIconDiv; + } + break; + } + } + #setDimensions() { const { viewport } = this; if (this.pdfPage) { @@ -496,17 +521,6 @@ class PDFPageView { this.loadingIconDiv?.setAttribute("aria-label", msg); }); div.append(this.loadingIconDiv); - } else { - this.toggleLoadingIconSpinner(); - div.append(this.loadingIconDiv); - } - - if ( - (typeof PDFJSDev === "undefined" || - PDFJSDev.test("!PRODUCTION || GENERIC")) && - this._isStandalone - ) { - this.toggleLoadingIconSpinner(/* viewVisible = */ true); } } @@ -775,13 +789,6 @@ 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"); @@ -791,11 +798,6 @@ class PDFPageView { if (!pdfPage) { this.renderingState = RenderingStates.FINISHED; - - if (this.loadingIconDiv) { - this.loadingIconDiv.remove(); - delete this.loadingIconDiv; - } return Promise.reject(new Error("pdfPage is not loaded")); } @@ -888,11 +890,6 @@ class PDFPageView { this._renderError = error; this.renderingState = RenderingStates.FINISHED; - - if (this.loadingIconDiv) { - this.loadingIconDiv.remove(); - delete this.loadingIconDiv; - } this._resetZoomLayer(/* removeFromDOM = */ true); // Ensure that the thumbnails won't become partially (or fully) blank, diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 995b15d83..25caf7c15 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -1626,23 +1626,6 @@ class PDFViewer { 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); @@ -1656,7 +1639,6 @@ class PDFViewer { scrollAhead, preRenderExtra ); - this.#toggleLoadingIconSpinner(visiblePages.ids); if (pageView) { this.#ensurePdfPageLoaded(pageView).then(() => { From 4224984525a03b4826d114c1b2433cf3332ab345 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 26 Dec 2022 15:00:30 +0100 Subject: [PATCH 2/2] Only display the pageNumber-loadingIcon when rendering is currently running *This makes the same kind of changes as in the previous patch, but for the pageNumber-loadingIcon in the main toolbar.* To display the pageNumber-loadingIcon when rendering starts, if the page is the most visible one, we'll utilize the existing "pagerender" event. To toggle the pageNumber-loadingIcon as the user moves through the document we'll now instead utilize the "pagechanging" event, which should actually be slightly more efficient overall[1]. Note how we'd, in the old code, only consider the most visible page anyway when toggling the pageNumber-loadingIcon. --- [1] Even in a PDF document as relatively short/simple as `tracemonkey.pdf`, scrolling through the entire document can easily trigger the "updateviewarea" event more than a thousand times. --- web/app.js | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/web/app.js b/web/app.js index 493458bc2..60ce098b2 100644 --- a/web/app.js +++ b/web/app.js @@ -1804,6 +1804,7 @@ const PDFViewerApplication = { eventBus._on("hashchange", webViewerHashchange); eventBus._on("beforeprint", _boundEvents.beforePrint); eventBus._on("afterprint", _boundEvents.afterPrint); + eventBus._on("pagerender", webViewerPageRender); eventBus._on("pagerendered", webViewerPageRendered); eventBus._on("updateviewarea", webViewerUpdateViewarea); eventBus._on("pagechanging", webViewerPageChanging); @@ -1936,6 +1937,7 @@ const PDFViewerApplication = { eventBus._off("hashchange", webViewerHashchange); eventBus._off("beforeprint", _boundEvents.beforePrint); eventBus._off("afterprint", _boundEvents.afterPrint); + eventBus._off("pagerender", webViewerPageRender); eventBus._off("pagerendered", webViewerPageRendered); eventBus._off("updateviewarea", webViewerUpdateViewarea); eventBus._off("pagechanging", webViewerPageChanging); @@ -2212,6 +2214,14 @@ function webViewerInitialized() { } } +function webViewerPageRender({ pageNumber }) { + // If the page is (the most) visible when it starts rendering, + // ensure that the page number input loading indicator is displayed. + if (pageNumber === PDFViewerApplication.page) { + PDFViewerApplication.toolbar?.updateLoadingIndicatorState(true); + } +} + function webViewerPageRendered({ pageNumber, error }) { // If the page is still visible when it has finished rendering, // ensure that the page number input loading indicator is hidden. @@ -2321,20 +2331,13 @@ function webViewerUpdateViewarea({ location }) { // Unable to write to storage. }); } - const href = PDFViewerApplication.pdfLinkService.getAnchorUrl( - location.pdfOpenParams - ); if (PDFViewerApplication.appConfig.secondaryToolbar) { + const href = PDFViewerApplication.pdfLinkService.getAnchorUrl( + location.pdfOpenParams + ); PDFViewerApplication.appConfig.secondaryToolbar.viewBookmarkButton.href = href; } - - // Show/hide the loading indicator in the page number input element. - const currentPage = PDFViewerApplication.pdfViewer.getPageView( - /* index = */ PDFViewerApplication.page - 1 - ); - const loading = currentPage?.renderingState !== RenderingStates.FINISHED; - PDFViewerApplication.toolbar?.updateLoadingIndicatorState(loading); } function webViewerScrollModeChanged(evt) { @@ -2565,6 +2568,14 @@ function webViewerPageChanging({ pageNumber, pageLabel }) { pageNumber ); } + + // Show/hide the loading indicator in the page number input element. + const currentPage = PDFViewerApplication.pdfViewer.getPageView( + /* index = */ pageNumber - 1 + ); + PDFViewerApplication.toolbar?.updateLoadingIndicatorState( + currentPage?.renderingState === RenderingStates.RUNNING + ); } function webViewerResolutionChange(evt) {