search

LibrePDF / OpenPDF

OpenPDF is a free Java library for creating and editing PDF files, with a LGPL and MPL open source license. OpenPDF is based on a fork of iText. We welcome contributions from other developers. Please feel free to submit pull-requests and bugreports to this GitHub repository.
Other
3.63k stars 598 forks source link

Allow PdfCopy to be used for writing new pages #1166

Closed rasmusfaber closed 3 months ago

rasmusfaber commented 6 months ago

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

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 commented 6 months ago

This change will have twofold results:

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

mkl-public commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

sonarcloud[bot] commented 6 months ago

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