From fa8c0ef6164c7abfd5236e97823102a89517f8a4 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 3 Oct 2021 16:03:51 +0200 Subject: [PATCH] [api-minor] Change `PDFFindController` to use the "find"-event directly (issue 12731) Looking at the code, I do have to agree with the point made in issue 12731 about it being unexpected/unhelpful that the `PDFFindController.executeCommand`-method isn't directly usable with the "find"-event. The reason for it being this way is, as so often, for historical reasons: The `executeCommand`-method was added (just) prior to the introduction of the `EventBus` in the viewer. Obviously we cannot simply change the existing `PDFFindController.executeCommand`-method, since that'd be a breaking change in code which has existed for over five years. Initially I figured that we could simply add a new method in `PDFFindController` that'd accept the state from the "find"-event, however after thinking about this and looking through the use-cases in the default viewer I settled on a slightly different approach: Let the `PDFFindController` just listen for the "find"-event (on the `EventBus`-instance) directly instead, which also removes one level of (unneeded) indirection during searching in the default viewer. For GENERIC builds of the PDF.js library, the old `PDFFindController.executeCommand`-method is still available with a deprecation warning. --- examples/components/simpleviewer.js | 6 ++- examples/components/singlepageviewer.js | 6 ++- test/unit/pdf_find_controller_spec.js | 49 +++++++++++-------------- web/app.js | 38 ++++++------------- web/firefoxcom.js | 4 +- web/pdf_find_bar.js | 2 +- web/pdf_find_controller.js | 41 +++++++++++++++------ 7 files changed, 76 insertions(+), 70 deletions(-) diff --git a/examples/components/simpleviewer.js b/examples/components/simpleviewer.js index 942a80a63..f064a53be 100644 --- a/examples/components/simpleviewer.js +++ b/examples/components/simpleviewer.js @@ -77,7 +77,11 @@ eventBus.on("pagesinit", function () { // We can try searching for things. if (SEARCH_FOR) { - pdfFindController.executeCommand("find", { query: SEARCH_FOR }); + if (!pdfFindController._onFind) { + pdfFindController.executeCommand("find", { query: SEARCH_FOR }); + } else { + eventBus.dispatch("find", { type: "", query: SEARCH_FOR }); + } } }); diff --git a/examples/components/singlepageviewer.js b/examples/components/singlepageviewer.js index 3eac4c0c8..c22db3003 100644 --- a/examples/components/singlepageviewer.js +++ b/examples/components/singlepageviewer.js @@ -77,7 +77,11 @@ eventBus.on("pagesinit", function () { // We can try searching for things. if (SEARCH_FOR) { - pdfFindController.executeCommand("find", { query: SEARCH_FOR }); + if (!pdfFindController._onFind) { + pdfFindController.executeCommand("find", { query: SEARCH_FOR }); + } else { + eventBus.dispatch("find", { type: "", query: SEARCH_FOR }); + } } }); diff --git a/test/unit/pdf_find_controller_spec.js b/test/unit/pdf_find_controller_spec.js index 1b97f47e4..e0d6f6a98 100644 --- a/test/unit/pdf_find_controller_spec.js +++ b/test/unit/pdf_find_controller_spec.js @@ -69,14 +69,27 @@ async function initPdfFindController(filename) { function testSearch({ eventBus, pdfFindController, - parameters, + state, matchesPerPage, selectedMatch, pageMatches = null, pageMatchesLength = null, }) { return new Promise(function (resolve) { - pdfFindController.executeCommand("find", parameters); + const eventState = Object.assign( + Object.create(null), + { + source: this, + type: "", + query: null, + caseSensitive: false, + entireWord: false, + phraseSearch: true, + findPrevious: false, + }, + state + ); + eventBus.dispatch("find", eventState); // The `updatefindmatchescount` event is only emitted if the page contains // at least one match for the query, so the last non-zero item in the @@ -142,12 +155,8 @@ describe("pdf_find_controller", function () { await testSearch({ eventBus, pdfFindController, - parameters: { + state: { query: "Dynamic", - caseSensitive: false, - entireWord: false, - phraseSearch: true, - findPrevious: false, }, matchesPerPage: [11, 5, 0, 3, 0, 0, 0, 1, 1, 1, 0, 3, 4, 4], selectedMatch: { @@ -166,11 +175,8 @@ describe("pdf_find_controller", function () { await testSearch({ eventBus, pdfFindController, - parameters: { + state: { query: "conference", - caseSensitive: false, - entireWord: false, - phraseSearch: true, findPrevious: true, }, matchesPerPage: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5], @@ -187,12 +193,9 @@ describe("pdf_find_controller", function () { await testSearch({ eventBus, pdfFindController, - parameters: { + state: { query: "Dynamic", caseSensitive: true, - entireWord: false, - phraseSearch: true, - findPrevious: false, }, matchesPerPage: [3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 3], selectedMatch: { @@ -210,12 +213,9 @@ describe("pdf_find_controller", function () { await testSearch({ eventBus, pdfFindController, - parameters: { + state: { query: "Government", - caseSensitive: false, entireWord: true, - phraseSearch: true, - findPrevious: false, }, matchesPerPage: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0], selectedMatch: { @@ -233,12 +233,9 @@ describe("pdf_find_controller", function () { await testSearch({ eventBus, pdfFindController, - parameters: { + state: { query: "alternate solution", - caseSensitive: false, - entireWord: false, phraseSearch: false, - findPrevious: false, }, matchesPerPage: [0, 0, 0, 0, 0, 1, 0, 0, 4, 0, 0, 0, 0, 0], selectedMatch: { @@ -256,12 +253,8 @@ describe("pdf_find_controller", function () { await testSearch({ eventBus, pdfFindController, - parameters: { + state: { query: "fraction", - caseSensitive: false, - entireWord: false, - phraseSearch: true, - findPrevious: false, }, matchesPerPage: [3], selectedMatch: { diff --git a/web/app.js b/web/app.js index a804bb04a..3c81f5dbe 100644 --- a/web/app.js +++ b/web/app.js @@ -1929,7 +1929,6 @@ const PDFViewerApplication = { eventBus._on("switchspreadmode", webViewerSwitchSpreadMode); eventBus._on("spreadmodechanged", webViewerSpreadModeChanged); eventBus._on("documentproperties", webViewerDocumentProperties); - eventBus._on("find", webViewerFind); eventBus._on("findfromurlhash", webViewerFindFromUrlHash); eventBus._on("updatefindmatchescount", webViewerUpdateFindMatchesCount); eventBus._on("updatefindcontrolstate", webViewerUpdateFindControlState); @@ -2025,7 +2024,6 @@ const PDFViewerApplication = { eventBus._off("switchspreadmode", webViewerSwitchSpreadMode); eventBus._off("spreadmodechanged", webViewerSpreadModeChanged); eventBus._off("documentproperties", webViewerDocumentProperties); - eventBus._off("find", webViewerFind); eventBus._off("findfromurlhash", webViewerFindFromUrlHash); eventBus._off("updatefindmatchescount", webViewerUpdateFindMatchesCount); eventBus._off("updatefindcontrolstate", webViewerUpdateFindControlState); @@ -2605,19 +2603,10 @@ function webViewerDocumentProperties() { PDFViewerApplication.pdfDocumentProperties.open(); } -function webViewerFind(evt) { - PDFViewerApplication.findController.executeCommand("find" + evt.type, { - query: evt.query, - phraseSearch: evt.phraseSearch, - caseSensitive: evt.caseSensitive, - entireWord: evt.entireWord, - highlightAll: evt.highlightAll, - findPrevious: evt.findPrevious, - }); -} - function webViewerFindFromUrlHash(evt) { - PDFViewerApplication.findController.executeCommand("find", { + PDFViewerApplication.eventBus.dispatch("find", { + source: evt.source, + type: "", query: evt.query, phraseSearch: evt.phraseSearch, caseSensitive: false, @@ -2794,6 +2783,8 @@ function webViewerKeyDown(evt) { if (PDFViewerApplication.overlayManager.active) { return; } + const { eventBus, pdfViewer } = PDFViewerApplication; + const isViewerInPresentationMode = pdfViewer.isInPresentationMode; let handled = false, ensureViewerFocused = false; @@ -2803,9 +2794,6 @@ function webViewerKeyDown(evt) { (evt.shiftKey ? 4 : 0) | (evt.metaKey ? 8 : 0); - const pdfViewer = PDFViewerApplication.pdfViewer; - const isViewerInPresentationMode = pdfViewer?.isInPresentationMode; - // First, handle the key bindings that are independent whether an input // control is selected or not. if (cmd === 1 || cmd === 8 || cmd === 5 || cmd === 12) { @@ -2819,16 +2807,14 @@ function webViewerKeyDown(evt) { break; case 71: // g if (!PDFViewerApplication.supportsIntegratedFind) { - const findState = PDFViewerApplication.findController.state; - if (findState) { - PDFViewerApplication.findController.executeCommand("findagain", { - query: findState.query, - phraseSearch: findState.phraseSearch, - caseSensitive: findState.caseSensitive, - entireWord: findState.entireWord, - highlightAll: findState.highlightAll, + const { state } = PDFViewerApplication.findController; + if (state) { + const eventState = Object.assign(Object.create(null), state, { + source: window, + type: "again", findPrevious: cmd === 5 || cmd === 12, }); + eventBus.dispatch("find", eventState); } handled = true; } @@ -2883,8 +2869,6 @@ function webViewerKeyDown(evt) { } if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC || CHROME")) { - const { eventBus } = PDFViewerApplication; - // CTRL or META without shift if (cmd === 1 || cmd === 8) { switch (evt.keyCode) { diff --git a/web/firefoxcom.js b/web/firefoxcom.js index 129190a74..5b3b2a351 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -219,6 +219,8 @@ class MozL10n { "findentirewordchange", "findbarclose", ]; + const findLen = "find".length; + const handleEvent = function ({ type, detail }) { if (!PDFViewerApplication.initialized) { return; @@ -229,7 +231,7 @@ class MozL10n { } PDFViewerApplication.eventBus.dispatch("find", { source: window, - type: type.substring("find".length), + type: type.substring(findLen), query: detail.query, phraseSearch: true, caseSensitive: !!detail.caseSensitive, diff --git a/web/pdf_find_bar.js b/web/pdf_find_bar.js index 3388711d9..3c0192566 100644 --- a/web/pdf_find_bar.js +++ b/web/pdf_find_bar.js @@ -89,7 +89,7 @@ class PDFFindBar { this.updateUIState(); } - dispatchEvent(type, findPrev) { + dispatchEvent(type, findPrev = false) { this.eventBus.dispatch("find", { source: this, type, diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 91302efc5..9e36a1631 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -104,7 +104,22 @@ class PDFFindController { this._eventBus = eventBus; this._reset(); + eventBus._on("find", this._onFind.bind(this)); eventBus._on("findbarclose", this._onFindBarClose.bind(this)); + + if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { + this.executeCommand = (cmd, state) => { + console.error( + "Deprecated method `PDFFindController.executeCommand` called, " + + 'please dispatch a "find"-event using the EventBus instead.' + ); + + const eventState = Object.assign(Object.create(null), state, { + type: cmd.substring("find".length), + }); + this._onFind(eventState); + }; + } } get highlightMatches() { @@ -144,17 +159,21 @@ class PDFFindController { this._firstPageCapability.resolve(); } - executeCommand(cmd, state) { + /** + * @private + */ + _onFind(state) { if (!state) { return; } const pdfDocument = this._pdfDocument; + const { type } = state; - if (this._state === null || this._shouldDirtyMatch(cmd, state)) { + if (this._state === null || this._shouldDirtyMatch(state)) { this._dirtyMatch = true; } this._state = state; - if (cmd !== "findhighlightallchange") { + if (type !== "highlightallchange") { this._updateUIState(FindState.PENDING); } @@ -176,7 +195,7 @@ class PDFFindController { clearTimeout(this._findTimeout); this._findTimeout = null; } - if (cmd === "find") { + if (!type) { // Trigger the find action with a small delay to avoid starting the // search when the user is still typing (saving resources). this._findTimeout = setTimeout(() => { @@ -187,7 +206,7 @@ class PDFFindController { // Immediately trigger searching for non-'find' operations, when the // current state needs to be reset and matches re-calculated. this._nextMatch(); - } else if (cmd === "findagain") { + } else if (type === "again") { this._nextMatch(); // When the findbar was previously closed, and `highlightAll` is set, @@ -195,7 +214,7 @@ class PDFFindController { if (findbarClosed && this._state.highlightAll) { this._updateAllPages(); } - } else if (cmd === "findhighlightallchange") { + } else if (type === "highlightallchange") { // If there was a pending search operation, synchronously trigger a new // search *first* to ensure that the correct matches are highlighted. if (pendingTimeout) { @@ -275,14 +294,14 @@ class PDFFindController { return this._normalizedQuery; } - _shouldDirtyMatch(cmd, state) { + _shouldDirtyMatch(state) { // When the search query changes, regardless of the actual search command // used, always re-calculate matches to avoid errors (fixes bug 1030622). if (state.query !== this._state.query) { return true; } - switch (cmd) { - case "findagain": + switch (state.type) { + case "again": const pageNumber = this._selected.pageIdx + 1; const linkService = this._linkService; // Only treat a 'findagain' event as a new search operation when it's @@ -302,7 +321,7 @@ class PDFFindController { return true; } return false; - case "findhighlightallchange": + case "highlightallchange": return false; } return true; @@ -797,7 +816,7 @@ class PDFFindController { }); } - _updateUIState(state, previous) { + _updateUIState(state, previous = false) { this._eventBus.dispatch("updatefindcontrolstate", { source: this, state,