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

BUG: Fix missing page for bookmark #1016

Merged
merged 4 commits into from Jun 23, 2022
Merged

BUG: Fix missing page for bookmark #1016

merged 4 commits into from Jun 23, 2022

Conversation

MartinThoma
Copy link
Member

@MartinThoma MartinThoma commented Jun 21, 2022

Closes #968 (introduced with #339)

@MartinThoma
Copy link
Member Author

Using

from PyPDF2 import PdfMerger

merger = PdfMerger()
merger.append("crazyones.pdf")

bookmark = merger.add_bookmark("A bookmark", 0)
bm2 = merger.add_bookmark("deeper", 1, parent=bookmark)

merger.write("issue968.pdf")
merger.close()

The first level has a bookmark, but the second has not:

image

@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #1016 (cc09600) into main (7d820f0) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head cc09600 differs from pull request most recent head ce70b1c. Consider uploading reports for the commit ce70b1c to get more accurate results

@@            Coverage Diff             @@
##             main    #1016      +/-   ##
==========================================
+ Coverage   89.30%   89.34%   +0.03%     
==========================================
  Files          24       24              
  Lines        4443     4420      -23     
  Branches      921      917       -4     
==========================================
- Hits         3968     3949      -19     
+ Misses        323      321       -2     
+ Partials      152      150       -2     
Impacted Files Coverage Δ
PyPDF2/_merger.py 87.59% <100.00%> (+0.44%) ⬆️
PyPDF2/_writer.py 88.90% <100.00%> (-0.03%) ⬇️
PyPDF2/generic.py 91.25% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d820f0...ce70b1c. Read the comment docs.

@MartinThoma
Copy link
Member Author

The issue with the second page was simply that I specified page index 1, but only index 0 exists. After fixing it, the nested one worked.

@MasterOdin
Copy link
Member

Your PR description does not make sense to me, as I'd hope that a yet to be merged PR does not introduce a bug that's existed since before the PR existed. Did you mean to reference 07848e5?

@MartinThoma
Copy link
Member Author

@MasterOdin Thank you for the hint - I wanted to references #339

@MartinThoma
Copy link
Member Author

@pubpub-zz Do you think this PR ok?

@MartinThoma MartinThoma merged commit a412e26 into main Jun 23, 2022
@MartinThoma MartinThoma deleted the missing-bookmark branch June 23, 2022 19:26
@pubpub-zz
Copy link
Collaborator

Sorry I missed this messsge
I'm a little doubtful about setting the page number in the destination : for me it should be a indirect object not a NumberObject

@MartinThoma
Copy link
Member Author

I've tried it with the indirect object, but that failed for the PdfMerger. I don't know why.

@pubpub-zz
Copy link
Collaborator

the referencing of the page into the xref (in order to define the object number seems to be done when writing. A little complex to find solution in a few minutes. Let's wait for a new issue to clarify I'm right or wrong

@MartinThoma MartinThoma added the is-bug From a users perspective, this is a bug - a violation of the expected behavior with a compliant PDF label Jun 26, 2022
MartinThoma added a commit that referenced this pull request Jun 26, 2022
New Features (ENH):
-  Support R6 decrypting (#1015)
-  Add PdfReader.pdf_header (#1013)

Performance Improvements (PI):
-  Remove ord_ calls (#1014)

Bug Fixes (BUG):
-  Fix missing page for bookmark (#1016)

Robustness (ROB):
-  Deal with invalid Destinations (#1028)

Documentation (DOC):
-  get_form_text_fields does not extract dropdown data (#1029)
-  Adjust PdfWriter.add_uri docstring
-  Mention crypto extra_requires for installation (#1017)

Developer Experience (DEV):
-  Use /n line endings everywhere (#1027)
-  Adjust string formatting to be able to use mutmut (#1020)
-  Update Bug report template

Full Changelog: 2.3.1...2.4.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is-bug From a users perspective, this is a bug - a violation of the expected behavior with a compliant PDF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add_bookmark doesn't set pagenum
3 participants