Implement a PDFSinglePageViewer class (issue 8188)
#8724
Conversation
|
I think this would be a really good approach! Duplicating and having to maintain a verbatim copy of the code does not sound like the way to go, whereas this is much easier to manage. |
|
Cool, let try it this way. May I suggest splitting into several files as well: rename to BaseViewer, then move the code into separate pdf_viewer.js and pdf_single_page_viewer.js files. |
web/pdf_viewer.js
Outdated
| scrollIntoView(pageDiv, pageSpot); | ||
|
|
||
| // Always update the scroll direction, since it most likely isn't correct. | ||
| this.scroll.down = scrolledDown; |
There was a problem hiding this comment.
This is really an ugly hack that doesn't even work in all cases, so a more robust solution for tracking scroll direction seems necessary (taking into account first of all the page-switching direction and also the user scroll direction somehow, I'll look into it later in the week).
There was a problem hiding this comment.
Since the code currently relies on this.scroll when determining which page should be pre-rendered, and I didn't feel like refactoring all of that here too, this patch is still updating this.scroll.down according to the page switching direction.
However it's now done in such a way that it at least should be consistent, since it waits for the next scroll event; and unless someone complains violently I'd lean towards keeping this for now.
|
/botio-linux preview |
|
#8188 (comment) mentions the possibility of using the What's not clear to me is exactly how that would work though, since just replacing a If we want to try and address the above in the scope of this PR, then I'd appreciate ideas for how that can be achieved! (If we come up with a good approach it might also be helpful when addressing issue #2638.) |
|
@yurydelendik Do you have any feedback on the questions/ideas in #8724 (comment), or should we just land this as-is and iterate on that in follow-ups instead? |
yes. If there is no breaking api changes, let's do just that. |
Since this patch only touches code in the Since I'm not sure if anyone has really reviewed this, besides quickly skimming though the diff, I'm not going to land this myself. |
Please note that the only reason for this change is to try and improve reviewability of later patches, by keeping the diffs more manageable.
This patch introduces an abstract `BaseViewer` class, that the existing `PDFViewer` then extends. *Please note:* This lays the necessary foundation for the next patch.
The new `PDFSinglePageViewer` class extends the previously created abstract `BaseViewer` class. There's *a lot* of existing functionality in `PDFViewer` that depends on all the pages being loaded and synchronously available, once the `setDocument` method has been called. Given that initializing `PDFPageView` instances requires passing a DOM element to which the page is attached, the simplest solution I could come up with is to append all pages to a (hidden) document fragment and just swap them (one at a time) into the viewer when page switching occurs.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/d407cc8dc50d6bb/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/d407cc8dc50d6bb/output.txt Total script time: 2.40 mins Published |
|
Nice work! I'll file follow-up issues for the other ideas. |
Implement a `PDFSinglePageViewer` class (issue 8188)
This PR contains a suggested approach for addressing only #8188 (comment), but doesn't include a
componentsexample.[1]For testing purposes, the second temporary commit uses the new
PDFSinglePageViewerclass in the default viewer (all normal viewer functionality appears to work in my quick tests).[1] Please note: I initially intended to try and provide feedback on PR #8716, but the new example provided there didn't even run. Fixing that locally, I couldn't help but noticing that the new class there looks and also behaves just like a verbatim copy of the existing
web/pdf_viewer.jsfile; which I cannot imagine is what we want, since then why not simply usePDFViewerdirectly?It thus seemed to me that the suggested solution in #8716 was too far from workable that it'd be difficult to provide meaningful review comments; and I figured that I'd see how difficult it would be to implement this.