From 13fda7caeb4904f3e3162529073a2896db1dd0cc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 26 May 2022 11:58:36 +0200 Subject: [PATCH 1/3] Remove the `view`-specific getters in the `PDFSidebar` class With the exception of `isThumbnailViewVisible`, these getters are completely unused. Generally speaking, using the `visibleView`-getter directly works just as well and seems (at least to me) to be overall preferable considering how our classes are usually implemented. --- web/app.js | 16 +++++++--------- web/pdf_sidebar.js | 18 +----------------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/web/app.js b/web/app.js index 90c492dfdd31d..13f8dfa97fc6d 100644 --- a/web/app.js +++ b/web/app.js @@ -1765,7 +1765,7 @@ const PDFViewerApplication = { forceRendering() { this.pdfRenderingQueue.printing = !!this.printService; this.pdfRenderingQueue.isThumbnailViewEnabled = - this.pdfSidebar.isThumbnailViewVisible; + this.pdfSidebar.visibleView === SidebarView.THUMBS; this.pdfRenderingQueue.renderHighestPriority(); }, @@ -2261,7 +2261,7 @@ function webViewerPageRendered({ pageNumber, error }) { } // Use the rendered page to set the corresponding thumbnail image. - if (PDFViewerApplication.pdfSidebar.isThumbnailViewVisible) { + if (PDFViewerApplication.pdfSidebar.visibleView === SidebarView.THUMBS) { const pageView = PDFViewerApplication.pdfViewer.getPageView( /* index = */ pageNumber - 1 ); @@ -2338,21 +2338,19 @@ function webViewerPresentationModeChanged(evt) { PDFViewerApplication.pdfViewer.presentationModeState = evt.state; } -function webViewerSidebarViewChanged(evt) { +function webViewerSidebarViewChanged({ view }) { PDFViewerApplication.pdfRenderingQueue.isThumbnailViewEnabled = - PDFViewerApplication.pdfSidebar.isThumbnailViewVisible; + view === SidebarView.THUMBS; if (PDFViewerApplication.isInitialViewSet) { // Only update the storage when the document has been loaded *and* rendered. - PDFViewerApplication.store?.set("sidebarView", evt.view).catch(() => { + PDFViewerApplication.store?.set("sidebarView", view).catch(() => { // Unable to write to storage. }); } } -function webViewerUpdateViewarea(evt) { - const location = evt.location; - +function webViewerUpdateViewarea({ location }) { if (PDFViewerApplication.isInitialViewSet) { // Only update the storage when the document has been loaded *and* rendered. PDFViewerApplication.store @@ -2587,7 +2585,7 @@ function webViewerPageChanging({ pageNumber, pageLabel }) { PDFViewerApplication.toolbar.setPageNumber(pageNumber, pageLabel); PDFViewerApplication.secondaryToolbar.setPageNumber(pageNumber); - if (PDFViewerApplication.pdfSidebar.isThumbnailViewVisible) { + if (PDFViewerApplication.pdfSidebar.visibleView === SidebarView.THUMBS) { PDFViewerApplication.pdfThumbnailViewer.scrollThumbnailIntoView(pageNumber); } } diff --git a/web/pdf_sidebar.js b/web/pdf_sidebar.js index e2c0b201fc26e..534b38e3947b7 100644 --- a/web/pdf_sidebar.js +++ b/web/pdf_sidebar.js @@ -120,22 +120,6 @@ class PDFSidebar { return this.isOpen ? this.active : SidebarView.NONE; } - get isThumbnailViewVisible() { - return this.isOpen && this.active === SidebarView.THUMBS; - } - - get isOutlineViewVisible() { - return this.isOpen && this.active === SidebarView.OUTLINE; - } - - get isAttachmentsViewVisible() { - return this.isOpen && this.active === SidebarView.ATTACHMENTS; - } - - get isLayersViewVisible() { - return this.isOpen && this.active === SidebarView.LAYERS; - } - /** * @param {number} view - The sidebar view that should become visible, * must be one of the values in {SidebarView}. @@ -447,7 +431,7 @@ class PDFSidebar { this.eventBus._on("presentationmodechanged", evt => { if ( evt.state === PresentationModeState.NORMAL && - this.isThumbnailViewVisible + this.visibleView === SidebarView.THUMBS ) { this._updateThumbnailViewer(); } From d289da76a78e4ef1c4ad11e4fbf7485db23498f6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 26 May 2022 12:16:56 +0200 Subject: [PATCH 2/3] Re-factor the `PDFSidebar.{setInitialView, switchView}` methods (PR 10502 follow-up) This removes the internal `_switchView`-method, since looking at all of this again it feels simpler to instead track the initial event dispatching. --- web/pdf_sidebar.js | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/web/pdf_sidebar.js b/web/pdf_sidebar.js index 534b38e3947b7..1aae698494c6b 100644 --- a/web/pdf_sidebar.js +++ b/web/pdf_sidebar.js @@ -68,6 +68,7 @@ class PDFSidebar { this.isOpen = false; this.active = SidebarView.THUMBS; this.isInitialViewSet = false; + this.isInitialEventDispatched = false; /** * Callback used when the sidebar has been opened/closed, to ensure that @@ -103,6 +104,7 @@ class PDFSidebar { reset() { this.isInitialViewSet = false; + this.isInitialEventDispatched = false; this._hideUINotification(/* reset = */ true); this.switchView(SidebarView.THUMBS); @@ -136,9 +138,11 @@ class PDFSidebar { this._dispatchEvent(); return; } - // Prevent dispatching two back-to-back `sidebarviewchanged` events, - // since `this._switchView` dispatched the event if the view changed. - if (!this._switchView(view, /* forceOpen */ true)) { + this.switchView(view, /* forceOpen = */ true); + + // Prevent dispatching two back-to-back "sidebarviewchanged" events, + // since `this.switchView` dispatched the event if the view changed. + if (!this.isInitialEventDispatched) { this._dispatchEvent(); } } @@ -150,14 +154,6 @@ class PDFSidebar { * The default value is `false`. */ switchView(view, forceOpen = false) { - this._switchView(view, forceOpen); - } - - /** - * @returns {boolean} Indicating if `this._dispatchEvent` was called. - * @private - */ - _switchView(view, forceOpen = false) { const isViewChanged = view !== this.active; let shouldForceRendering = false; @@ -165,9 +161,8 @@ class PDFSidebar { case SidebarView.NONE: if (this.isOpen) { this.close(); - return true; // Closing will trigger rendering and dispatch the event. } - return false; + return; // Closing will trigger rendering and dispatch the event. case SidebarView.THUMBS: if (this.isOpen && isViewChanged) { shouldForceRendering = true; @@ -175,22 +170,22 @@ class PDFSidebar { break; case SidebarView.OUTLINE: if (this.outlineButton.disabled) { - return false; + return; } break; case SidebarView.ATTACHMENTS: if (this.attachmentsButton.disabled) { - return false; + return; } break; case SidebarView.LAYERS: if (this.layersButton.disabled) { - return false; + return; } break; default: - console.error(`PDFSidebar._switchView: "${view}" is not a valid view.`); - return false; + console.error(`PDFSidebar.switchView: "${view}" is not a valid view.`); + return; } // Update the active view *after* it has been validated above, // in order to prevent setting it to an invalid state. @@ -222,7 +217,7 @@ class PDFSidebar { if (forceOpen && !this.isOpen) { this.open(); - return true; // Opening will trigger rendering and dispatch the event. + return; // Opening will trigger rendering and dispatch the event. } if (shouldForceRendering) { this._updateThumbnailViewer(); @@ -231,7 +226,6 @@ class PDFSidebar { if (isViewChanged) { this._dispatchEvent(); } - return isViewChanged; } open() { @@ -280,6 +274,10 @@ class PDFSidebar { * @private */ _dispatchEvent() { + if (this.isInitialViewSet && !this.isInitialEventDispatched) { + this.isInitialEventDispatched = true; + } + this.eventBus.dispatch("sidebarviewchanged", { source: this, view: this.visibleView, From c0e7a454a1bcc8290721229e6a8b7c9988d60b7c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 26 May 2022 12:30:36 +0200 Subject: [PATCH 3/3] Convert the `PDFSidebar` class to use private methods --- web/pdf_sidebar.js | 60 ++++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/web/pdf_sidebar.js b/web/pdf_sidebar.js index 1aae698494c6b..9e69a73272ece 100644 --- a/web/pdf_sidebar.js +++ b/web/pdf_sidebar.js @@ -99,14 +99,14 @@ class PDFSidebar { this.eventBus = eventBus; this.l10n = l10n; - this._addEventListeners(); + this.#addEventListeners(); } reset() { this.isInitialViewSet = false; this.isInitialEventDispatched = false; - this._hideUINotification(/* reset = */ true); + this.#hideUINotification(/* reset = */ true); this.switchView(SidebarView.THUMBS); this.outlineButton.disabled = false; @@ -135,7 +135,7 @@ class PDFSidebar { // If the user has already manually opened the sidebar, immediately closing // it would be bad UX; also ignore the "unknown" sidebar view value. if (view === SidebarView.NONE || view === SidebarView.UNKNOWN) { - this._dispatchEvent(); + this.#dispatchEvent(); return; } this.switchView(view, /* forceOpen = */ true); @@ -143,7 +143,7 @@ class PDFSidebar { // Prevent dispatching two back-to-back "sidebarviewchanged" events, // since `this.switchView` dispatched the event if the view changed. if (!this.isInitialEventDispatched) { - this._dispatchEvent(); + this.#dispatchEvent(); } } @@ -220,11 +220,11 @@ class PDFSidebar { return; // Opening will trigger rendering and dispatch the event. } if (shouldForceRendering) { - this._updateThumbnailViewer(); - this._forceRendering(); + this.#updateThumbnailViewer(); + this.#forceRendering(); } if (isViewChanged) { - this._dispatchEvent(); + this.#dispatchEvent(); } } @@ -239,12 +239,12 @@ class PDFSidebar { this.outerContainer.classList.add("sidebarMoving", "sidebarOpen"); if (this.active === SidebarView.THUMBS) { - this._updateThumbnailViewer(); + this.#updateThumbnailViewer(); } - this._forceRendering(); - this._dispatchEvent(); + this.#forceRendering(); + this.#dispatchEvent(); - this._hideUINotification(); + this.#hideUINotification(); } close() { @@ -258,8 +258,8 @@ class PDFSidebar { this.outerContainer.classList.add("sidebarMoving"); this.outerContainer.classList.remove("sidebarOpen"); - this._forceRendering(); - this._dispatchEvent(); + this.#forceRendering(); + this.#dispatchEvent(); } toggle() { @@ -270,10 +270,7 @@ class PDFSidebar { } } - /** - * @private - */ - _dispatchEvent() { + #dispatchEvent() { if (this.isInitialViewSet && !this.isInitialEventDispatched) { this.isInitialEventDispatched = true; } @@ -284,10 +281,7 @@ class PDFSidebar { }); } - /** - * @private - */ - _forceRendering() { + #forceRendering() { if (this.onToggled) { this.onToggled(); } else { @@ -297,10 +291,7 @@ class PDFSidebar { } } - /** - * @private - */ - _updateThumbnailViewer() { + #updateThumbnailViewer() { const { pdfViewer, pdfThumbnailViewer } = this; // Use the rendered pages to set the corresponding thumbnail images. @@ -315,10 +306,7 @@ class PDFSidebar { pdfThumbnailViewer.scrollThumbnailIntoView(pdfViewer.currentPageNumber); } - /** - * @private - */ - _showUINotification() { + #showUINotification() { this.l10n.get("toggle_sidebar_notification2.title").then(msg => { this.toggleButton.title = msg; }); @@ -330,10 +318,7 @@ class PDFSidebar { } } - /** - * @private - */ - _hideUINotification(reset = false) { + #hideUINotification(reset = false) { if (this.isOpen || reset) { // Only hide the notification on the `toggleButton` if the sidebar is // currently open, or when the current PDF document is being closed. @@ -347,10 +332,7 @@ class PDFSidebar { } } - /** - * @private - */ - _addEventListeners() { + #addEventListeners() { this.sidebarContainer.addEventListener("transitionend", evt => { if (evt.target === this.sidebarContainer) { this.outerContainer.classList.remove("sidebarMoving"); @@ -394,7 +376,7 @@ class PDFSidebar { button.disabled = !count; if (count) { - this._showUINotification(); + this.#showUINotification(); } else if (this.active === view) { // If the `view` was opened by the user during document load, // switch away from it if it turns out to be empty. @@ -431,7 +413,7 @@ class PDFSidebar { evt.state === PresentationModeState.NORMAL && this.visibleView === SidebarView.THUMBS ) { - this._updateThumbnailViewer(); + this.#updateThumbnailViewer(); } }); }