From 21d959bb82c838d8662a493f40768ae1ebd0caa2 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Tue, 11 Sep 2018 20:01:50 +0200 Subject: [PATCH 1/5] Remove unused member variable `hadMatch` from the find controller It's only being assigned, but not read anymore. --- web/pdf_find_controller.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index e23068601..a4754265f 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -368,7 +368,6 @@ class PDFFindController { this.selected.pageIdx = this.selected.matchIdx = -1; this.offset.pageIdx = currentPageIndex; this.offset.matchIdx = null; - this.hadMatch = false; this.resumePageIdx = null; this.pageMatches = []; this.matchesCountTotal = 0; @@ -411,7 +410,6 @@ class PDFFindController { (previous && offset.matchIdx > 0)) { // The simple case; we just have advance the matchIdx to select // the next match on the page. - this.hadMatch = true; offset.matchIdx = (previous ? offset.matchIdx - 1 : offset.matchIdx + 1); this._updateMatch(/* found = */ true); @@ -432,7 +430,6 @@ class PDFFindController { if (numMatches) { // There were matches for the page, so initialize `matchIdx`. - this.hadMatch = true; offset.matchIdx = (previous ? numMatches - 1 : 0); this._updateMatch(/* found = */ true); return true; From a859f0eafd01072fd79109cdffcffa05fdaf3d5e Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Tue, 11 Sep 2018 20:14:57 +0200 Subject: [PATCH 2/5] Remove unnecessary `startedTextExtraction` member variable from the find controller The find controller already has quite a lot of state to maintain. We can avoid keeping track of this member variable because when the find controller is reset, so is the extract text promises array. Therefore, we can just check if that array contains items or not to determine if text extraction already started. Moreover, there is no need to reset the `pageContents` array since the `reset` method already takes care of that. --- web/pdf_find_controller.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index a4754265f..e286bf62d 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -62,7 +62,6 @@ class PDFFindController { } reset() { - this.startedTextExtraction = false; this.extractTextPromises = []; this.pendingFindMatches = Object.create(null); this.active = false; // If active, find results will be highlighted. @@ -309,11 +308,10 @@ class PDFFindController { } _extractText() { - if (this.startedTextExtraction) { + // Perform text extraction once if this method is called multiple times. + if (this.extractTextPromises.length > 0) { return; } - this.startedTextExtraction = true; - this.pageContents.length = 0; let promise = Promise.resolve(); for (let i = 0, ii = this.pdfViewer.pagesCount; i < ii; i++) { From 38c9f5fc24d4b2985fa01d4409db1796155d288d Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Wed, 12 Sep 2018 15:10:45 +0200 Subject: [PATCH 3/5] Mark all private members as such in the find controller Moreover, use getters for all members that are only being read. --- web/pdf_find_controller.js | 199 ++++++++++++++++++++----------------- 1 file changed, 108 insertions(+), 91 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index e286bf62d..0689ab202 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -48,8 +48,8 @@ const CHARACTERS_TO_NORMALIZE = { */ class PDFFindController { constructor({ pdfViewer, eventBus = getGlobalEventBus(), }) { - this.pdfViewer = pdfViewer; - this.eventBus = eventBus; + this._pdfViewer = pdfViewer; + this._eventBus = eventBus; this.onUpdateResultsCount = null; this.onUpdateState = null; @@ -58,33 +58,49 @@ class PDFFindController { // Compile the regular expression for text normalization once. let replace = Object.keys(CHARACTERS_TO_NORMALIZE).join(''); - this.normalizationRegex = new RegExp('[' + replace + ']', 'g'); + this._normalizationRegex = new RegExp('[' + replace + ']', 'g'); + } + + get pageMatches() { + return this._pageMatches; + } + + get pageMatchesLength() { + return this._pageMatchesLength; + } + + get selected() { + return this._selected; + } + + get state() { + return this._state; } reset() { - this.extractTextPromises = []; - this.pendingFindMatches = Object.create(null); this.active = false; // If active, find results will be highlighted. - this.pageContents = []; // Stores the text for each page. - this.pageMatches = []; - this.pageMatchesLength = null; - this.matchesCountTotal = 0; - this.selected = { // Currently selected match. + this._pageMatches = []; + this._pageMatchesLength = null; + this._state = null; + this._selected = { // Currently selected match. pageIdx: -1, matchIdx: -1, }; - this.offset = { // Where the find algorithm currently is in the document. + this._offset = { // Where the find algorithm currently is in the document. pageIdx: null, matchIdx: null, }; - this.pagesToSearch = null; - this.resumePageIdx = null; - this.state = null; - this.dirtyMatch = false; - this.findTimeout = null; + this._extractTextPromises = []; + this._pageContents = []; // Stores the text for each page. + this._matchesCountTotal = 0; + this._pagesToSearch = null; + this._pendingFindMatches = Object.create(null); + this._resumePageIdx = null; + this._dirtyMatch = false; + this._findTimeout = null; this._firstPagePromise = new Promise((resolve) => { - const eventBus = this.eventBus; + const eventBus = this._eventBus; eventBus.on('pagesinit', function onPagesInit() { eventBus.off('pagesinit', onPagesInit); resolve(); @@ -93,20 +109,21 @@ class PDFFindController { } executeCommand(cmd, state) { - if (this.state === null || cmd !== 'findagain') { - this.dirtyMatch = true; + if (this._state === null || cmd !== 'findagain') { + this._dirtyMatch = true; } - this.state = state; + this._state = state; this._updateUIState(FindState.PENDING); this._firstPagePromise.then(() => { this._extractText(); - clearTimeout(this.findTimeout); + clearTimeout(this._findTimeout); 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.bind(this), FIND_TIMEOUT); } else { this._nextMatch(); } @@ -134,7 +151,7 @@ class PDFFindController { } _normalize(text) { - return text.replace(this.normalizationRegex, function (ch) { + return text.replace(this._normalizationRegex, function (ch) { return CHARACTERS_TO_NORMALIZE[ch]; }); } @@ -227,7 +244,7 @@ class PDFFindController { } matches.push(matchIdx); } - this.pageMatches[pageIndex] = matches; + this._pageMatches[pageIndex] = matches; } _calculateWordMatch(query, pageIndex, pageContent, entireWord) { @@ -257,24 +274,24 @@ class PDFFindController { } // Prepare arrays for storing the matches. - if (!this.pageMatchesLength) { - this.pageMatchesLength = []; + if (!this._pageMatchesLength) { + this._pageMatchesLength = []; } - this.pageMatchesLength[pageIndex] = []; - this.pageMatches[pageIndex] = []; + this._pageMatchesLength[pageIndex] = []; + this._pageMatches[pageIndex] = []; // Sort `matchesWithLength`, remove intersecting terms and put the result // into the two arrays. - this._prepareMatches(matchesWithLength, this.pageMatches[pageIndex], - this.pageMatchesLength[pageIndex]); + this._prepareMatches(matchesWithLength, this._pageMatches[pageIndex], + this._pageMatchesLength[pageIndex]); } _calculateMatch(pageIndex) { - let pageContent = this._normalize(this.pageContents[pageIndex]); - let query = this._normalize(this.state.query); - let caseSensitive = this.state.caseSensitive; - let phraseSearch = this.state.phraseSearch; - const entireWord = this.state.entireWord; + let pageContent = this._normalize(this._pageContents[pageIndex]); + let query = this._normalize(this._state.query); + let caseSensitive = this._state.caseSensitive; + let phraseSearch = this._state.phraseSearch; + const entireWord = this._state.entireWord; let queryLen = query.length; if (queryLen === 0) { @@ -294,32 +311,32 @@ class PDFFindController { } this._updatePage(pageIndex); - if (this.resumePageIdx === pageIndex) { - this.resumePageIdx = null; + if (this._resumePageIdx === pageIndex) { + this._resumePageIdx = null; this._nextPageMatch(); } // Update the match count. - const pageMatchesCount = this.pageMatches[pageIndex].length; + const pageMatchesCount = this._pageMatches[pageIndex].length; if (pageMatchesCount > 0) { - this.matchesCountTotal += pageMatchesCount; + this._matchesCountTotal += pageMatchesCount; this._updateUIResultsCount(); } } _extractText() { // Perform text extraction once if this method is called multiple times. - if (this.extractTextPromises.length > 0) { + if (this._extractTextPromises.length > 0) { return; } let promise = Promise.resolve(); - for (let i = 0, ii = this.pdfViewer.pagesCount; i < ii; i++) { + for (let i = 0, ii = this._pdfViewer.pagesCount; i < ii; i++) { let extractTextCapability = createPromiseCapability(); - this.extractTextPromises[i] = extractTextCapability.promise; + this._extractTextPromises[i] = extractTextCapability.promise; promise = promise.then(() => { - return this.pdfViewer.getPageTextContent(i).then((textContent) => { + return this._pdfViewer.getPageTextContent(i).then((textContent) => { let textItems = textContent.items; let strBuf = []; @@ -327,12 +344,12 @@ class PDFFindController { strBuf.push(textItems[j].str); } // Store the pageContent as a string. - this.pageContents[i] = strBuf.join(''); + this._pageContents[i] = strBuf.join(''); extractTextCapability.resolve(i); }, (reason) => { console.error(`Unable to get page ${i + 1} text content`, reason); // Page error -- assuming no text content. - this.pageContents[i] = ''; + this._pageContents[i] = ''; extractTextCapability.resolve(i); }); }); @@ -340,46 +357,46 @@ class PDFFindController { } _updatePage(index) { - if (this.selected.pageIdx === index) { + if (this._selected.pageIdx === index) { // If the page is selected, scroll the page into view, which triggers // rendering the page, which adds the textLayer. Once the textLayer is // build, it will scroll onto the selected match. - this.pdfViewer.currentPageNumber = index + 1; + this._pdfViewer.currentPageNumber = index + 1; } - let page = this.pdfViewer.getPageView(index); + let page = this._pdfViewer.getPageView(index); if (page.textLayer) { page.textLayer.updateMatches(); } } _nextMatch() { - let previous = this.state.findPrevious; - let currentPageIndex = this.pdfViewer.currentPageNumber - 1; - let numPages = this.pdfViewer.pagesCount; + let previous = this._state.findPrevious; + let currentPageIndex = this._pdfViewer.currentPageNumber - 1; + let numPages = this._pdfViewer.pagesCount; this.active = true; - if (this.dirtyMatch) { + if (this._dirtyMatch) { // Need to recalculate the matches, reset everything. - this.dirtyMatch = false; - this.selected.pageIdx = this.selected.matchIdx = -1; - this.offset.pageIdx = currentPageIndex; - this.offset.matchIdx = null; - this.resumePageIdx = null; - this.pageMatches = []; - this.matchesCountTotal = 0; - this.pageMatchesLength = null; + this._dirtyMatch = false; + this._selected.pageIdx = this._selected.matchIdx = -1; + this._offset.pageIdx = currentPageIndex; + this._offset.matchIdx = null; + this._resumePageIdx = null; + this._pageMatches = []; + this._matchesCountTotal = 0; + this._pageMatchesLength = null; for (let i = 0; i < numPages; i++) { // Wipe out any previously highlighted matches. this._updatePage(i); // Start finding the matches as soon as the text is extracted. - if (!(i in this.pendingFindMatches)) { - this.pendingFindMatches[i] = true; - this.extractTextPromises[i].then((pageIdx) => { - delete this.pendingFindMatches[pageIdx]; + if (!(i in this._pendingFindMatches)) { + this._pendingFindMatches[i] = true; + this._extractTextPromises[i].then((pageIdx) => { + delete this._pendingFindMatches[pageIdx]; this._calculateMatch(pageIdx); }); } @@ -387,23 +404,23 @@ class PDFFindController { } // If there's no query there's no point in searching. - if (this.state.query === '') { + if (this._state.query === '') { this._updateUIState(FindState.FOUND); return; } // If we're waiting on a page, we return since we can't do anything else. - if (this.resumePageIdx) { + if (this._resumePageIdx) { return; } - let offset = this.offset; + let offset = this._offset; // Keep track of how many pages we should maximally iterate through. - this.pagesToSearch = numPages; + this._pagesToSearch = numPages; // If there's already a `matchIdx` that means we are iterating through a // page's matches. if (offset.matchIdx !== null) { - let numPageMatches = this.pageMatches[offset.pageIdx].length; + let numPageMatches = this._pageMatches[offset.pageIdx].length; if ((!previous && offset.matchIdx + 1 < numPageMatches) || (previous && offset.matchIdx > 0)) { // The simple case; we just have advance the matchIdx to select @@ -422,9 +439,9 @@ class PDFFindController { } _matchesReady(matches) { - let offset = this.offset; + let offset = this._offset; let numMatches = matches.length; - let previous = this.state.findPrevious; + let previous = this._state.findPrevious; if (numMatches) { // There were matches for the page, so initialize `matchIdx`. @@ -436,7 +453,7 @@ class PDFFindController { this._advanceOffsetPage(previous); if (offset.wrapped) { offset.matchIdx = null; - if (this.pagesToSearch < 0) { + if (this._pagesToSearch < 0) { // No point in wrapping again, there were no matches. this._updateMatch(/* found = */ false); // While matches were not found, searching for a page @@ -449,30 +466,30 @@ class PDFFindController { } _nextPageMatch() { - if (this.resumePageIdx !== null) { + if (this._resumePageIdx !== null) { console.error('There can only be one pending page.'); } let matches = null; do { - let pageIdx = this.offset.pageIdx; - matches = this.pageMatches[pageIdx]; + let pageIdx = this._offset.pageIdx; + matches = this._pageMatches[pageIdx]; if (!matches) { // The matches don't exist yet for processing by `_matchesReady`, // so set a resume point for when they do exist. - this.resumePageIdx = pageIdx; + this._resumePageIdx = pageIdx; break; } } while (!this._matchesReady(matches)); } _advanceOffsetPage(previous) { - let offset = this.offset; - let numPages = this.extractTextPromises.length; + let offset = this._offset; + let numPages = this._extractTextPromises.length; offset.pageIdx = (previous ? offset.pageIdx - 1 : offset.pageIdx + 1); offset.matchIdx = null; - this.pagesToSearch--; + this._pagesToSearch--; if (offset.pageIdx >= numPages || offset.pageIdx < 0) { offset.pageIdx = (previous ? numPages - 1 : 0); @@ -482,33 +499,33 @@ class PDFFindController { _updateMatch(found = false) { let state = FindState.NOT_FOUND; - let wrapped = this.offset.wrapped; - this.offset.wrapped = false; + let wrapped = this._offset.wrapped; + this._offset.wrapped = false; if (found) { - let previousPage = this.selected.pageIdx; - this.selected.pageIdx = this.offset.pageIdx; - this.selected.matchIdx = this.offset.matchIdx; + let previousPage = this._selected.pageIdx; + this.selected.pageIdx = this._offset.pageIdx; + this.selected.matchIdx = this._offset.matchIdx; state = (wrapped ? FindState.WRAPPED : FindState.FOUND); // Update the currently selected page to wipe out any selected matches. - if (previousPage !== -1 && previousPage !== this.selected.pageIdx) { + if (previousPage !== -1 && previousPage !== this._selected.pageIdx) { this._updatePage(previousPage); } } - this._updateUIState(state, this.state.findPrevious); - if (this.selected.pageIdx !== -1) { - this._updatePage(this.selected.pageIdx); + this._updateUIState(state, this._state.findPrevious); + if (this._selected.pageIdx !== -1) { + this._updatePage(this._selected.pageIdx); } } _requestMatchesCount() { - const { pageIdx, matchIdx, } = this.selected; - let current = 0, total = this.matchesCountTotal; + const { pageIdx, matchIdx, } = this._selected; + let current = 0, total = this._matchesCountTotal; if (matchIdx !== -1) { for (let i = 0; i < pageIdx; i++) { - current += (this.pageMatches[i] && this.pageMatches[i].length) || 0; + current += (this._pageMatches[i] && this._pageMatches[i].length) || 0; } current += matchIdx + 1; } From ede414554e8b880be105a17d755bdec71281fa9e Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Wed, 12 Sep 2018 15:25:46 +0200 Subject: [PATCH 4/5] Change `let` to `const` where possible in the find controller Doing so clearly indicates which variables are read-only and may not be mutated, which helps readability and prevents subtle issues. --- web/pdf_find_controller.js | 79 +++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 0689ab202..030a1a484 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -57,8 +57,8 @@ class PDFFindController { this.reset(); // Compile the regular expression for text normalization once. - let replace = Object.keys(CHARACTERS_TO_NORMALIZE).join(''); - this._normalizationRegex = new RegExp('[' + replace + ']', 'g'); + const replace = Object.keys(CHARACTERS_TO_NORMALIZE).join(''); + this._normalizationRegex = new RegExp(`[${replace}]`, 'g'); } get pageMatches() { @@ -141,7 +141,7 @@ class PDFFindController { updateMatchPosition(pageIndex, matchIndex, elements, beginIdx) { if (this.selected.matchIdx === matchIndex && this.selected.pageIdx === pageIndex) { - let spot = { + const spot = { top: FIND_SCROLL_OFFSET_TOP, left: FIND_SCROLL_OFFSET_LEFT, }; @@ -151,7 +151,7 @@ class PDFFindController { } _normalize(text) { - return text.replace(this._normalizationRegex, function (ch) { + return text.replace(this._normalizationRegex, function(ch) { return CHARACTERS_TO_NORMALIZE[ch]; }); } @@ -164,8 +164,8 @@ class PDFFindController { */ _prepareMatches(matchesWithLength, matches, matchesLength) { function isSubTerm(matchesWithLength, currentIndex) { - let currentElem = matchesWithLength[currentIndex]; - let nextElem = matchesWithLength[currentIndex + 1]; + const currentElem = matchesWithLength[currentIndex]; + const nextElem = matchesWithLength[currentIndex + 1]; // Check for cases like "TAMEd TAME". if (currentIndex < matchesWithLength.length - 1 && @@ -176,7 +176,7 @@ class PDFFindController { // Check for cases like "thIS IS". for (let i = currentIndex - 1; i >= 0; i--) { - let prevElem = matchesWithLength[i]; + const prevElem = matchesWithLength[i]; if (prevElem.skipped) { continue; } @@ -231,8 +231,9 @@ class PDFFindController { } _calculatePhraseMatch(query, pageIndex, pageContent, entireWord) { - let matches = []; - let queryLen = query.length; + const matches = []; + const queryLen = query.length; + let matchIdx = -queryLen; while (true) { matchIdx = pageContent.indexOf(query, matchIdx + queryLen); @@ -248,12 +249,14 @@ class PDFFindController { } _calculateWordMatch(query, pageIndex, pageContent, entireWord) { - let matchesWithLength = []; + const matchesWithLength = []; + // Divide the query into pieces and search for text in each piece. - let queryArray = query.match(/\S+/g); + const queryArray = query.match(/\S+/g); for (let i = 0, len = queryArray.length; i < len; i++) { - let subquery = queryArray[i]; - let subqueryLen = subquery.length; + const subquery = queryArray[i]; + const subqueryLen = subquery.length; + let matchIdx = -subqueryLen; while (true) { matchIdx = pageContent.indexOf(subquery, matchIdx + subqueryLen); @@ -289,12 +292,9 @@ class PDFFindController { _calculateMatch(pageIndex) { let pageContent = this._normalize(this._pageContents[pageIndex]); let query = this._normalize(this._state.query); - let caseSensitive = this._state.caseSensitive; - let phraseSearch = this._state.phraseSearch; - const entireWord = this._state.entireWord; - let queryLen = query.length; + const { caseSensitive, entireWord, phraseSearch, } = this._state; - if (queryLen === 0) { + if (query.length === 0) { // Do nothing: the matches should be wiped out already. return; } @@ -332,22 +332,23 @@ class PDFFindController { let promise = Promise.resolve(); for (let i = 0, ii = this._pdfViewer.pagesCount; i < ii; i++) { - let extractTextCapability = createPromiseCapability(); + const extractTextCapability = createPromiseCapability(); this._extractTextPromises[i] = extractTextCapability.promise; promise = promise.then(() => { return this._pdfViewer.getPageTextContent(i).then((textContent) => { - let textItems = textContent.items; - let strBuf = []; + const textItems = textContent.items; + const strBuf = []; for (let j = 0, jj = textItems.length; j < jj; j++) { strBuf.push(textItems[j].str); } - // Store the pageContent as a string. + + // Store the page content (text items) as one string. this._pageContents[i] = strBuf.join(''); extractTextCapability.resolve(i); }, (reason) => { - console.error(`Unable to get page ${i + 1} text content`, reason); + console.error(`Unable to get text content for page ${i + 1}`, reason); // Page error -- assuming no text content. this._pageContents[i] = ''; extractTextCapability.resolve(i); @@ -364,16 +365,16 @@ class PDFFindController { this._pdfViewer.currentPageNumber = index + 1; } - let page = this._pdfViewer.getPageView(index); + const page = this._pdfViewer.getPageView(index); if (page.textLayer) { page.textLayer.updateMatches(); } } _nextMatch() { - let previous = this._state.findPrevious; - let currentPageIndex = this._pdfViewer.currentPageNumber - 1; - let numPages = this._pdfViewer.pagesCount; + const previous = this._state.findPrevious; + const currentPageIndex = this._pdfViewer.currentPageNumber - 1; + const numPages = this._pdfViewer.pagesCount; this.active = true; @@ -384,9 +385,9 @@ class PDFFindController { this._offset.pageIdx = currentPageIndex; this._offset.matchIdx = null; this._resumePageIdx = null; - this._pageMatches = []; - this._matchesCountTotal = 0; + this._pageMatches.length = 0; this._pageMatchesLength = null; + this._matchesCountTotal = 0; for (let i = 0; i < numPages; i++) { // Wipe out any previously highlighted matches. @@ -414,13 +415,13 @@ class PDFFindController { return; } - let offset = this._offset; + const offset = this._offset; // Keep track of how many pages we should maximally iterate through. this._pagesToSearch = numPages; // If there's already a `matchIdx` that means we are iterating through a // page's matches. if (offset.matchIdx !== null) { - let numPageMatches = this._pageMatches[offset.pageIdx].length; + const numPageMatches = this._pageMatches[offset.pageIdx].length; if ((!previous && offset.matchIdx + 1 < numPageMatches) || (previous && offset.matchIdx > 0)) { // The simple case; we just have advance the matchIdx to select @@ -439,9 +440,9 @@ class PDFFindController { } _matchesReady(matches) { - let offset = this._offset; - let numMatches = matches.length; - let previous = this._state.findPrevious; + const offset = this._offset; + const numMatches = matches.length; + const previous = this._state.findPrevious; if (numMatches) { // There were matches for the page, so initialize `matchIdx`. @@ -472,7 +473,7 @@ class PDFFindController { let matches = null; do { - let pageIdx = this._offset.pageIdx; + const pageIdx = this._offset.pageIdx; matches = this._pageMatches[pageIdx]; if (!matches) { // The matches don't exist yet for processing by `_matchesReady`, @@ -484,8 +485,8 @@ class PDFFindController { } _advanceOffsetPage(previous) { - let offset = this._offset; - let numPages = this._extractTextPromises.length; + const offset = this._offset; + const numPages = this._extractTextPromises.length; offset.pageIdx = (previous ? offset.pageIdx - 1 : offset.pageIdx + 1); offset.matchIdx = null; @@ -499,11 +500,11 @@ class PDFFindController { _updateMatch(found = false) { let state = FindState.NOT_FOUND; - let wrapped = this._offset.wrapped; + const wrapped = this._offset.wrapped; this._offset.wrapped = false; if (found) { - let previousPage = this._selected.pageIdx; + const previousPage = this._selected.pageIdx; this.selected.pageIdx = this._offset.pageIdx; this.selected.matchIdx = this._offset.matchIdx; state = (wrapped ? FindState.WRAPPED : FindState.FOUND); From 67e1e39f99fc78e4c423a4d3684275f70389c6a8 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Wed, 12 Sep 2018 15:42:29 +0200 Subject: [PATCH 5/5] Move scrolling the selected match into view from the find controller to the text layer builder The find controller should only coordinate finding a string in the document and should not be responsible for presenting the matches to the user. The text layer builder already contains the logic to render the matches in the viewer, so it should also take care of scrolling the selected match into view. --- web/pdf_find_controller.js | 23 ----------------------- web/text_layer_builder.js | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 25 deletions(-) diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 030a1a484..815a3a93c 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -16,7 +16,6 @@ import { createPromiseCapability } from 'pdfjs-lib'; import { getCharacterType } from './pdf_find_utils'; import { getGlobalEventBus } from './dom_events'; -import { scrollIntoView } from './ui_utils'; const FindState = { FOUND: 0, @@ -25,8 +24,6 @@ const FindState = { PENDING: 3, }; -const FIND_SCROLL_OFFSET_TOP = -50; -const FIND_SCROLL_OFFSET_LEFT = -400; const FIND_TIMEOUT = 250; // ms const CHARACTERS_TO_NORMALIZE = { @@ -130,26 +127,6 @@ class PDFFindController { }); } - /** - * Called from the text layer when match presentation is updated. - * - * @param {number} pageIndex - The index of the page. - * @param {number} matchIndex - The index of the match. - * @param {Array} elements - Text layer `div` elements. - * @param {number} beginIdx - Start index of the `div` array for the match. - */ - updateMatchPosition(pageIndex, matchIndex, elements, beginIdx) { - if (this.selected.matchIdx === matchIndex && - this.selected.pageIdx === pageIndex) { - const spot = { - top: FIND_SCROLL_OFFSET_TOP, - left: FIND_SCROLL_OFFSET_LEFT, - }; - scrollIntoView(elements[beginIdx], spot, - /* skipOverflowHiddenElements = */ true); - } - } - _normalize(text) { return text.replace(this._normalizationRegex, function(ch) { return CHARACTERS_TO_NORMALIZE[ch]; diff --git a/web/text_layer_builder.js b/web/text_layer_builder.js index 5b4bf6ff8..da7833291 100644 --- a/web/text_layer_builder.js +++ b/web/text_layer_builder.js @@ -15,8 +15,11 @@ import { getGlobalEventBus } from './dom_events'; import { renderTextLayer } from 'pdfjs-lib'; +import { scrollIntoView } from './ui_utils'; const EXPAND_DIVS_TIMEOUT = 300; // ms +const MATCH_SCROLL_OFFSET_TOP = -50; +const MATCH_SCROLL_OFFSET_LEFT = -400; /** * @typedef {Object} TextLayerBuilderOptions @@ -243,9 +246,17 @@ class TextLayerBuilder { let isSelected = (isSelectedPage && i === selectedMatchIdx); let highlightSuffix = (isSelected ? ' selected' : ''); + // Scroll the selected match into view. if (this.findController) { - this.findController.updateMatchPosition(pageIdx, i, textDivs, - begin.divIdx); + if (this.findController.selected.matchIdx === i && + this.findController.selected.pageIdx === pageIdx) { + const spot = { + top: MATCH_SCROLL_OFFSET_TOP, + left: MATCH_SCROLL_OFFSET_LEFT, + }; + scrollIntoView(textDivs[begin.divIdx], spot, + /* skipOverflowHiddenElements = */ true); + } } // Match inside new div.