From 234b57bb45ac8be7d7b9866382db92781a0358eb Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Tue, 26 Dec 2023 10:56:38 +0100
Subject: [PATCH 1/2] Remove the internal "toolbarreset" event and slightly
 re-factor the code

With modern JavaScript class features we can move the relevant event handling into private methods, and thus invoke it directly when resetting the toolbar UI-state.

*Please note:* This patch slightly reduces the size of the `web/toolbar.js` file.
---
 web/toolbar.js | 160 +++++++++++++++++++++++--------------------------
 1 file changed, 74 insertions(+), 86 deletions(-)

diff --git a/web/toolbar.js b/web/toolbar.js
index 3bf9e3fa9..45570687f 100644
--- a/web/toolbar.js
+++ b/web/toolbar.js
@@ -43,14 +43,16 @@ import {
  */
 
 class Toolbar {
+  #opts;
+
   /**
    * @param {ToolbarOptions} options
    * @param {EventBus} eventBus
    */
   constructor(options, eventBus) {
-    this.toolbar = options.container;
+    this.#opts = options;
     this.eventBus = eventBus;
-    this.buttons = [
+    const buttons = [
       { element: options.previous, eventName: "previouspage" },
       { element: options.next, eventName: "nextpage" },
       { element: options.zoomIn, eventName: "zoomin" },
@@ -106,22 +108,12 @@ class Toolbar {
         },
       },
     ];
-    this.items = {
-      numPages: options.numPages,
-      pageNumber: options.pageNumber,
-      scaleSelect: options.scaleSelect,
-      customScaleOption: options.customScaleOption,
-      previous: options.previous,
-      next: options.next,
-      zoomIn: options.zoomIn,
-      zoomOut: options.zoomOut,
-    };
 
     // Bind the event listeners for click and various other actions.
-    this.#bindListeners(options);
+    this.#bindListeners(buttons);
 
     if (options.editorHighlightColorPicker) {
-      this.eventBus._on(
+      eventBus._on(
         "annotationeditoruimanager",
         ({ uiManager }) => {
           this.#setAnnotationEditorUIManager(
@@ -172,18 +164,19 @@ class Toolbar {
     this.updateLoadingIndicatorState();
 
     // Reset the Editor buttons too, since they're document specific.
-    this.eventBus.dispatch("toolbarreset", { source: this });
+    this.#editorModeChanged({ mode: AnnotationEditorType.DISABLE });
   }
 
-  #bindListeners(options) {
-    const { pageNumber, scaleSelect } = this.items;
+  #bindListeners(buttons) {
+    const { eventBus } = this;
+    const { pageNumber, scaleSelect } = this.#opts;
     const self = this;
 
     // The buttons within the toolbar.
-    for (const { element, eventName, eventDetails } of this.buttons) {
+    for (const { element, eventName, eventDetails } of buttons) {
       element.addEventListener("click", evt => {
         if (eventName !== null) {
-          this.eventBus.dispatch(eventName, {
+          eventBus.dispatch(eventName, {
             source: this,
             ...eventDetails,
             // evt.detail is the number of clicks.
@@ -197,7 +190,7 @@ class Toolbar {
       this.select();
     });
     pageNumber.addEventListener("change", function () {
-      self.eventBus.dispatch("pagenumberchanged", {
+      eventBus.dispatch("pagenumberchanged", {
         source: self,
         value: this.value,
       });
@@ -207,15 +200,14 @@ class Toolbar {
       if (this.value === "custom") {
         return;
       }
-      self.eventBus.dispatch("scalechanged", {
+      eventBus.dispatch("scalechanged", {
         source: self,
         value: this.value,
       });
     });
     // Here we depend on browsers dispatching the "click" event *after* the
     // "change" event, when the <select>-element changes.
-    scaleSelect.addEventListener("click", function (evt) {
-      const target = evt.target;
+    scaleSelect.addEventListener("click", function ({ target }) {
       // Remove focus when an <option>-element was *clicked*, to improve the UX
       // for mouse users (fixes bug 1300525 and issue 4923).
       if (
@@ -228,95 +220,92 @@ class Toolbar {
     // Suppress context menus for some controls.
     scaleSelect.oncontextmenu = noContextMenu;
 
-    this.#bindEditorToolsListener(options);
+    eventBus._on(
+      "annotationeditormodechanged",
+      this.#editorModeChanged.bind(this)
+    );
   }
 
-  #bindEditorToolsListener({
-    editorFreeTextButton,
-    editorFreeTextParamsToolbar,
-    editorHighlightButton,
-    editorHighlightParamsToolbar,
-    editorInkButton,
-    editorInkParamsToolbar,
-    editorStampButton,
-    editorStampParamsToolbar,
-  }) {
-    const editorModeChanged = ({ mode }) => {
-      toggleCheckedBtn(
-        editorFreeTextButton,
-        mode === AnnotationEditorType.FREETEXT,
-        editorFreeTextParamsToolbar
-      );
-      toggleCheckedBtn(
-        editorHighlightButton,
-        mode === AnnotationEditorType.HIGHLIGHT,
-        editorHighlightParamsToolbar
-      );
-      toggleCheckedBtn(
-        editorInkButton,
-        mode === AnnotationEditorType.INK,
-        editorInkParamsToolbar
-      );
-      toggleCheckedBtn(
-        editorStampButton,
-        mode === AnnotationEditorType.STAMP,
-        editorStampParamsToolbar
-      );
+  #editorModeChanged({ mode }) {
+    const {
+      editorFreeTextButton,
+      editorFreeTextParamsToolbar,
+      editorHighlightButton,
+      editorHighlightParamsToolbar,
+      editorInkButton,
+      editorInkParamsToolbar,
+      editorStampButton,
+      editorStampParamsToolbar,
+    } = this.#opts;
 
-      const isDisable = mode === AnnotationEditorType.DISABLE;
-      editorFreeTextButton.disabled = isDisable;
-      editorHighlightButton.disabled = isDisable;
-      editorInkButton.disabled = isDisable;
-      editorStampButton.disabled = isDisable;
-    };
-    this.eventBus._on("annotationeditormodechanged", editorModeChanged);
+    toggleCheckedBtn(
+      editorFreeTextButton,
+      mode === AnnotationEditorType.FREETEXT,
+      editorFreeTextParamsToolbar
+    );
+    toggleCheckedBtn(
+      editorHighlightButton,
+      mode === AnnotationEditorType.HIGHLIGHT,
+      editorHighlightParamsToolbar
+    );
+    toggleCheckedBtn(
+      editorInkButton,
+      mode === AnnotationEditorType.INK,
+      editorInkParamsToolbar
+    );
+    toggleCheckedBtn(
+      editorStampButton,
+      mode === AnnotationEditorType.STAMP,
+      editorStampParamsToolbar
+    );
 
-    this.eventBus._on("toolbarreset", evt => {
-      if (evt.source === this) {
-        editorModeChanged({ mode: AnnotationEditorType.DISABLE });
-      }
-    });
+    const isDisable = mode === AnnotationEditorType.DISABLE;
+    editorFreeTextButton.disabled = isDisable;
+    editorHighlightButton.disabled = isDisable;
+    editorInkButton.disabled = isDisable;
+    editorStampButton.disabled = isDisable;
   }
 
   #updateUIState(resetNumPages = false) {
-    const { pageNumber, pagesCount, pageScaleValue, pageScale, items } = this;
+    const { pageNumber, pagesCount, pageScaleValue, pageScale } = this;
+    const opts = this.#opts;
 
     if (resetNumPages) {
       if (this.hasPageLabels) {
-        items.pageNumber.type = "text";
+        opts.pageNumber.type = "text";
 
-        items.numPages.setAttribute("data-l10n-id", "pdfjs-page-of-pages");
+        opts.numPages.setAttribute("data-l10n-id", "pdfjs-page-of-pages");
       } else {
-        items.pageNumber.type = "number";
+        opts.pageNumber.type = "number";
 
-        items.numPages.setAttribute("data-l10n-id", "pdfjs-of-pages");
-        items.numPages.setAttribute(
+        opts.numPages.setAttribute("data-l10n-id", "pdfjs-of-pages");
+        opts.numPages.setAttribute(
           "data-l10n-args",
           JSON.stringify({ pagesCount })
         );
       }
-      items.pageNumber.max = pagesCount;
+      opts.pageNumber.max = pagesCount;
     }
 
     if (this.hasPageLabels) {
-      items.pageNumber.value = this.pageLabel;
+      opts.pageNumber.value = this.pageLabel;
 
-      items.numPages.setAttribute(
+      opts.numPages.setAttribute(
         "data-l10n-args",
         JSON.stringify({ pageNumber, pagesCount })
       );
     } else {
-      items.pageNumber.value = pageNumber;
+      opts.pageNumber.value = pageNumber;
     }
 
-    items.previous.disabled = pageNumber <= 1;
-    items.next.disabled = pageNumber >= pagesCount;
+    opts.previous.disabled = pageNumber <= 1;
+    opts.next.disabled = pageNumber >= pagesCount;
 
-    items.zoomOut.disabled = pageScale <= MIN_SCALE;
-    items.zoomIn.disabled = pageScale >= MAX_SCALE;
+    opts.zoomOut.disabled = pageScale <= MIN_SCALE;
+    opts.zoomIn.disabled = pageScale >= MAX_SCALE;
 
     let predefinedValueFound = false;
-    for (const option of items.scaleSelect.options) {
+    for (const option of opts.scaleSelect.options) {
       if (option.value !== pageScaleValue) {
         option.selected = false;
         continue;
@@ -325,8 +314,8 @@ class Toolbar {
       predefinedValueFound = true;
     }
     if (!predefinedValueFound) {
-      items.customScaleOption.selected = true;
-      items.customScaleOption.setAttribute(
+      opts.customScaleOption.selected = true;
+      opts.customScaleOption.setAttribute(
         "data-l10n-args",
         JSON.stringify({
           scale: Math.round(pageScale * 10000) / 100,
@@ -336,8 +325,7 @@ class Toolbar {
   }
 
   updateLoadingIndicatorState(loading = false) {
-    const { pageNumber } = this.items;
-
+    const { pageNumber } = this.#opts;
     pageNumber.classList.toggle("loading", loading);
   }
 }

From b1ca27016217b641f3287c33b31292fb67a2dffc Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Tue, 26 Dec 2023 10:56:48 +0100
Subject: [PATCH 2/2] Remove the internal "secondarytoolbarreset" event and
 slightly re-factor the code

With modern JavaScript class features we can move the relevant event handling into private methods, and thus invoke it directly when resetting the toolbar UI-state.

*Please note:* This patch slightly reduces the size of the `web/secondary_toolbar.js` file.
---
 web/secondary_toolbar.js | 156 +++++++++++++++++++--------------------
 1 file changed, 74 insertions(+), 82 deletions(-)

diff --git a/web/secondary_toolbar.js b/web/secondary_toolbar.js
index f05ee3f08..d4c3c9541 100644
--- a/web/secondary_toolbar.js
+++ b/web/secondary_toolbar.js
@@ -52,14 +52,15 @@ import { PagesCountLimit } from "./pdf_viewer.js";
  */
 
 class SecondaryToolbar {
+  #opts;
+
   /**
    * @param {SecondaryToolbarOptions} options
    * @param {EventBus} eventBus
    */
   constructor(options, eventBus) {
-    this.toolbar = options.toolbar;
-    this.toggleButton = options.toggleButton;
-    this.buttons = [
+    this.#opts = options;
+    const buttons = [
       {
         element: options.presentationModeButton,
         eventName: "presentationmode",
@@ -141,28 +142,19 @@ class SecondaryToolbar {
       },
     ];
     if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
-      this.buttons.push({
+      buttons.push({
         element: options.openFileButton,
         eventName: "openfile",
         close: true,
       });
     }
-    this.items = {
-      firstPage: options.firstPageButton,
-      lastPage: options.lastPageButton,
-      pageRotateCw: options.pageRotateCwButton,
-      pageRotateCcw: options.pageRotateCcwButton,
-    };
 
     this.eventBus = eventBus;
     this.opened = false;
 
     // Bind the event listeners for click, cursor tool, and scroll/spread mode
     // actions.
-    this.#bindClickListeners();
-    this.#bindCursorToolsListener(options);
-    this.#bindScrollModeListener(options);
-    this.#bindSpreadModeListener(options);
+    this.#bindListeners(buttons);
 
     this.reset();
   }
@@ -190,30 +182,40 @@ class SecondaryToolbar {
     this.#updateUIState();
 
     // Reset the Scroll/Spread buttons too, since they're document specific.
-    this.eventBus.dispatch("secondarytoolbarreset", { source: this });
+    this.#scrollModeChanged({ mode: ScrollMode.VERTICAL });
+    this.#spreadModeChanged({ mode: SpreadMode.NONE });
   }
 
   #updateUIState() {
-    this.items.firstPage.disabled = this.pageNumber <= 1;
-    this.items.lastPage.disabled = this.pageNumber >= this.pagesCount;
-    this.items.pageRotateCw.disabled = this.pagesCount === 0;
-    this.items.pageRotateCcw.disabled = this.pagesCount === 0;
+    const {
+      firstPageButton,
+      lastPageButton,
+      pageRotateCwButton,
+      pageRotateCcwButton,
+    } = this.#opts;
+
+    firstPageButton.disabled = this.pageNumber <= 1;
+    lastPageButton.disabled = this.pageNumber >= this.pagesCount;
+    pageRotateCwButton.disabled = this.pagesCount === 0;
+    pageRotateCcwButton.disabled = this.pagesCount === 0;
   }
 
-  #bindClickListeners() {
+  #bindListeners(buttons) {
+    const { eventBus } = this;
+    const { toggleButton } = this.#opts;
     // Button to toggle the visibility of the secondary toolbar.
-    this.toggleButton.addEventListener("click", this.toggle.bind(this));
+    toggleButton.addEventListener("click", this.toggle.bind(this));
 
     // All items within the secondary toolbar.
-    for (const { element, eventName, close, eventDetails } of this.buttons) {
+    for (const { element, eventName, close, eventDetails } of buttons) {
       element.addEventListener("click", evt => {
         if (eventName !== null) {
-          this.eventBus.dispatch(eventName, { source: this, ...eventDetails });
+          eventBus.dispatch(eventName, { source: this, ...eventDetails });
         }
         if (close) {
           this.close();
         }
-        this.eventBus.dispatch("reporttelemetry", {
+        eventBus.dispatch("reporttelemetry", {
           source: this,
           details: {
             type: "buttons",
@@ -222,72 +224,58 @@ class SecondaryToolbar {
         });
       });
     }
+
+    eventBus._on("cursortoolchanged", this.#cursorToolChanged.bind(this));
+    eventBus._on("scrollmodechanged", this.#scrollModeChanged.bind(this));
+    eventBus._on("spreadmodechanged", this.#spreadModeChanged.bind(this));
   }
 
-  #bindCursorToolsListener({ cursorSelectToolButton, cursorHandToolButton }) {
-    this.eventBus._on("cursortoolchanged", ({ tool }) => {
-      toggleCheckedBtn(cursorSelectToolButton, tool === CursorTool.SELECT);
-      toggleCheckedBtn(cursorHandToolButton, tool === CursorTool.HAND);
-    });
+  #cursorToolChanged({ tool }) {
+    const { cursorSelectToolButton, cursorHandToolButton } = this.#opts;
+
+    toggleCheckedBtn(cursorSelectToolButton, tool === CursorTool.SELECT);
+    toggleCheckedBtn(cursorHandToolButton, tool === CursorTool.HAND);
   }
 
-  #bindScrollModeListener({
-    scrollPageButton,
-    scrollVerticalButton,
-    scrollHorizontalButton,
-    scrollWrappedButton,
-    spreadNoneButton,
-    spreadOddButton,
-    spreadEvenButton,
-  }) {
-    const scrollModeChanged = ({ mode }) => {
-      toggleCheckedBtn(scrollPageButton, mode === ScrollMode.PAGE);
-      toggleCheckedBtn(scrollVerticalButton, mode === ScrollMode.VERTICAL);
-      toggleCheckedBtn(scrollHorizontalButton, mode === ScrollMode.HORIZONTAL);
-      toggleCheckedBtn(scrollWrappedButton, mode === ScrollMode.WRAPPED);
+  #scrollModeChanged({ mode }) {
+    const {
+      scrollPageButton,
+      scrollVerticalButton,
+      scrollHorizontalButton,
+      scrollWrappedButton,
+      spreadNoneButton,
+      spreadOddButton,
+      spreadEvenButton,
+    } = this.#opts;
 
-      // Permanently *disable* the Scroll buttons when PAGE-scrolling is being
-      // enforced for *very* long/large documents; please see the `BaseViewer`.
-      const forceScrollModePage =
-        this.pagesCount > PagesCountLimit.FORCE_SCROLL_MODE_PAGE;
-      scrollPageButton.disabled = forceScrollModePage;
-      scrollVerticalButton.disabled = forceScrollModePage;
-      scrollHorizontalButton.disabled = forceScrollModePage;
-      scrollWrappedButton.disabled = forceScrollModePage;
+    toggleCheckedBtn(scrollPageButton, mode === ScrollMode.PAGE);
+    toggleCheckedBtn(scrollVerticalButton, mode === ScrollMode.VERTICAL);
+    toggleCheckedBtn(scrollHorizontalButton, mode === ScrollMode.HORIZONTAL);
+    toggleCheckedBtn(scrollWrappedButton, mode === ScrollMode.WRAPPED);
 
-      // Temporarily *disable* the Spread buttons when horizontal scrolling is
-      // enabled, since the non-default Spread modes doesn't affect the layout.
-      const isHorizontal = mode === ScrollMode.HORIZONTAL;
-      spreadNoneButton.disabled = isHorizontal;
-      spreadOddButton.disabled = isHorizontal;
-      spreadEvenButton.disabled = isHorizontal;
-    };
-    this.eventBus._on("scrollmodechanged", scrollModeChanged);
+    // Permanently *disable* the Scroll buttons when PAGE-scrolling is being
+    // enforced for *very* long/large documents; please see the `BaseViewer`.
+    const forceScrollModePage =
+      this.pagesCount > PagesCountLimit.FORCE_SCROLL_MODE_PAGE;
+    scrollPageButton.disabled = forceScrollModePage;
+    scrollVerticalButton.disabled = forceScrollModePage;
+    scrollHorizontalButton.disabled = forceScrollModePage;
+    scrollWrappedButton.disabled = forceScrollModePage;
 
-    this.eventBus._on("secondarytoolbarreset", evt => {
-      if (evt.source === this) {
-        scrollModeChanged({ mode: ScrollMode.VERTICAL });
-      }
-    });
+    // Temporarily *disable* the Spread buttons when horizontal scrolling is
+    // enabled, since the non-default Spread modes doesn't affect the layout.
+    const isHorizontal = mode === ScrollMode.HORIZONTAL;
+    spreadNoneButton.disabled = isHorizontal;
+    spreadOddButton.disabled = isHorizontal;
+    spreadEvenButton.disabled = isHorizontal;
   }
 
-  #bindSpreadModeListener({
-    spreadNoneButton,
-    spreadOddButton,
-    spreadEvenButton,
-  }) {
-    const spreadModeChanged = ({ mode }) => {
-      toggleCheckedBtn(spreadNoneButton, mode === SpreadMode.NONE);
-      toggleCheckedBtn(spreadOddButton, mode === SpreadMode.ODD);
-      toggleCheckedBtn(spreadEvenButton, mode === SpreadMode.EVEN);
-    };
-    this.eventBus._on("spreadmodechanged", spreadModeChanged);
+  #spreadModeChanged({ mode }) {
+    const { spreadNoneButton, spreadOddButton, spreadEvenButton } = this.#opts;
 
-    this.eventBus._on("secondarytoolbarreset", evt => {
-      if (evt.source === this) {
-        spreadModeChanged({ mode: SpreadMode.NONE });
-      }
-    });
+    toggleCheckedBtn(spreadNoneButton, mode === SpreadMode.NONE);
+    toggleCheckedBtn(spreadOddButton, mode === SpreadMode.ODD);
+    toggleCheckedBtn(spreadEvenButton, mode === SpreadMode.EVEN);
   }
 
   open() {
@@ -295,7 +283,9 @@ class SecondaryToolbar {
       return;
     }
     this.opened = true;
-    toggleExpandedBtn(this.toggleButton, true, this.toolbar);
+
+    const { toggleButton, toolbar } = this.#opts;
+    toggleExpandedBtn(toggleButton, true, toolbar);
   }
 
   close() {
@@ -303,7 +293,9 @@ class SecondaryToolbar {
       return;
     }
     this.opened = false;
-    toggleExpandedBtn(this.toggleButton, false, this.toolbar);
+
+    const { toggleButton, toolbar } = this.#opts;
+    toggleExpandedBtn(toggleButton, false, toolbar);
   }
 
   toggle() {