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

[api-minor] Change getPageLabels to take a boolean value to indicate if standard page numbering should be skipped #6923

Closed
wants to merge 1 commit into from
Closed

Conversation

Snuffleupagus
Copy link
Collaborator

When prototyping the viewer integration for pageLabels, it became apparent that returning an empty array for "standard" page numbering is quite clunky.
It should obviously be possible to fetch the pageLabels, through the API, exactly as they appear in the PDF file. Sorry about this oversight!

/cc @timvandermeij Since you reviewed PR #6803, would you mind reviewing this as well?

Edit: Perhaps marginally easier reviewing with https://github.com/mozilla/pdf.js/pull/6923/files?w=1.

…e if standard page numbering should be skipped

When prototyping the viewer integration for pageLabels, it became apparent that returning an empty array for "standard" page numbering is quite clunky.
It should obviously be possible to fetch the pageLabels, through the API, *exactly* as they appear in the PDF file. Sorry about this oversight!
@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/c5e82ccf7156943/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/6cd042f18d54e19/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/6cd042f18d54e19/output.txt

Total script time: 1.17 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/c5e82ccf7156943/output.txt

Total script time: 1.41 mins

  • Unit Tests: Passed

@yurydelendik
Copy link
Contributor

Shall we remove the ignoreStandardNumbering parameter? we can update the 1.4.x version.

@timvandermeij
Copy link
Contributor

The API is new it is not yet in use, so I would also say that we can remove that parameter.

@Snuffleupagus
Copy link
Collaborator Author

I'm sorry, but I don't understand the question about removing the ignoreStandardNumbering parameter. This PR adds it, in order to fix an issue with the current API.

In practice, it's unfortunately more common than you might think for PDF files to include pageLabels that correspond to "normal" page numbering. In the viewer I think that we'll want to be able to ignore these, which seem to happen in Adobe Reader as well, since they don't add any real value.

The point of this PR is thus to let us skip such pageLabels, by using the parameter, while still enabling API consumers to always retrieve pageLabels if/when they exist in the PDF file.
Otherwise we would have a situation where getPageLabels in the API will return null, with no way of telling if that's because no pageLabels exist, or if it's caused by them being equal to "normal" numbering.

@yurydelendik, @timvandermeij Are you saying that we should just ignore pageLabels using "normal" numbering, and not care about the API being unable to retrieve the pageLabels as defined in the PDF file in those cases?

@yurydelendik
Copy link
Contributor

For me it got more confusing, we may need to adjust API to return whatever we got in the dictionary. If the page labels contains standard number we will keep it as is (excluding the post processing/conversion), if it's absent -- we return null. The UI may pre-process the resulting array by itself and "determine" if it's standard numbering. Introducing "ignoreStandardNumbering" will make API more complicated IMHO.

The API still at "beta" phase so we can adjust it.

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.

None yet

4 participants