From 2ed3591b2240466e194a5bec494800d511460e9d Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 3 Oct 2018 12:42:41 +0200 Subject: [PATCH] Make `PDFFindController` less confusing to use, by allowing searching to start when `setDocument` is called *This patch is based on something that I noticed while working on PR 10126.* The recent re-factoring of `PDFFindController` brought many improvements, among those the fact that access to `BaseViewer` is no longer required. However, with these changes there's one thing which now strikes me as not particularly user-friendly[1]: The fact that in order for searching to actually work, `PDFFindController.setDocument` must be called *and* a 'pagesinit' event must be dispatched (from somewhere). For all other viewer components, calling the `setDocument` method[2] is enough in order for the component to actually be usable. The `PDFFindController` thus stands out quite a bit, and it also becomes difficult to work with in any sort of custom implementation. For example: Imagine someone trying to use `PDFFindController` separately from the viewer[3], which *should* now be relatively simple given the re-factoring, and thus having to (somehow) figure out that they'll also need to manually dispatch a 'pagesinit' event for searching to work. Note that the above even affects the unit-tests, where an out-of-place 'pagesinit' event is being used. To attempt to address these problems, I'm thus suggesting that *only* `setDocument` should be used to indicate that searching may start. For the default viewer and/or the viewer components, `BaseViewer.setDocument` will now call `PDFFindController.setDocument` when the document is ready, thus requiring no outside configuration anymore[4]. For custom implementation, and the unit-tests, it's now as simple as just calling `PDFFindController.setDocument` to allow searching to start. --- [1] I should have caught this during review of PR 10099, but unfortunately it's sometimes not until you actually work with the code in question that things like these become clear. [2] Assuming, obviously, that the viewer component in question actually implements such a method :-) [3] There's even a very recent issue, filed by someone trying to do just that. [4] Short of providing a `PDFFindController` instance when creating a `BaseViewer` instance, of course. --- examples/components/simpleviewer.js | 1 - examples/components/singlepageviewer.js | 1 - test/unit/pdf_find_controller_spec.js | 7 +++---- web/app.js | 2 -- web/base_viewer.js | 7 +++++++ web/pdf_find_controller.js | 17 ++++------------- 6 files changed, 14 insertions(+), 21 deletions(-) diff --git a/examples/components/simpleviewer.js b/examples/components/simpleviewer.js index 090acc3bc..20a0d66ab 100644 --- a/examples/components/simpleviewer.js +++ b/examples/components/simpleviewer.js @@ -70,5 +70,4 @@ pdfjsLib.getDocument({ pdfViewer.setDocument(pdfDocument); pdfLinkService.setDocument(pdfDocument, null); - pdfFindController.setDocument(pdfDocument); }); diff --git a/examples/components/singlepageviewer.js b/examples/components/singlepageviewer.js index 67c7a416e..b3b7ab268 100644 --- a/examples/components/singlepageviewer.js +++ b/examples/components/singlepageviewer.js @@ -70,5 +70,4 @@ pdfjsLib.getDocument({ pdfSinglePageViewer.setDocument(pdfDocument); pdfLinkService.setDocument(pdfDocument, null); - pdfFindController.setDocument(pdfDocument); }); diff --git a/test/unit/pdf_find_controller_spec.js b/test/unit/pdf_find_controller_spec.js index 269e6c65f..9fe9803a0 100644 --- a/test/unit/pdf_find_controller_spec.js +++ b/test/unit/pdf_find_controller_spec.js @@ -51,18 +51,17 @@ describe('pdf_find_controller', function() { beforeEach(function(done) { const loadingTask = getDocument(buildGetDocumentParams('tracemonkey.pdf')); loadingTask.promise.then(function(pdfDocument) { + eventBus = new EventBus(); + const linkService = new MockLinkService(); linkService.setDocument(pdfDocument); - eventBus = new EventBus(); - pdfFindController = new PDFFindController({ linkService, eventBus, }); - pdfFindController.setDocument(pdfDocument); + pdfFindController.setDocument(pdfDocument); // Enable searching. - eventBus.dispatch('pagesinit'); done(); }); }); diff --git a/web/app.js b/web/app.js index 822cf4fbd..9264ca114 100644 --- a/web/app.js +++ b/web/app.js @@ -569,7 +569,6 @@ let PDFViewerApplication = { if (this.pdfDocument) { this.pdfDocument = null; - this.findController.setDocument(null); this.pdfThumbnailViewer.setDocument(null); this.pdfViewer.setDocument(null); this.pdfLinkService.setDocument(null); @@ -893,7 +892,6 @@ let PDFViewerApplication = { } else if (PDFJSDev.test('CHROME')) { baseDocumentUrl = location.href.split('#')[0]; } - this.findController.setDocument(pdfDocument); this.pdfLinkService.setDocument(pdfDocument, baseDocumentUrl); this.pdfDocumentProperties.setDocument(pdfDocument, this.url); diff --git a/web/base_viewer.js b/web/base_viewer.js index 57153057f..bf8f091a8 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -363,6 +363,10 @@ class BaseViewer { if (this.pdfDocument) { this._cancelRendering(); this._resetView(); + + if (this.findController) { + this.findController.setDocument(null); + } } this.pdfDocument = pdfDocument; @@ -471,6 +475,9 @@ class BaseViewer { this.eventBus.dispatch('pagesinit', { source: this, }); + if (this.findController) { + this.findController.setDocument(pdfDocument); // Enable searching. + } if (this.defaultRenderingQueue) { this.update(); } diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 93b5d31d0..5c19b4435 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -99,6 +99,7 @@ class PDFFindController { return; } this._pdfDocument = pdfDocument; + this._firstPageCapability.resolve(); } executeCommand(cmd, state) { @@ -110,7 +111,7 @@ class PDFFindController { this._state = state; this._updateUIState(FindState.PENDING); - this._firstPagePromise.then(() => { + this._firstPageCapability.promise.then(() => { if (!this._pdfDocument || (pdfDocument && this._pdfDocument !== pdfDocument)) { // If the document was closed before searching began, or if the search @@ -160,17 +161,7 @@ class PDFFindController { clearTimeout(this._findTimeout); this._findTimeout = null; - this._firstPagePromise = new Promise((resolve) => { - 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); - }); + this._firstPageCapability = createPromiseCapability(); } _normalize(text) { @@ -553,7 +544,7 @@ class PDFFindController { // 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(() => { + this._firstPageCapability.promise.then(() => { if (!this._pdfDocument || (pdfDocument && this._pdfDocument !== pdfDocument)) { // Only update the UI if the document is open, and is the current one.