Skip to content
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

Merged
merged 2 commits into from
Oct 8, 2014

Conversation

fzembow
Copy link

@fzembow fzembow commented Sep 21, 2014

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.

@Snuffleupagus
Copy link
Collaborator

As a precursor to the review, please squash the commits into one, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.

@fzembow
Copy link
Author

fzembow commented Sep 23, 2014

Thanks, Jonas -- sorry, missed that step. Commit is compacted now, tested, and ready for review!

@fzembow
Copy link
Author

fzembow commented Oct 1, 2014

Anything else I need to do in order to get this pull request reviewed? Thanks :)

@brendandahl
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Oct 1, 2014

From: Bot.io (Linux)


Received

Command cmd_preview from @brendandahl received. Current queue size: 0

Live output at: http://107.21.233.14:8877/4fc258b110a2a91/output.txt

@brendandahl
Copy link
Contributor

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.

@fzembow fzembow force-pushed the findcontrollerfix branch 2 times, most recently from a18f552 to e97a01c Compare October 1, 2014 23:53
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.
@fzembow
Copy link
Author

fzembow commented Oct 1, 2014

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;
Copy link
Collaborator

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?

@fzembow
Copy link
Author

fzembow commented Oct 2, 2014

Updated -- not re-calculating numPages now.

brendandahl added a commit that referenced this pull request Oct 8, 2014
Update findbar to wrap on initial search
@brendandahl brendandahl merged commit c8d729f into mozilla:master Oct 8, 2014
@brendandahl
Copy link
Contributor

Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Viewer findbar input should automatically wrap
5 participants