From dd4620530d89381746ea974e5eb1c39f7f966542 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 2 Feb 2019 10:03:30 +0100 Subject: [PATCH 1/5] Change `PDFSidebar.switchView` to act as a wrapper for a "private" `PDFSidebar._switchView` method The new "private" method will return a boolean, indicating if the `sidebarviewchanged` event was dispatched, thus allowing some simplification of the `PDFSidebar.setInitialView` method. --- web/pdf_sidebar.js | 47 ++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/web/pdf_sidebar.js b/web/pdf_sidebar.js index 27a77ad59..ebc73321b 100644 --- a/web/pdf_sidebar.js +++ b/web/pdf_sidebar.js @@ -130,18 +130,15 @@ class PDFSidebar { } this.isInitialViewSet = true; - if (this.isOpen && view === SidebarView.NONE) { + // If the user has already manually opened the sidebar, immediately closing + // it would be bad UX. + if (view === SidebarView.NONE) { this._dispatchEvent(); - // If the user has already manually opened the sidebar, - // immediately closing it would be bad UX. return; } - let isViewPreserved = (view === this.visibleView); - this.switchView(view, /* forceOpen */ true); - - if (isViewPreserved) { - // Prevent dispatching two back-to-back `sidebarviewchanged` events, - // since `this.switchView` dispatched the event if the view changed. + // 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(); } } @@ -153,14 +150,24 @@ class PDFSidebar { * The default value is `false`. */ switchView(view, forceOpen = false) { - if (view === SidebarView.NONE) { - this.close(); - return; - } - let isViewChanged = (view !== this.active); + 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; case SidebarView.THUMBS: this.thumbnailButton.classList.add('toggled'); this.outlineButton.classList.remove('toggled'); @@ -177,7 +184,7 @@ class PDFSidebar { break; case SidebarView.OUTLINE: if (this.outlineButton.disabled) { - return; + return false; } this.thumbnailButton.classList.remove('toggled'); this.outlineButton.classList.add('toggled'); @@ -189,7 +196,7 @@ class PDFSidebar { break; case SidebarView.ATTACHMENTS: if (this.attachmentsButton.disabled) { - return; + return false; } this.thumbnailButton.classList.remove('toggled'); this.outlineButton.classList.remove('toggled'); @@ -200,9 +207,8 @@ class PDFSidebar { this.attachmentsView.classList.remove('hidden'); break; default: - console.error('PDFSidebar_switchView: "' + view + - '" is an unsupported value.'); - return; + console.error(`PDFSidebar._switchView: "${view}" is not a valid view.`); + return false; } // Update the active view *after* it has been validated above, // in order to prevent setting it to an invalid state. @@ -210,7 +216,7 @@ class PDFSidebar { if (forceOpen && !this.isOpen) { this.open(); - return; // NOTE: Opening will trigger rendering, and dispatch the event. + return true; // Opening will trigger rendering and dispatch the event. } if (shouldForceRendering) { this._forceRendering(); @@ -219,6 +225,7 @@ class PDFSidebar { this._dispatchEvent(); } this._hideUINotification(this.active); + return isViewChanged; } open() { From 6806248030fdc24b985a610e6dc4123b50c73a7a Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 27 Jan 2019 12:07:38 +0100 Subject: [PATCH 2/5] Modify a number of the viewer preferences, whose current default value is `0`, such that they behave as expected with the view history The intention with preferences such as `sidebarViewOnLoad`/`scrollModeOnLoad`/`spreadModeOnLoad` were always that they should be able to *unconditionally* override their view history counterparts. Due to the way that these preferences were initially implemented[1], trying to e.g. force the sidebar to remain hidden on load cannot be guaranteed[2]. The reason for this is the use of "enumeration values" containing zero, which in hindsight was an unfortunate choice on my part. At this point it's also not as simple as just re-numbering the affected structures, since that would wreak havoc on existing (modified) preferences. The only reasonable solution that I was able to come up with was to change the *default* values of the preferences themselves, but not their actual values or the meaning thereof. As part of the refactoring, the `disablePageMode` preference was combined with the *adjusted* `sidebarViewOnLoad` one, to hopefully reduce confusion by not tracking related state separately. Additionally, the `showPreviousViewOnLoad` and `disableOpenActionDestination` preferences were combined into a *new* `viewOnLoad` enumeration preference, to further avoid tracking related state separately. --- extensions/chromium/options/options.html | 16 +++++ extensions/chromium/preferences_schema.json | 33 ++++++---- web/app.js | 71 ++++++++++++--------- web/app_options.js | 26 +++----- web/base_viewer.js | 27 ++------ web/default_preferences.json | 10 ++- web/pdf_sidebar.js | 8 ++- web/secondary_toolbar.js | 3 +- web/ui_utils.js | 28 ++++++++ 9 files changed, 132 insertions(+), 90 deletions(-) diff --git a/extensions/chromium/options/options.html b/extensions/chromium/options/options.html index 412e80bb1..57ddae7b1 100644 --- a/extensions/chromium/options/options.html +++ b/extensions/chromium/options/options.html @@ -43,6 +43,19 @@ body { + +