Skip to content

BUG: Fix missing page for bookmark#1016

Merged
MartinThoma merged 4 commits intomainfrom
missing-bookmark
Jun 23, 2022
Merged

BUG: Fix missing page for bookmark#1016
MartinThoma merged 4 commits intomainfrom
missing-bookmark

Conversation

@MartinThoma
Copy link
Copy Markdown
Member

@MartinThoma MartinThoma commented Jun 21, 2022

Closes #968 (introduced with #339)

@MartinThoma
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@MartinThoma
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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