-
Notifications
You must be signed in to change notification settings - Fork 10.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update findbar to wrap on initial search #5325
Conversation
As a precursor to the review, please squash the commits into one, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits. |
31b4fb0
to
3a2bef9
Compare
Thanks, Jonas -- sorry, missed that step. Commit is compacted now, tested, and ready for review! |
Anything else I need to do in order to get this pull request reviewed? Thanks :) |
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @brendandahl received. Current queue size: 0 Live output at: http://107.21.233.14:8877/4fc258b110a2a91/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/4fc258b110a2a91/output.txt Total script time: 0.76 mins Published
|
Seems to fix the issue you mention, but introduces a new issue. If I start out on page 1 of tracemonkey and search for 'references' it finds it but then if I search for the next match it says phrase not found. |
a18f552
to
e97a01c
Compare
Instead of keeping track of where the search starts, just keep track of the number of pages and make sure we don't visit pages more than once.
f2cb6cb
to
4275481
Compare
Nice catch, Brendan. I changed the approach a bit (to just keep track of the number of pages that were visited during a search to never visit a page more than once) and that fixes your issue as well as the original. |
@@ -255,6 +256,8 @@ var PDFFindController = (function PDFFindControllerClosure() { | |||
} | |||
|
|||
var offset = this.offset; | |||
// Keep track of how many pages we should maximally iterate through. | |||
this.pagesToSearch = this.pdfViewer.pagesCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you do this.pagesToSearch = numPages;
here instead?
Updated -- not re-calculating |
Update findbar to wrap on initial search
Thanks for the fix! |
Fixes #5324
Updated pdf_find_controller to keep track of the original page index
on which the search begins. This allows us to wrap a search without looping
infinitely through the document: if we've wrapped and advanced beyond
the original page index, we know there are no more results to be found.