Make PDFFindController less confusing to use, by allowing searching to start when setDocument is called
#10128
Conversation
Snuffleupagus
force-pushed
the
find-controller-enable
branch
from
38bd82b to
c1c22bc
Compare
| resolve(); | ||
| }); | ||
| }); | ||
| this._firstPageCapability = createPromiseCapability(); |
There was a problem hiding this comment.
I'm wondering if _firstPageCapability is a good name now that we're not actually waiting until the pagesinit event is emitted. Maybe _initializationCapability or something similar would be better; what do you think?
There was a problem hiding this comment.
I'm wondering if
_firstPageCapabilityis a good name now that we're not actually waiting until thepagesinitevent is emitted.
The answer is probably both yes and no at this point, depending on your perspective; naming things is hard :-)
The name is still very much relevant for the default use-case, i.e. the viewer/components, since PDFFindController.setDocument is still called after the first page has been resolved in BaseViewer.setDocument.
However, for the general case where PDFFindController could now be used standalone from the viewer, the name is obviously somewhat strange.
Given that it's tracking internal state anyway, I don't know if the name matter too much though and keeping the current one is also more inline with the default viewer use-case.
Snuffleupagus
force-pushed
the
find-controller-enable
branch
from
c1c22bc to
75f2808
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/d21767452aaf05b/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/d21767452aaf05b/output.txt Total script time: 2.99 mins Published |
… 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.
Snuffleupagus
force-pushed
the
find-controller-enable
branch
from
75f2808 to
2ed3591
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/c2c35a5da1b7f8f/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/c2c35a5da1b7f8f/output.txt Total script time: 3.08 mins Published |
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/ae8d1b92b05498d/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/3636d16816fc85f/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/ae8d1b92b05498d/output.txt Total script time: 3.94 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/3636d16816fc85f/output.txt Total script time: 5.22 mins
|
|
Good improvement! |
This patch is based on something that I noticed while working on PR #10126.
The recent re-factoring of
PDFFindControllerbrought many improvements, among those the fact that access toBaseVieweris 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.setDocumentmust be called and a 'pagesinit' event must be dispatched (from somewhere).For all other viewer components, calling the
setDocumentmethod[2] is enough in order for the component to actually be usable.The
PDFFindControllerthus 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 usePDFFindControllerseparately 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
setDocumentshould be used to indicate that searching may start. For the default viewer and/or the viewer components,BaseViewer.setDocumentwill now callPDFFindController.setDocumentwhen 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 callingPDFFindController.setDocumentto 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
PDFFindControllerinstance when creating aBaseViewerinstance, of course.