Add previous/next-page functionality that takes scroll/spread-modes into account (issue 11946) #12870
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Snuffleupagus
force-pushed
the
page-advance
branch
9 times, most recently
from
3f27189 to
1190193
Compare
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/ba2de5820cad997/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/44c8e1db573662c/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/ba2de5820cad997/output.txt Total script time: 3.60 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/44c8e1db573662c/output.txt Total script time: 4.66 mins
|
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3b9c59e060d8335/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/3b9c59e060d8335/output.txt Total script time: 4.18 mins Published |
Snuffleupagus
force-pushed
the
page-advance
branch
2 times, most recently
from
c12d070 to
07e90aa
Compare
…nto account (issue 11946) - For wrapped scrolling, we unfortunately need to do a fair bit of parsing of the *current* page layout. Compared to e.g. the spread-modes, where we can easily tell how the pages are laid out, with wrapped scrolling we cannot tell without actually checking. In particular documents with varying page sizes require some care, since we need to check all pages on the "row" of the current page are visible and that there aren't any "holes" present. Otherwise, in the general case, there's a risk that we'd skip over pages if we'd simply always advance to the previous/next "row" in wrapped scrolling. - For horizontal scrolling, this patch simply maintains the current behaviour of advancing *one* page at a time. The reason for this is to prevent inconsistent behaviour for the next and previous cases, since those cannot be handled identically. For the next-case, it'd obviously be simple to advance to the first not completely visible page. However for the previous-case, we'd only be able to go back *one* page since it's not possible to (easily) determine the page layout of non-visible pages (documents with varying page sizes being a particular issue). - For vertical scrolling, this patch maintains the current behaviour by default. When spread-modes are being used, we'll now attempt to advance to the next *spread*, rather than just the next page, whenever possible. To prevent skipping over a page, this two-page advance will only apply when both pages of the current spread are visible (to avoid breaking documents with varying page sizes) and when the second page in the current spread is fully visible *horizontally* (to handle larger zoom values). In order to reduce the performance impact of these changes, note that the previous/next-functionality will only call `getVisibleElements` for the scroll/spread-modes where that's necessary and that "normal" vertical scrolling is thus unaffected by these changes. To support these changes, the `getVisibleElements` helper function will now also include the `widthPercent` in addition to the existing `percent` property. The `PDFViewer._updateHelper` method is changed slightly w.r.t. updating the `currentPageNumber` for the non-vertical/spread modes, i.e. won't affect "normal" vertical scrolling, since that helped simplify the overall calculation of the page advance. Finally, these new `BaseViewer` methods also allow (some) simplification of previous/next-page functionality in various viewer components. *Please note:* There's one thing that this patch does not attempt to change, namely disabling of the previous/next toolbarButtons respectively the firstPage/lastPage secondaryToolbarButtons. The reason for this is that doing so would add quite a bit of complexity in general, and if for some reason `BaseViewer._getPageAdvance` would get things wrong we could end up incorrectly disabling the buttons. Hence it seemed overall safer to *not* touch this, and accept that the buttons won't be `disabled` despite in some edge-cases no further scrolling being possible.
Snuffleupagus
force-pushed
the
page-advance
branch
from
07e90aa to
a2b592f
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/b714e4ea9af8c06/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b714e4ea9af8c06/output.txt Total script time: 4.28 mins Published |
timvandermeij
approved these changes
Jan 23, 2021
|
Thank you! I had a good look at this isn't entirely trivial, but I couldn't spot any logic errors and in my manual testing this also seems to work as expected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For wrapped scrolling, we unfortunately need to do a fair bit of parsing of the current page layout. Compared to e.g. the spread-modes, where we can easily tell how the pages are laid out, with wrapped scrolling we cannot tell without actually checking. In particular documents with varying page sizes require some care, since we need to check all pages on the "row" of the current page are visible and that there aren't any "holes" present. Otherwise, in the general case, there's a risk that we'd skip over pages if we'd simply always advance to the previous/next "row" in wrapped scrolling.
For horizontal scrolling, this patch simply maintains the current behaviour of advancing one page at a time. The reason for this is to prevent inconsistent behaviour for the next and previous cases, since those cannot be handled identically. For the next-case, it'd obviously be simple to advance to the first not completely visible page. However for the previous-case, we'd only be able to go back one page since it's not possible to (easily) determine the page layout of non-visible pages (documents with varying page sizes being a particular issue).
For vertical scrolling, this patch maintains the current behaviour by default. When spread-modes are being used, we'll now attempt to advance to the next spread, rather than just the next page, whenever possible. To prevent skipping over a page, this two-page advance will only apply when both pages of the current spread are visible (to avoid breaking documents with varying page sizes) and when the second page in the current spread is fully visible horizontally (to handle larger zoom values).
In order to reduce the performance impact of these changes, note that the previous/next-functionality will only call
getVisibleElementsfor the scroll/spread-modes where that's necessary and that "normal" vertical scrolling is thus unaffected by these changes.To support these changes, the
getVisibleElementshelper function will now also include thewidthPercentin addition to the existingpercentproperty.The
PDFViewer._updateHelpermethod is changed slightly w.r.t. updating thecurrentPageNumberfor the non-vertical/spread modes, i.e. won't affect "normal" vertical scrolling, since that helped simplify the overall calculation of the page advance.Finally, these new
BaseViewermethods also allow (some) simplification of previous/next-page functionality in various viewer components.Please note: There's one thing that this patch does not attempt to change, namely disabling of the previous/next toolbarButtons respectively the firstPage/lastPage secondaryToolbarButtons. The reason for this is that doing so would add quite a bit of complexity in general, and if for some reason
BaseViewer._getPageAdvancewould get things wrong we could end up incorrectly disabling the buttons. Hence it seemed overall safer to not touch this, and accept that the buttons won't bedisableddespite in some edge-cases no further scrolling being possible.Fixes #11946