ENH: Add PdfWriter.remove_objects_from_page(page: PageObject, to_delete: ObjectDeletionFlag)

[pubpub-zz]

fix remove_text to set contents as indirect_objects (iaw PDF Specification)

wipe out text also in XObject forms

Same issues + refactoring for remove_images()

closes #1644
closes #1650

[codecov]

Codecov Report

Base: 92.25% // Head: 92.40% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (b70986a) compared to base (14b61ea).
Patch coverage: 91.50% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   92.25%   92.40%   +0.15%     
==========================================
  Files          33       33              
  Lines        6431     6468      +37     
  Branches     1286     1287       +1     
==========================================
+ Hits         5933     5977      +44     
  Misses        315      315              
+ Partials      183      176       -7     

Impacted Files	Coverage Δ
pypdf/_writer.py	85.72% <91.08%> (+1.05%)	⬆️
pypdf/__init__.py	100.00% <100.00%> (ø)
pypdf/generic/_base.py	99.65% <100.00%> (+<0.01%)	⬆️
pypdf/generic/_annotations.py	97.75% <0.00%> (+0.13%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pubpub-zz]

I've completed the test file with some Form data
ANNOTED completed.pdf

[pubpub-zz]

@MartinThoma
All yours. 😊
Happy to be back over 92% of test coverage

[MartinThoma]

I would rather make this a private method:

Suggested change

	def remove_img_or_text(
	def _remove_img_or_text(

(that needs some adjustments in other parts as well)

[pubpub-zz]

I would prefer to leave it public : currently remove_images() and remove_text() are cleaning the full document : this will provide a solution to just clean one page.
However I agree the name is too tricky : I propose to rename the function into remove_image_or_text_from_page()

Your opinion ?

[MartinThoma]

Ah, I understand! So it's actually adding a new feature :-)

I don't like the image_or_text part of the method name. Maybe we can change the signature to this:

class ObjectDeletionFlag(enum.IntFlag):
    TEXT = enum.auto()
    IMAGES = enum.auto()

...

    def remove_objects_from_page(
        self, page: Union[PageObject, DictionaryObject], to_delete: ObjectDeletionFlag
    ) -> None:

I see some advantages in the proposed interface:

1. We could add the capability to delete annotations (maybe even specific annotation types) in the same method
2. We could allow users to specify multiple flags at one method call
3. Personal preference: When I see an _or_ in a method name, I tend to think that those should be two methods 😄
4. Personal preference: I think that the current use of del_image=False => delete text is not intuitive. I would like to avoid that.

[pubpub-zz]

    def remove_objects_from_page(
        self, page: Union[PageObject, DictionaryObject], to_delete: ObjectDeletionFlag
    ) -> None:

Sounds good to me.

I see some advantages in the proposed interface:
1. We could add the capability to delete annotations (maybe even specific annotation types) in the same method

Text and Images requires to remove to parse content, that's why they are specific, however from a interface point of view we can merge them.

2. We could allow users to specify multiple flags at one method 

good idea implemented.

3. Personal preference: When I see an `_or_` in a method name, I tend to think that those should be two methods 😄

 not iaw my rules...😉🤣🤣🤣🤣 

4. Personal preference: I think that the current use of `del_image=False` => delete text is not intuitive. I would like to avoid that.

btw I hate the name of the function remove_links() which name is not describing it properly. Shouldn't we have remove_annots() and remove_links() - for links only?

[MartinThoma]

I had to look it up - https://pypdf.readthedocs.io/en/latest/modules/PdfWriter.html#pypdf.PdfWriter.remove_links

Yes, that is exactly this kind of unexpected dual-purpose which I don't like -.-

I want to keep the amount of breaking changes low for this year. Give people a chance to adapt. But we should make a list of such things to be cleaned-up in future.

Additionally, the proposed remove_objects_from_page could support links and annotations as well (just by adding it to the ObjectDeletionFlag / letting users provide that value.

[pubpub-zz]

for extra tests
GeoBaseWithComments.pdf

[MartinThoma]

I think we should make our lives here easier and not allow the Iterables (tuple / list). It is a flag and thus people should use it.

If they don't know it, they will just do multiple calls (which is fine as well)

[pubpub-zz]

I do not see how to change keeping the code just affecting annotations/comments...

[MartinThoma]

@pubpub-zz I would like to get this merged in the release today. Would it be OK for you if I take care of the remaining (rather stylistic) changes within your PR?

[pubpub-zz]

last point remaining is about LIST/TUPLES

[pubpub-zz]

@MartinThoma All yours 😊

[MartinThoma]

Very nice PR! I've just merged it and will release it today :-)

Good work @pubpub-zz 👏