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

change PdfPageLabels.getPageLabels() to scope prefixes to the range #9

Merged
merged 2 commits into from Nov 2, 2015

Conversation

Palmr
Copy link
Contributor

@Palmr Palmr commented Oct 23, 2015

they are defined for

While reading a PDF generated by Adobe InDesign CS6 using the Adobe
PDF Library 10.0.1 which included several page label sections I
noticed iText was keeping the prefix applied to subsequent sections
in the values return by PdfPageLabels.getPageLabels(), even when the
key was omitted in the following page label dictionary entries.
According to the PDF Reference 1.7, Table 8.10 says that /P is an
optional label prefix for page labels in this range. PDF Readers I
have installed scope the prfix only to the range it is specified.
This change empties out the prefix field to give behaviour I think
aligns with the reference and my experience of other PDF readers.
I also reformatted the function as previously it was a mix of tab &
space indenting.

I am not aware of any potential regression issues introduced by this
change, existing page label dictionaries created by iText set /P to
an empty string so this will only affect reading PDFs with partially
prefixed page label ranges as far as I know.

@iText-CI
Copy link
Contributor

Can one of the admins verify this patch?

@Palmr
Copy link
Contributor Author

Palmr commented Oct 23, 2015

I read the CONTRIBUTING.md and think I followed everything (aside from posting to StackOverflow as I don't have an account and this is effectively a 2 line change).
If there's anything else I should do, let me know.

(I don't think I need to sign the iCLA for this as the actual change is minimal, only whitespace formatting and the unit test make it >20 lines. If I should sign it however, let me know as nothing is stopping me)

@amedee
Copy link
Contributor

amedee commented Oct 26, 2015

Please do not mix reformatting and changed functionality into one commit. It's easier to review when it's in two separate commits.

@Palmr
Copy link
Contributor Author

Palmr commented Oct 26, 2015

@amedee Sorry, I had whitespace comparing turned off in my diff tool and didn't notice how bad it looks on Github.
Should I close this pull and open another with separated changelists or something else?

@amedee
Copy link
Contributor

amedee commented Oct 26, 2015

If you push to your branch from where you made your PR, the PR should be automatically updated. You can even force push to your own branch.
More info here: mislav/hub#198

they are defined for

While reading a PDF generated by Adobe InDesign CS6 using the Adobe
PDF Library 10.0.1 which included several page label sections I
noticed iText was keeping the prefix applied to subsequent sections
in the values return by PdfPageLabels.getPageLabels(), even when the
key was omitted in the following page label dictionary entries.
According to the PDF Reference 1.7, Table 8.10 says that /P is an
optional label prefix for page labels in this range. PDF Readers I
have installed scope the prfix only to the range it is specified.
This change empties out the prefix field to give behaviour I think
aligns with the reference and my experience of other PDF readers.

I am not aware of any potential regression issues introduced by this
change, existing page label dictionaries created by iText set /P to
an empty string so this will only affect reading PDFs with partially
prefixed page label ranges as far as I know.
@Palmr
Copy link
Contributor Author

Palmr commented Oct 26, 2015

I did my ammend in 856b648 to make my change just the actual fix & unit tests, but then couldn't psuh without pulling & merging, hence f73c13e
I've never tried a git commit --ammend before as I'm not a regular git user, so I assume I missed a flag somewhere, but it seems to have worked in general?

@amedee
Copy link
Contributor

amedee commented Oct 26, 2015

You can do a forced push with git push -f so you won't have to pull and merge. That will replace (overwrite) your remote branch on GitHub.

The rest of the codebase seemed to be using 4 spaces to indent while
this method had been written a while ago and contained a mix of tab
characters and spaces.
@Palmr
Copy link
Contributor Author

Palmr commented Oct 26, 2015

Thanks for the advice, I think I've got it all sorted now.

  • 856b648 is the commit with the actual fix in it and the unit test
  • a1404ec is the commit with the whitespace fix so the function matches the rest of the codebase

@iText-CI iText-CI merged commit a1404ec into itext:develop Nov 2, 2015
@amedee
Copy link
Contributor

amedee commented Nov 3, 2015

Thank you for your contribution, as you can see we have merged in your pull request.

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