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

Make PdfFileMerger.addBookmark() to behave life PdfFileWriters' #339

Merged
merged 7 commits into from Apr 24, 2022

Conversation

khalida
Copy link
Contributor

@khalida khalida commented Mar 30, 2017

I have modified PdfFileMerger.addBookmark() so that it behaves more like PdfFileWriter.addBookmark(), in order to address a bug which is mentioned here, and also here.

I have more-or-less just copy-pasted PdfFileWriter.addBookmark() into PyPDF2/merger.py and fixed the few bugs which arise.

In terms of validation, I have re-run your unit tests python -m unittest Tests.tests, and have also confirmed the self-contained example here works with the revised code using PdfFileMerger. But nothing more.

@MartinThoma MartinThoma added the PdfMerger The PdfMerger component is affected label Apr 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2022

Codecov Report

Merging #339 (001873f) into main (6729b80) will decrease coverage by 0.45%.
The diff coverage is 54.83%.

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
- Coverage   75.35%   74.89%   -0.46%     
==========================================
  Files          12       12              
  Lines        3538     3553      +15     
  Branches      815      819       +4     
==========================================
- Hits         2666     2661       -5     
- Misses        658      675      +17     
- Partials      214      217       +3     
Impacted Files Coverage Δ
PyPDF2/merger.py 68.07% <54.83%> (-4.87%) ⬇️
PyPDF2/pdf.py 81.79% <0.00%> (-0.07%) ⬇️

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 6729b80...001873f. Read the comment docs.

@MartinThoma MartinThoma added the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Apr 21, 2022
@MartinThoma MartinThoma added the workflow-bookmarks From a users perspective, bookmarks is the affected feature/workflow label Apr 22, 2022
@MartinThoma MartinThoma changed the title modified PdfFileMerger.addBookmark() to behave life PdfFileWriter.add… Make PdfFileMerger.addBookmark() to behave life PdfFileWriters' Apr 22, 2022
@MartinThoma MartinThoma merged commit 07848e5 into py-pdf:main Apr 24, 2022
@MartinThoma
Copy link
Member

Thank you for your contribution 🤗 I'm sorry it took so long - your change will be part of the release today :-)

MartinThoma added a commit that referenced this pull request Apr 24, 2022
A change I would like to highlight is the performance improvement for
large PDF files (#808) 🎉

New Features (ENH):
-  Add papersizes (#800)
-  Allow setting permission flags when encrypting (#803)
-  Allow setting form field flags (#802)

Bug Fixes (BUG):
-  TypeError in xmp._converter_date (#813)
-  Improve spacing for text extraction (#806)
-  Fix PDFDocEncoding Character Set (#809)

Robustness (ROB):
-  Use null ID when encrypted but no ID given (#812)
-  Handle recursion error (#804)

Documentation (DOC):
-  CMaps (#811)
-  The PDF Format + commit prefixes (#810)
-  Add compression example (#792)

Developer Experience (DEV):
-  Add Benchmark for Performance Testing (#781)

Maintenance (MAINT):
-  Validate PDF magic byte in strict mode (#814)
-  Make PdfFileMerger.addBookmark() behave life PdfFileWriters\' (#339)
-  Quadratic runtime while parsing reduced to linear  (#808)

Testing (TST):
-  Newlines in text extraction (#807)

Full Changelog: 1.27.8...1.27.9
VictorCarlquist pushed a commit to VictorCarlquist/PyPDF2 that referenced this pull request Apr 29, 2022
A change I would like to highlight is the performance improvement for
large PDF files (py-pdf#808) 🎉

New Features (ENH):
-  Add papersizes (py-pdf#800)
-  Allow setting permission flags when encrypting (py-pdf#803)
-  Allow setting form field flags (py-pdf#802)

Bug Fixes (BUG):
-  TypeError in xmp._converter_date (py-pdf#813)
-  Improve spacing for text extraction (py-pdf#806)
-  Fix PDFDocEncoding Character Set (py-pdf#809)

Robustness (ROB):
-  Use null ID when encrypted but no ID given (py-pdf#812)
-  Handle recursion error (py-pdf#804)

Documentation (DOC):
-  CMaps (py-pdf#811)
-  The PDF Format + commit prefixes (py-pdf#810)
-  Add compression example (py-pdf#792)

Developer Experience (DEV):
-  Add Benchmark for Performance Testing (py-pdf#781)

Maintenance (MAINT):
-  Validate PDF magic byte in strict mode (py-pdf#814)
-  Make PdfFileMerger.addBookmark() behave life PdfFileWriters\' (py-pdf#339)
-  Quadratic runtime while parsing reduced to linear  (py-pdf#808)

Testing (TST):
-  Newlines in text extraction (py-pdf#807)

Full Changelog: py-pdf/pypdf@1.27.8...1.27.9
MartinThoma added a commit that referenced this pull request Jun 23, 2022
Remove code duplication

Closes #968 (introduced with #339)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PdfMerger The PdfMerger component is affected soon PRs that are almost ready to be merged, issues that get solved pretty soon workflow-bookmarks From a users perspective, bookmarks is the affected feature/workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants