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

Allow PdfCopy to be used for writing new pages #1166

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

rasmusfaber
Copy link
Contributor

Description of the new Feature/Bugfix

Remove unnecessary overridden methods from PdfCopy thus allowing it to be used to write new pages as well as copy them.

Related Issue: This fixes #1165

Unit-Tests for the new Feature/Bugfix

  • Unit-Tests added to reproduce the bug
  • Unit-Tests added to the added feature

Compatibilities Issues

The behavior of PdfCopy is of course changed, since it now actually adds new pages if requested. I would expect that anyone that does that, actually intended to do it.

Your real name

Rasmus Faber-Espensen

Testing details

See unit test.

@asturio
Copy link
Member

asturio commented May 27, 2024

This change will have twofold results:

  • Semantics of PdfCopy will change
  • PdyCopy can be used to merge PDFs.

Leaving the PR open for the community to drop some 2 cents.

@mkl-public
Copy link
Contributor

Leaving the PR open for the community to drop some 2 cents.

You mentioned in review:

I'm just not sure, if allowing PdfCopy to manipulate the content of the document is what the original author planed for the class.

Not only have they not planned that, this feature has not been tested for years in use. I would not be surprised if there were numerous corner cases for which the feature will fail.

Thus, a larger number of tests of different situations would be nice. We have first adding content, then merging; other tests could be first merging, then adding content, probably also a longer mix of mergers and content additions.

Nonetheless, the feature of course is interesting, it also was missing in all pre-7 versions of iText and there were requests again and again.

@rasmusfaber
Copy link
Contributor Author

Thus, a larger number of tests of different situations would be nice. We have first adding content, then merging; other tests could be first merging, then adding content, probably also a longer mix of mergers and content additions.

The single test I wrote already added/merged/added. I just added another test that interleaves two files and combines it with adding a page in front of each.

@rasmusfaber
Copy link
Contributor Author

I'm just not sure, if allowing PdfCopy to manipulate the content of the document is what the original author planed for the class.

Not only have they not planned that, this feature has not been tested for years in use. I would not be surprised if there were numerous corner cases for which the feature will fail.

Just for reference, none of the other PdfWriter specializations (PdfStamper, PdfCopyFields, PdfCopyForms etc) override these methods. If there are corner cases where this don't work, they most likely don't work for those classes either - and at least the code tries to do something now instead of just silently doing nothing.

@mkl-public
Copy link
Contributor

I just added another test that interleaves two files and combines it with adding a page in front of each.

Great, thanks!

Just for reference, none of the other PdfWriter specializations (PdfStamper, PdfCopyFields, PdfCopyForms etc) override these methods.

Neither of those classes is a PdfWriter specializations. They all wrap PdfWriter specializations (PdfStamperImp, PdfCopyFieldsImp, PdfCopyFormsImp). Admittedly, it is easy to access the respective specialization using getWriter.

If there are corner cases where this don't work, they most likely don't work for those classes either - and at least the code tries to do something now instead of just silently doing nothing.

Maybe one should give this feature in PdfCopy a chance to prove itself.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@asturio asturio merged commit 9c5aefc into LibrePDF:master Aug 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PdfCopy cannot be used for writing
3 participants