From af99d1dc08a535dbecaed8bb465a89ad907d8e26 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 30 Oct 2018 11:08:03 +0100 Subject: [PATCH 1/3] Attempt to improve readability of `PDFFindController.executeCommand` by (slightly) refactoring the code responsible for calling `PDFFindController._nextMatch` Unfortunately the `PDFFindController.executeCommand` method has now become a bit more complicated than one would like, but hopefully this small change will improve the structure somewhat (especially for subsequent patches). --- web/pdf_find_controller.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 624017844..2411a9705 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -128,6 +128,8 @@ class PDFFindController { } this._extractText(); + const findbarClosed = !this._highlightMatches; + if (this._findTimeout) { clearTimeout(this._findTimeout); this._findTimeout = null; @@ -139,14 +141,16 @@ class PDFFindController { this._nextMatch(); this._findTimeout = null; }, FIND_TIMEOUT); - } else if (cmd === 'findagain' && !this._dirtyMatch) { - const updateHighlightAll = (!this._highlightMatches && - this._state.highlightAll); + } else if (this._dirtyMatch) { + // 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') { this._nextMatch(); // When the findbar was previously closed, and `highlightAll` is set, // ensure that the matches on all active pages are highlighted again. - if (updateHighlightAll) { + if (findbarClosed && this._state.highlightAll) { this._updateAllPages(); } } else { From d7941b4ce741619bcd917dc3c9a762701f355fbc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 3 Nov 2018 11:52:48 +0100 Subject: [PATCH 2/3] Add a helper method, in `PDFFindController`, to determine if matches need to be re-calculated when a new search operation occurs --- web/pdf_find_controller.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 2411a9705..7279a6ae5 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -113,7 +113,7 @@ class PDFFindController { executeCommand(cmd, state) { const pdfDocument = this._pdfDocument; - if (this._state === null || cmd !== 'findagain') { + if (this._state === null || this._shouldDirtyMatch(cmd)) { this._dirtyMatch = true; } this._state = state; @@ -198,6 +198,14 @@ class PDFFindController { return this._normalizedQuery; } + _shouldDirtyMatch(cmd) { + switch (cmd) { + case 'findagain': + return false; + } + return true; + } + /** * Helper for multi-term search that fills the `matchesWithLength` array * and handles cases where one search term includes another search term (for From d805d799ffad7d27f9938143b67a75020a7d40b3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 30 Oct 2018 11:08:26 +0100 Subject: [PATCH 3/3] For repeated 'findagain' operations, attempt to reset the search position if the user has e.g. scrolled in the document (issue 4141) Currently we'll only attempt to start from the current page when a new search is done, however for 'findagain' operations we'll always continue from the last match position. This could easily lead to confusing behaviour if the user has scrolled to a completely different part of the document. In an attempt to improve this somewhat, for repeated 'findagain' operations, we'll instead reset the position to the current page when it's *absolutely* certain that the user has scrolled. Note that this required adding a new `BaseViewer` method, and exposing that through `PDFLinkService`, in order to check if a given page is visible. In an attempt to avoid issues, in custom implementations of `PDFFindController`, the code checks for the existence of the `PDFLinkService.isPageVisible` method *before* using it. --- web/base_viewer.js | 17 +++++++++++++++++ web/interfaces.js | 5 +++++ web/pdf_find_controller.js | 15 +++++++++++++++ web/pdf_link_service.js | 14 ++++++++++++++ 4 files changed, 51 insertions(+) diff --git a/web/base_viewer.js b/web/base_viewer.js index 9588d75dc..19b3c344a 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -878,6 +878,23 @@ class BaseViewer { throw new Error('Not implemented: _getVisiblePages'); } + /** + * @param {number} pageNumber + */ + isPageVisible(pageNumber) { + if (!this.pdfDocument) { + return false; + } + if (this.pageNumber < 1 || pageNumber > this.pagesCount) { + console.error( + `${this._name}.isPageVisible: "${pageNumber}" is out of bounds.`); + return false; + } + return this._getVisiblePages().views.some(function(view) { + return (view.id === pageNumber); + }); + } + cleanup() { for (let i = 0, ii = this._pages.length; i < ii; i++) { if (this._pages[i] && diff --git a/web/interfaces.js b/web/interfaces.js index 170622681..55631555e 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -77,6 +77,11 @@ class IPDFLinkService { * @param {Object} pageRef - reference to the page. */ cachePageRef(pageNum, pageRef) {} + + /** + * @param {number} pageNumber + */ + isPageVisible(pageNumber) {} } /** diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 7279a6ae5..3c1785124 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -201,6 +201,21 @@ class PDFFindController { _shouldDirtyMatch(cmd) { switch (cmd) { case 'findagain': + const pageNumber = this._selected.pageIdx + 1; + const linkService = this._linkService; + // Only treat a 'findagain' event as a new search operation when it's + // *absolutely* certain that the currently selected match is no longer + // visible, e.g. as a result of the user scrolling in the document. + // + // NOTE: If only a simple `this._linkService.page` check was used here, + // there's a risk that consecutive 'findagain' operations could "skip" + // over matches at the top/bottom of pages thus making them completely + // inaccessible when there's multiple pages visible in the viewer. + if (pageNumber >= 1 && pageNumber <= linkService.pagesCount && + linkService.page !== pageNumber && linkService.isPageVisible && + !linkService.isPageVisible(pageNumber)) { + break; + } return false; } return true; diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 9fe9427b5..44bd88a34 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -352,6 +352,13 @@ class PDFLinkService { let refStr = pageRef.num + ' ' + pageRef.gen + ' R'; return (this._pagesRefCache && this._pagesRefCache[refStr]) || null; } + + /** + * @param {number} pageNumber + */ + isPageVisible(pageNumber) { + return this.pdfViewer.isPageVisible(pageNumber); + } } function isValidExplicitDestination(dest) { @@ -483,6 +490,13 @@ class SimpleLinkService { * @param {Object} pageRef - reference to the page. */ cachePageRef(pageNum, pageRef) {} + + /** + * @param {number} pageNumber + */ + isPageVisible(pageNumber) { + return true; + } } export {