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.
This commit is contained in:
parent
8411a7da28
commit
2ed3591b22
@ -70,5 +70,4 @@ pdfjsLib.getDocument({
|
|||||||
pdfViewer.setDocument(pdfDocument);
|
pdfViewer.setDocument(pdfDocument);
|
||||||
|
|
||||||
pdfLinkService.setDocument(pdfDocument, null);
|
pdfLinkService.setDocument(pdfDocument, null);
|
||||||
pdfFindController.setDocument(pdfDocument);
|
|
||||||
});
|
});
|
||||||
|
@ -70,5 +70,4 @@ pdfjsLib.getDocument({
|
|||||||
pdfSinglePageViewer.setDocument(pdfDocument);
|
pdfSinglePageViewer.setDocument(pdfDocument);
|
||||||
|
|
||||||
pdfLinkService.setDocument(pdfDocument, null);
|
pdfLinkService.setDocument(pdfDocument, null);
|
||||||
pdfFindController.setDocument(pdfDocument);
|
|
||||||
});
|
});
|
||||||
|
@ -51,18 +51,17 @@ describe('pdf_find_controller', function() {
|
|||||||
beforeEach(function(done) {
|
beforeEach(function(done) {
|
||||||
const loadingTask = getDocument(buildGetDocumentParams('tracemonkey.pdf'));
|
const loadingTask = getDocument(buildGetDocumentParams('tracemonkey.pdf'));
|
||||||
loadingTask.promise.then(function(pdfDocument) {
|
loadingTask.promise.then(function(pdfDocument) {
|
||||||
|
eventBus = new EventBus();
|
||||||
|
|
||||||
const linkService = new MockLinkService();
|
const linkService = new MockLinkService();
|
||||||
linkService.setDocument(pdfDocument);
|
linkService.setDocument(pdfDocument);
|
||||||
|
|
||||||
eventBus = new EventBus();
|
|
||||||
|
|
||||||
pdfFindController = new PDFFindController({
|
pdfFindController = new PDFFindController({
|
||||||
linkService,
|
linkService,
|
||||||
eventBus,
|
eventBus,
|
||||||
});
|
});
|
||||||
pdfFindController.setDocument(pdfDocument);
|
pdfFindController.setDocument(pdfDocument); // Enable searching.
|
||||||
|
|
||||||
eventBus.dispatch('pagesinit');
|
|
||||||
done();
|
done();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
@ -569,7 +569,6 @@ let PDFViewerApplication = {
|
|||||||
if (this.pdfDocument) {
|
if (this.pdfDocument) {
|
||||||
this.pdfDocument = null;
|
this.pdfDocument = null;
|
||||||
|
|
||||||
this.findController.setDocument(null);
|
|
||||||
this.pdfThumbnailViewer.setDocument(null);
|
this.pdfThumbnailViewer.setDocument(null);
|
||||||
this.pdfViewer.setDocument(null);
|
this.pdfViewer.setDocument(null);
|
||||||
this.pdfLinkService.setDocument(null);
|
this.pdfLinkService.setDocument(null);
|
||||||
@ -893,7 +892,6 @@ let PDFViewerApplication = {
|
|||||||
} else if (PDFJSDev.test('CHROME')) {
|
} else if (PDFJSDev.test('CHROME')) {
|
||||||
baseDocumentUrl = location.href.split('#')[0];
|
baseDocumentUrl = location.href.split('#')[0];
|
||||||
}
|
}
|
||||||
this.findController.setDocument(pdfDocument);
|
|
||||||
this.pdfLinkService.setDocument(pdfDocument, baseDocumentUrl);
|
this.pdfLinkService.setDocument(pdfDocument, baseDocumentUrl);
|
||||||
this.pdfDocumentProperties.setDocument(pdfDocument, this.url);
|
this.pdfDocumentProperties.setDocument(pdfDocument, this.url);
|
||||||
|
|
||||||
|
@ -363,6 +363,10 @@ class BaseViewer {
|
|||||||
if (this.pdfDocument) {
|
if (this.pdfDocument) {
|
||||||
this._cancelRendering();
|
this._cancelRendering();
|
||||||
this._resetView();
|
this._resetView();
|
||||||
|
|
||||||
|
if (this.findController) {
|
||||||
|
this.findController.setDocument(null);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
this.pdfDocument = pdfDocument;
|
this.pdfDocument = pdfDocument;
|
||||||
@ -471,6 +475,9 @@ class BaseViewer {
|
|||||||
|
|
||||||
this.eventBus.dispatch('pagesinit', { source: this, });
|
this.eventBus.dispatch('pagesinit', { source: this, });
|
||||||
|
|
||||||
|
if (this.findController) {
|
||||||
|
this.findController.setDocument(pdfDocument); // Enable searching.
|
||||||
|
}
|
||||||
if (this.defaultRenderingQueue) {
|
if (this.defaultRenderingQueue) {
|
||||||
this.update();
|
this.update();
|
||||||
}
|
}
|
||||||
|
@ -99,6 +99,7 @@ class PDFFindController {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
this._pdfDocument = pdfDocument;
|
this._pdfDocument = pdfDocument;
|
||||||
|
this._firstPageCapability.resolve();
|
||||||
}
|
}
|
||||||
|
|
||||||
executeCommand(cmd, state) {
|
executeCommand(cmd, state) {
|
||||||
@ -110,7 +111,7 @@ class PDFFindController {
|
|||||||
this._state = state;
|
this._state = state;
|
||||||
this._updateUIState(FindState.PENDING);
|
this._updateUIState(FindState.PENDING);
|
||||||
|
|
||||||
this._firstPagePromise.then(() => {
|
this._firstPageCapability.promise.then(() => {
|
||||||
if (!this._pdfDocument ||
|
if (!this._pdfDocument ||
|
||||||
(pdfDocument && this._pdfDocument !== pdfDocument)) {
|
(pdfDocument && this._pdfDocument !== pdfDocument)) {
|
||||||
// If the document was closed before searching began, or if the search
|
// If the document was closed before searching began, or if the search
|
||||||
@ -160,17 +161,7 @@ class PDFFindController {
|
|||||||
clearTimeout(this._findTimeout);
|
clearTimeout(this._findTimeout);
|
||||||
this._findTimeout = null;
|
this._findTimeout = null;
|
||||||
|
|
||||||
this._firstPagePromise = new Promise((resolve) => {
|
this._firstPageCapability = createPromiseCapability();
|
||||||
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);
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
_normalize(text) {
|
_normalize(text) {
|
||||||
@ -553,7 +544,7 @@ class PDFFindController {
|
|||||||
// Since searching is asynchronous, ensure that the removal of highlighted
|
// Since searching is asynchronous, ensure that the removal of highlighted
|
||||||
// matches (from the UI) is async too such that the 'updatetextlayermatches'
|
// matches (from the UI) is async too such that the 'updatetextlayermatches'
|
||||||
// events will always be dispatched in the expected order.
|
// events will always be dispatched in the expected order.
|
||||||
this._firstPagePromise.then(() => {
|
this._firstPageCapability.promise.then(() => {
|
||||||
if (!this._pdfDocument ||
|
if (!this._pdfDocument ||
|
||||||
(pdfDocument && this._pdfDocument !== pdfDocument)) {
|
(pdfDocument && this._pdfDocument !== pdfDocument)) {
|
||||||
// Only update the UI if the document is open, and is the current one.
|
// Only update the UI if the document is open, and is the current one.
|
||||||
|
Loading…
Reference in New Issue
Block a user