From 6806248030fdc24b985a610e6dc4123b50c73a7a Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Sun, 27 Jan 2019 12:07:38 +0100
Subject: [PATCH] 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 {
 </div>
 </template>
 
+<template id="viewOnLoad-template">
+<div class="settings-row">
+  <label>
+    <span></span>
+    <select>
+      <option value="-1">Default</option>
+      <option value="0">Show previous position</option>
+      <option value="1">Show initial position</option>
+    </select>
+  </label>
+</div>
+</template>
+
 <template id="defaultZoomValue-template">
 <div class="settings-row">
   <label>
@@ -71,6 +84,7 @@ body {
   <label>
     <span></span>
     <select>
+      <option value="-1">Default</option>
       <option value="0">Do not show sidebar</option>
       <option value="1">Show thumbnails in sidebar</option>
       <option value="2">Show document outline in sidebar</option>
@@ -125,6 +139,7 @@ body {
   <label>
     <span></span>
     <select>
+      <option value="-1">Default</option>
       <option value="0">Vertical scrolling</option>
       <option value="1">Horizontal scrolling</option>
       <option value="2">Wrapped scrolling</option>
@@ -138,6 +153,7 @@ body {
   <label>
     <span></span>
     <select>
+      <option value="-1">Default</option>
       <option value="0">No spreads</option>
       <option value="1">Odd spreads</option>
       <option value="2">Even spreads</option>
diff --git a/extensions/chromium/preferences_schema.json b/extensions/chromium/preferences_schema.json
index eb264dee2..2ce2a53ba 100644
--- a/extensions/chromium/preferences_schema.json
+++ b/extensions/chromium/preferences_schema.json
@@ -2,11 +2,20 @@
   "type": "object",
   "properties": {
     "showPreviousViewOnLoad": {
-      "title": "Show previous position of PDF upon load",
-      "description": "Whether to view PDF documents in the last page and position upon opening the viewer.",
+      "description": "DEPRECATED. Set viewOnLoad to 1 to disable showing the last page/position on load.",
       "type": "boolean",
       "default": true
     },
+    "viewOnLoad": {
+      "title": "View position on load",
+      "description": "The position in the document upon load.\n -1 = Default (uses OpenAction if available, otherwise equal to `viewOnLoad = 0`).\n 0 = The last viewed page/position.\n 1 = The initial page/position.",
+      "enum": [
+        -1,
+        0,
+        1
+      ],
+      "default": 0
+    },
     "defaultZoomValue": {
       "title": "Default zoom level",
       "description": "Default zoom level of the viewer. Accepted values: 'auto', 'page-actual', 'page-width', 'page-height', 'page-fit', or a zoom level in percents.",
@@ -16,15 +25,16 @@
     },
     "sidebarViewOnLoad": {
       "title": "Sidebar state on load",
-      "description": "Controls the state of the sidebar upon load.\n 0 = do not show sidebar.\n 1 = show thumbnails in sidebar.\n 2 = show document outline in sidebar.\n 3 = Show attachments in sidebar.",
+      "description": "Controls the state of the sidebar upon load.\n -1 = Default (uses PageMode if available, otherwise the last position if available/enabled).\n 0 = Do not show sidebar.\n 1 = Show thumbnails in sidebar.\n 2 = Show document outline in sidebar.\n 3 = Show attachments in sidebar.",
       "type": "integer",
       "enum": [
+        -1,
         0,
         1,
         2,
         3
       ],
-      "default": 0
+      "default": -1
     },
     "enableHandToolOnLoad": {
       "description": "DEPRECATED. Set cursorToolOnLoad to 1 to enable the hand tool by default.",
@@ -117,15 +127,12 @@
       ],
       "default": 0
     },
-    "disableOpenActionDestination": {
-      "type": "boolean",
-      "default": true
-    },
     "disablePageLabels": {
       "type": "boolean",
       "default": false
     },
     "disablePageMode": {
+      "description": "DEPRECATED.",
       "type": "boolean",
       "default": false
     },
@@ -159,25 +166,27 @@
     },
     "scrollModeOnLoad": {
       "title": "Scroll mode on load",
-      "description": "Controls how the viewer scrolls upon load.\n 0 = Vertical scrolling.\n 1 = Horizontal scrolling.\n 2 = Wrapped scrolling.",
+      "description": "Controls how the viewer scrolls upon load.\n -1 = Default (uses the last position if available/enabled).\n 0 = Vertical scrolling.\n 1 = Horizontal scrolling.\n 2 = Wrapped scrolling.",
       "type": "integer",
       "enum": [
+        -1,
         0,
         1,
         2
       ],
-      "default": 0
+      "default": -1
     },
     "spreadModeOnLoad": {
       "title": "Spread mode on load",
-      "description": "Whether the viewer should join pages into spreads upon load.\n 0 = No spreads.\n 1 = Odd spreads.\n 2 = Even spreads.",
+      "description": "Whether the viewer should join pages into spreads upon load.\n -1 = Default (uses the last position if available/enabled).\n 0 = No spreads.\n 1 = Odd spreads.\n 2 = Even spreads.",
       "type": "integer",
       "enum": [
+        -1,
         0,
         1,
         2
       ],
-      "default": 0
+      "default": -1
     }
   }
 }
diff --git a/web/app.js b/web/app.js
index 0d09e67ef..fc4de3110 100644
--- a/web/app.js
+++ b/web/app.js
@@ -16,9 +16,10 @@
 
 import {
   animationStarted, DEFAULT_SCALE_VALUE, getGlobalEventBus,
-  getPDFFileNameFromURL, isValidRotation, MAX_SCALE, MIN_SCALE,
-  noContextMenuHandler, normalizeWheelEventDelta, parseQueryString,
-  PresentationModeState, ProgressBar, RendererType, TextLayerMode
+  getPDFFileNameFromURL, isValidRotation, isValidScrollMode, isValidSpreadMode,
+  MAX_SCALE, MIN_SCALE, noContextMenuHandler, normalizeWheelEventDelta,
+  parseQueryString, PresentationModeState, ProgressBar, RendererType,
+  ScrollMode, SpreadMode, TextLayerMode
 } from './ui_utils';
 import {
   build, createObjectURL, getDocument, getFilenameFromUrl, GlobalWorkerOptions,
@@ -52,6 +53,12 @@ const DISABLE_AUTO_FETCH_LOADING_BAR_TIMEOUT = 5000; // ms
 const FORCE_PAGES_LOADED_TIMEOUT = 10000; // ms
 const WHEEL_ZOOM_DISABLED_TIMEOUT = 1000; // ms
 
+const ViewOnLoad = {
+  UNKNOWN: -1,
+  PREVIOUS: 0, // Default value.
+  INITIAL: 1,
+};
+
 const DefaultExternalServices = {
   updateFindControlState(data) {},
   updateFindMatchesCount(data) {},
@@ -905,7 +912,7 @@ let PDFViewerApplication = {
         // i.e. not when it is embedded in a web page.
         this.pdfHistory.initialize({
           fingerprint: pdfDocument.fingerprint,
-          resetHistory: !AppOptions.get('showPreviousViewOnLoad'),
+          resetHistory: AppOptions.get('viewOnLoad') === ViewOnLoad.INITIAL,
           updateUrl: AppOptions.get('historyUpdateUrl'),
         });
 
@@ -916,23 +923,25 @@ let PDFViewerApplication = {
         }
       }
 
-      let storePromise = store.getMultiple({
+      const storePromise = store.getMultiple({
         page: null,
         zoom: DEFAULT_SCALE_VALUE,
         scrollLeft: '0',
         scrollTop: '0',
         rotation: null,
-        sidebarView: SidebarView.NONE,
-        scrollMode: null,
-        spreadMode: null,
+        sidebarView: SidebarView.UNKNOWN,
+        scrollMode: ScrollMode.UNKNOWN,
+        spreadMode: SpreadMode.UNKNOWN,
       }).catch(() => { /* Unable to read from storage; ignoring errors. */ });
 
       Promise.all([
         storePromise, pageModePromise, openActionDestPromise,
       ]).then(async ([values = {}, pageMode, openActionDest]) => {
+        const viewOnLoad = AppOptions.get('viewOnLoad');
+
+        // Always let the browser history/document hash take precedence.
         if (openActionDest && !this.initialBookmark &&
-            !AppOptions.get('disableOpenActionDestination')) {
-          // Always let the browser history/document hash take precedence.
+            viewOnLoad === ViewOnLoad.UNKNOWN) {
           this.initialBookmark = JSON.stringify(openActionDest);
           // TODO: Re-factor the `PDFHistory` initialization to remove this hack
           // that's currently necessary to prevent weird initial history state.
@@ -940,6 +949,7 @@ let PDFViewerApplication = {
                                  pageNumber: null, });
         }
         const initialBookmark = this.initialBookmark;
+
         // Initialize the default values, from user preferences.
         const zoom = AppOptions.get('defaultZoomValue');
         let hash = zoom ? `zoom=${zoom}` : null;
@@ -949,18 +959,25 @@ let PDFViewerApplication = {
         let scrollMode = AppOptions.get('scrollModeOnLoad');
         let spreadMode = AppOptions.get('spreadModeOnLoad');
 
-        if (values.page && AppOptions.get('showPreviousViewOnLoad')) {
-          hash = 'page=' + values.page + '&zoom=' + (zoom || values.zoom) +
-            ',' + values.scrollLeft + ',' + values.scrollTop;
+        if (values.page && viewOnLoad !== ViewOnLoad.INITIAL) {
+          hash = `page=${values.page}&zoom=${zoom || values.zoom},` +
+                 `${values.scrollLeft},${values.scrollTop}`;
 
           rotation = parseInt(values.rotation, 10);
-          sidebarView = sidebarView || (values.sidebarView | 0);
-          scrollMode = scrollMode || (values.scrollMode | 0);
-          spreadMode = spreadMode || (values.spreadMode | 0);
+          // Always let user preferences take precedence over the view history.
+          if (sidebarView === SidebarView.UNKNOWN) {
+            sidebarView = (values.sidebarView | 0);
+          }
+          if (scrollMode === ScrollMode.UNKNOWN) {
+            scrollMode = (values.scrollMode | 0);
+          }
+          if (spreadMode === SpreadMode.UNKNOWN) {
+            spreadMode = (values.spreadMode | 0);
+          }
         }
-        if (pageMode && !AppOptions.get('disablePageMode')) {
-          // Always let the user preference/history take precedence.
-          sidebarView = sidebarView || apiPageModeToSidebarView(pageMode);
+        // Always let the user preference/view history take precedence.
+        if (pageMode && sidebarView === SidebarView.UNKNOWN) {
+          sidebarView = apiPageModeToSidebarView(pageMode);
         }
 
         this.setInitialView(hash, {
@@ -1149,28 +1166,24 @@ let PDFViewerApplication = {
 
   setInitialView(storedHash, { rotation, sidebarView,
                                scrollMode, spreadMode, } = {}) {
-    let setRotation = (angle) => {
+    const setRotation = (angle) => {
       if (isValidRotation(angle)) {
         this.pdfViewer.pagesRotation = angle;
       }
     };
-    let setViewerModes = (scroll, spread) => {
-      if (Number.isInteger(scroll)) {
+    const setViewerModes = (scroll, spread) => {
+      if (isValidScrollMode(scroll)) {
         this.pdfViewer.scrollMode = scroll;
       }
-      if (Number.isInteger(spread)) {
+      if (isValidSpreadMode(spread)) {
         this.pdfViewer.spreadMode = spread;
       }
     };
-
-    // Putting these before isInitialViewSet = true prevents these values from
-    // being stored in the document history (and overriding any future changes
-    // made to the corresponding global preferences), just this once.
-    setViewerModes(scrollMode, spreadMode);
-
     this.isInitialViewSet = true;
     this.pdfSidebar.setInitialView(sidebarView);
 
+    setViewerModes(scrollMode, spreadMode);
+
     if (this.initialBookmark) {
       setRotation(this.initialRotation);
       delete this.initialRotation;
diff --git a/web/app_options.js b/web/app_options.js
index 7e9864623..9a593eed3 100644
--- a/web/app_options.js
+++ b/web/app_options.js
@@ -48,21 +48,11 @@ const defaultOptions = {
     value: false,
     kind: OptionKind.VIEWER,
   },
-  disableOpenActionDestination: {
-    /** @type {boolean} */
-    value: true,
-    kind: OptionKind.VIEWER,
-  },
   disablePageLabels: {
     /** @type {boolean} */
     value: false,
     kind: OptionKind.VIEWER,
   },
-  disablePageMode: {
-    /** @type {boolean} */
-    value: false,
-    kind: OptionKind.VIEWER,
-  },
   /**
    * The `disablePreferences` is, conditionally, defined below.
    */
@@ -124,24 +114,19 @@ const defaultOptions = {
     value: false,
     kind: OptionKind.VIEWER,
   },
-  showPreviousViewOnLoad: {
-    /** @type {boolean} */
-    value: true,
-    kind: OptionKind.VIEWER,
-  },
   sidebarViewOnLoad: {
     /** @type {number} */
-    value: 0,
+    value: -1,
     kind: OptionKind.VIEWER,
   },
   scrollModeOnLoad: {
     /** @type {number} */
-    value: 0,
+    value: -1,
     kind: OptionKind.VIEWER,
   },
   spreadModeOnLoad: {
     /** @type {number} */
-    value: 0,
+    value: -1,
     kind: OptionKind.VIEWER,
   },
   textLayerMode: {
@@ -154,6 +139,11 @@ const defaultOptions = {
     value: false,
     kind: OptionKind.VIEWER,
   },
+  viewOnLoad: {
+    /** @type {boolean} */
+    value: 0,
+    kind: OptionKind.VIEWER,
+  },
 
   cMapPacked: {
     /** @type {boolean} */
diff --git a/web/base_viewer.js b/web/base_viewer.js
index d3f03214c..337790939 100644
--- a/web/base_viewer.js
+++ b/web/base_viewer.js
@@ -15,10 +15,11 @@
 
 import {
   CSS_UNITS, DEFAULT_SCALE, DEFAULT_SCALE_VALUE, getGlobalEventBus,
-  getVisibleElements, isPortraitOrientation, isValidRotation, MAX_AUTO_SCALE,
-  moveToEndOfArray, NullL10n, PresentationModeState, RendererType,
-  SCROLLBAR_PADDING, scrollIntoView, TextLayerMode, UNKNOWN_SCALE,
-  VERTICAL_PADDING, watchScroll
+  getVisibleElements, isPortraitOrientation, isValidRotation, isValidScrollMode,
+  isValidSpreadMode, MAX_AUTO_SCALE, moveToEndOfArray, NullL10n,
+  PresentationModeState, RendererType, SCROLLBAR_PADDING, scrollIntoView,
+  ScrollMode, SpreadMode, TextLayerMode, UNKNOWN_SCALE, VERTICAL_PADDING,
+  watchScroll
 } from './ui_utils';
 import { PDFRenderingQueue, RenderingStates } from './pdf_rendering_queue';
 import { AnnotationLayerBuilder } from './annotation_layer_builder';
@@ -29,18 +30,6 @@ import { TextLayerBuilder } from './text_layer_builder';
 
 const DEFAULT_CACHE_SIZE = 10;
 
-const ScrollMode = {
-  VERTICAL: 0, // The default value.
-  HORIZONTAL: 1,
-  WRAPPED: 2,
-};
-
-const SpreadMode = {
-  NONE: 0, // The default value.
-  ODD: 1,
-  EVEN: 2,
-};
-
 /**
  * @typedef {Object} PDFViewerOptions
  * @property {HTMLDivElement} container - The container for the viewer element.
@@ -1086,7 +1075,7 @@ class BaseViewer {
     if (this._scrollMode === mode) {
       return; // The Scroll mode didn't change.
     }
-    if (!Number.isInteger(mode) || !Object.values(ScrollMode).includes(mode)) {
+    if (!isValidScrollMode(mode)) {
       throw new Error(`Invalid scroll mode: ${mode}`);
     }
     this._scrollMode = mode;
@@ -1132,7 +1121,7 @@ class BaseViewer {
     if (this._spreadMode === mode) {
       return; // The Spread mode didn't change.
     }
-    if (!Number.isInteger(mode) || !Object.values(SpreadMode).includes(mode)) {
+    if (!isValidSpreadMode(mode)) {
       throw new Error(`Invalid spread mode: ${mode}`);
     }
     this._spreadMode = mode;
@@ -1179,6 +1168,4 @@ class BaseViewer {
 
 export {
   BaseViewer,
-  ScrollMode,
-  SpreadMode,
 };
diff --git a/web/default_preferences.json b/web/default_preferences.json
index 702b5d5a9..22a115b44 100644
--- a/web/default_preferences.json
+++ b/web/default_preferences.json
@@ -1,7 +1,7 @@
 {
-  "showPreviousViewOnLoad": true,
+  "viewOnLoad": 0,
   "defaultZoomValue": "",
-  "sidebarViewOnLoad": 0,
+  "sidebarViewOnLoad": -1,
   "cursorToolOnLoad": 0,
   "enableWebGL": false,
   "eventBusDispatchToDOM": false,
@@ -16,10 +16,8 @@
   "renderer": "canvas",
   "renderInteractiveForms": false,
   "enablePrintAutoRotate": false,
-  "disableOpenActionDestination": true,
-  "disablePageMode": false,
   "disablePageLabels": false,
   "historyUpdateUrl": false,
-  "scrollModeOnLoad": 0,
-  "spreadModeOnLoad": 0
+  "scrollModeOnLoad": -1,
+  "spreadModeOnLoad": -1
 }
diff --git a/web/pdf_sidebar.js b/web/pdf_sidebar.js
index ebc73321b..80c5b3144 100644
--- a/web/pdf_sidebar.js
+++ b/web/pdf_sidebar.js
@@ -19,10 +19,12 @@ import { RenderingStates } from './pdf_rendering_queue';
 const UI_NOTIFICATION_CLASS = 'pdfSidebarNotification';
 
 const SidebarView = {
+  UNKNOWN: -1,
   NONE: 0,
-  THUMBS: 1,
+  THUMBS: 1, // Default value.
   OUTLINE: 2,
   ATTACHMENTS: 3,
+  LAYERS: 4,
 };
 
 /**
@@ -131,8 +133,8 @@ class PDFSidebar {
     this.isInitialViewSet = true;
 
     // If the user has already manually opened the sidebar, immediately closing
-    // it would be bad UX.
-    if (view === SidebarView.NONE) {
+    // it would be bad UX; also ignore the "unknown" sidebar view value.
+    if (view === SidebarView.NONE || view === SidebarView.UNKNOWN) {
       this._dispatchEvent();
       return;
     }
diff --git a/web/secondary_toolbar.js b/web/secondary_toolbar.js
index b9f6f6701..4c4c679e7 100644
--- a/web/secondary_toolbar.js
+++ b/web/secondary_toolbar.js
@@ -13,10 +13,9 @@
  * limitations under the License.
  */
 
-import { ScrollMode, SpreadMode } from './base_viewer';
+import { SCROLLBAR_PADDING, ScrollMode, SpreadMode } from './ui_utils';
 import { CursorTool } from './pdf_cursor_tools';
 import { PDFSinglePageViewer } from './pdf_single_page_viewer';
-import { SCROLLBAR_PADDING } from './ui_utils';
 
 /**
  * @typedef {Object} SecondaryToolbarOptions
diff --git a/web/ui_utils.js b/web/ui_utils.js
index f2d9bb058..e1235759a 100644
--- a/web/ui_utils.js
+++ b/web/ui_utils.js
@@ -41,6 +41,20 @@ const TextLayerMode = {
   ENABLE_ENHANCE: 2,
 };
 
+const ScrollMode = {
+  UNKNOWN: -1,
+  VERTICAL: 0, // Default value.
+  HORIZONTAL: 1,
+  WRAPPED: 2,
+};
+
+const SpreadMode = {
+  UNKNOWN: -1,
+  NONE: 0, // Default value.
+  ODD: 1,
+  EVEN: 2,
+};
+
 // Replaces {{arguments}} with their values.
 function formatL10nValue(text, args) {
   if (!args) {
@@ -602,6 +616,16 @@ function isValidRotation(angle) {
   return Number.isInteger(angle) && angle % 90 === 0;
 }
 
+function isValidScrollMode(mode) {
+  return (Number.isInteger(mode) && Object.values(ScrollMode).includes(mode) &&
+          mode !== ScrollMode.UNKNOWN);
+}
+
+function isValidSpreadMode(mode) {
+  return (Number.isInteger(mode) && Object.values(SpreadMode).includes(mode) &&
+          mode !== SpreadMode.UNKNOWN);
+}
+
 function isPortraitOrientation(size) {
   return size.width <= size.height;
 }
@@ -863,10 +887,14 @@ export {
   SCROLLBAR_PADDING,
   VERTICAL_PADDING,
   isValidRotation,
+  isValidScrollMode,
+  isValidSpreadMode,
   isPortraitOrientation,
   PresentationModeState,
   RendererType,
   TextLayerMode,
+  ScrollMode,
+  SpreadMode,
   NullL10n,
   EventBus,
   getGlobalEventBus,