From 13fda7caeb4904f3e3162529073a2896db1dd0cc Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
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 90c492dfd..13f8dfa97 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 e2c0b201f..534b38e39 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 <jonas.jenwald@gmail.com>
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 534b38e39..1aae69849 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 <jonas.jenwald@gmail.com>
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 1aae69849..9e69a7327 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();
}
});
}