From 236871c68b1330be9a62ae1534f7a53d9efe2f0e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 1 Oct 2018 17:41:57 +0200 Subject: [PATCH 1/2] [Regression] Restore the ability to start searching before a document has loaded, and ignore searches for previously opened documents (PR 10099 follow-up) For many years it's been possible to enter a search term into the findbar(s) before the document has finised loading, such that searching starts immediately once it has loaded. PR 10099 accidentally broke that, which I unfortunately missed during reviewing. Since searching is asynchronous you cannot directly check in `executeCommand` if the document is loaded/current, but need to wait until searching is actually enabled first. Furthermore this patch also ensures that the `_findTimeout` is always correctly cleared given that it adds further asynchronous behaviour to searching, since you obviously only want to deal with searches relevant to the current document. --- web/pdf_find_controller.js | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index cbd50dbb4..c103da0ba 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -110,9 +110,7 @@ class PDFFindController { } executeCommand(cmd, state) { - if (!this._pdfDocument) { - return; - } + const pdfDocument = this._pdfDocument; if (this._state === null || cmd !== 'findagain') { this._dirtyMatch = true; @@ -121,14 +119,25 @@ class PDFFindController { this._updateUIState(FindState.PENDING); this._firstPagePromise.then(() => { + if (!this._pdfDocument || + (pdfDocument && this._pdfDocument !== pdfDocument)) { + // If the document was closed before searching began, or if the search + // operation was relevant for a previously opened document, do nothing. + return; + } this._extractText(); - clearTimeout(this._findTimeout); + if (this._findTimeout) { + clearTimeout(this._findTimeout); + this._findTimeout = null; + } if (cmd === 'find') { // Trigger the find action with a small delay to avoid starting the // search when the user is still typing (saving resources). - this._findTimeout = - setTimeout(this._nextMatch.bind(this), FIND_TIMEOUT); + this._findTimeout = setTimeout(() => { + this._nextMatch(); + this._findTimeout = null; + }, FIND_TIMEOUT); } else { this._nextMatch(); } @@ -156,14 +165,19 @@ class PDFFindController { this._pendingFindMatches = Object.create(null); this._resumePageIdx = null; this._dirtyMatch = false; + clearTimeout(this._findTimeout); this._findTimeout = null; this._firstPagePromise = new Promise((resolve) => { - const eventBus = this._eventBus; - eventBus.on('pagesinit', function onPagesInit() { - eventBus.off('pagesinit', onPagesInit); + const onPagesInit = () => { + if (!this._pdfDocument) { + throw new Error( + 'PDFFindController: `setDocument()` should have been called.'); + } + this._eventBus.off('pagesinit', onPagesInit); resolve(); - }); + }; + this._eventBus.on('pagesinit', onPagesInit); }); } From 6be4921eaf5b1c5a51fa8f6aead8824ca9e632f9 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 1 Oct 2018 18:31:27 +0200 Subject: [PATCH 2/2] Make the clearing of find highlights, when closing the findbar, asynchronous Since searching itself is an asynchronous operation, removal of highlights needs to be asynchronous too since otherwise there's a risk that the events happen in the wrong order and find highlights thus remain visible. Also, this patch will now ensure that only 'findbarclose' events for the *current* document is handled since other ones doesn't really matter. Note in particular that when no document is loaded text-layers are, obviously, not present and subsequently it's unnecessary to attempt to hide non-existent find highlights. --- web/pdf_find_controller.js | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index c103da0ba..93b5d31d0 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -58,15 +58,7 @@ class PDFFindController { this._eventBus = eventBus; this._reset(); - - eventBus.on('findbarclose', () => { - this._highlightMatches = false; - - eventBus.dispatch('updatetextlayermatches', { - source: this, - pageIndex: -1, - }); - }); + eventBus.on('findbarclose', this._onFindBarClose.bind(this)); // Compile the regular expression for text normalization once. const replace = Object.keys(CHARACTERS_TO_NORMALIZE).join(''); @@ -556,6 +548,32 @@ class PDFFindController { } } + _onFindBarClose(evt) { + const pdfDocument = this._pdfDocument; + // Since searching is asynchronous, ensure that the removal of highlighted + // matches (from the UI) is async too such that the 'updatetextlayermatches' + // events will always be dispatched in the expected order. + this._firstPagePromise.then(() => { + if (!this._pdfDocument || + (pdfDocument && this._pdfDocument !== pdfDocument)) { + // Only update the UI if the document is open, and is the current one. + return; + } + if (this._findTimeout) { + clearTimeout(this._findTimeout); + this._findTimeout = null; + // Avoid the UI being in a pending state if the findbar is re-opened. + this._updateUIState(FindState.FOUND); + } + this._highlightMatches = false; + + this._eventBus.dispatch('updatetextlayermatches', { + source: this, + pageIndex: -1, + }); + }); + } + _requestMatchesCount() { const { pageIdx, matchIdx, } = this._selected; let current = 0, total = this._matchesCountTotal;