Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the PDFSidebar implementation #14962

Merged
merged 3 commits into from
May 29, 2022
Merged

Improve the PDFSidebar implementation #14962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13fda7c
Remove the `view`-specific getters in the `PDFSidebar` class
Snuffleupagus May 26, 2022
d289da7
Re-factor the `PDFSidebar.{setInitialView, switchView}` methods (PR 1…
Snuffleupagus May 26, 2022
c0e7a45
Convert the `PDFSidebar` class to use private methods
Snuffleupagus May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions web/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},

Expand Down Expand Up @@ -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
);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
Expand Down
116 changes: 40 additions & 76 deletions web/pdf_sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -98,13 +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;
Expand All @@ -120,22 +122,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}.
Expand All @@ -149,13 +135,15 @@ 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;
}
// 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._dispatchEvent();
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();
}
}

Expand All @@ -166,47 +154,38 @@ 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;

switch (view) {
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;
}
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.
Expand Down Expand Up @@ -238,16 +217,15 @@ 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();
this._forceRendering();
this.#updateThumbnailViewer();
this.#forceRendering();
}
if (isViewChanged) {
this._dispatchEvent();
this.#dispatchEvent();
}
return isViewChanged;
}

open() {
Expand All @@ -261,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() {
Expand All @@ -280,8 +258,8 @@ class PDFSidebar {
this.outerContainer.classList.add("sidebarMoving");
this.outerContainer.classList.remove("sidebarOpen");

this._forceRendering();
this._dispatchEvent();
this.#forceRendering();
this.#dispatchEvent();
}

toggle() {
Expand All @@ -292,20 +270,18 @@ class PDFSidebar {
}
}

/**
* @private
*/
_dispatchEvent() {
#dispatchEvent() {
if (this.isInitialViewSet && !this.isInitialEventDispatched) {
this.isInitialEventDispatched = true;
}

this.eventBus.dispatch("sidebarviewchanged", {
source: this,
view: this.visibleView,
});
}

/**
* @private
*/
_forceRendering() {
#forceRendering() {
if (this.onToggled) {
this.onToggled();
} else {
Expand All @@ -315,10 +291,7 @@ class PDFSidebar {
}
}

/**
* @private
*/
_updateThumbnailViewer() {
#updateThumbnailViewer() {
const { pdfViewer, pdfThumbnailViewer } = this;

// Use the rendered pages to set the corresponding thumbnail images.
Expand All @@ -333,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;
});
Expand All @@ -348,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.
Expand All @@ -365,10 +332,7 @@ class PDFSidebar {
}
}

/**
* @private
*/
_addEventListeners() {
#addEventListeners() {
this.sidebarContainer.addEventListener("transitionend", evt => {
if (evt.target === this.sidebarContainer) {
this.outerContainer.classList.remove("sidebarMoving");
Expand Down Expand Up @@ -412,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.
Expand Down Expand Up @@ -447,9 +411,9 @@ class PDFSidebar {
this.eventBus._on("presentationmodechanged", evt => {
if (
evt.state === PresentationModeState.NORMAL &&
this.isThumbnailViewVisible
this.visibleView === SidebarView.THUMBS
) {
this._updateThumbnailViewer();
this.#updateThumbnailViewer();
}
});
}
Expand Down