PPF-560 add pushbutton field, possibility to trasform it in image field

cip91sk commented 1 month ago

All Submissions:

[x] Have you followed the guidelines in the developer guide?
[x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

[x] Does your submission pass tests?
[x] Have you lint your code locally prior to submission? (I run pylint locally)

Changes to Core Features:

[ ] Have you added an explanation of what your changes do and why you'd like us to include them? (I don't really know where to add an explanation for this, recognizing all pushbuttons as such instead of recognizing only a subset as image fields)
[ ] Have you written new tests for your core changes, as applicable? (I don't know if there's a need for new tests, I modified some of them to correctly run with these changes)
[x] Have you successfully ran tests with your changes locally?

Basically this PR is based on my last comment on the issue , it removes the special image field and treats all pushbuttons equally, with the possibility to set an image to them

chinapandaman commented 1 month ago

Hey, thanks for contributing.

I read your last comment in the issue thread. While I appreciate your effort into making these changes, those reasons still do not validate the changes.

Bit 17 of field flag does differentiate push button from checkbox and radio buttons. However that only makes a button widget a push button, not an image field. I have explained the key behavioral difference. Image fields, when clicked using a lot of PDF editors, will pop up and let you insert an image. A push button does not fall under that behavior. For example in the test you modified (test_clear_form_button_not_checkbox), the Clear widget is a push button indeed, however it is not an image field as it will not pop up and let you insert an image. So no I cannot just simply remove image fields and loosely let an image being able to be inserted into push buttons.

Finally, if you really need to fill an image onto a push button, you always have the option of simply drawing on it.

cip91sk commented 1 month ago

After reading your comment I went another way: images can only be set for Image Fields, PushButtons are recognized as such and basically nothing can be done on them right now, except that they can be converted to Image Fields (and then an image is appliable). It works even in FormWrapper, so that you can save again the form ready for an image to be set.

Would this approach be okay?

chinapandaman commented 1 month ago

Hey, sorry for a rather late response. I was on vacation last week and did not have my computer with me.

Anyway, I checked out your changes in this PR and gave this snippet a try:

from PyPDFForm import PdfWrapper

new_form = PdfWrapper("pdf_samples/scenario/existed/ds11_pdf.pdf")
new_form.pushbutton_field_to_image_field("Clear")

with open("temp/output.pdf", "wb+") as output:
    output.write(new_form.read())

I downloaded the generated output.pdf and opened it in Adobe Acrobat. When I clicked on the supposedly now image field Clear I do see the image selection popup show up.

However, when I actually attempted to insert an image to Clear, the image was not able to be inserted as the button still displays the text "Clear Form".

For an actual image field, the image was successfully inserted.

So I believe while event.target.buttonImportIcon(); does enable the popup, it is not the only thing that makes a fully functional image field. Which also makes me think about making the rules that checks if a widget is image field even more strict (although I don't know how right now).

On top of the above, there are couple other reasons why I'm not a big fan of these changes:

We are introducing a new widget type Pushbutton which has no actual...meaning? It doesn't support any data type to fill and will only introduce confusion when calling .schema. In fact the snippet I posted in this comment errors out initially because you didn't implement these two methods in the Pushbutton sub-class. But then the question is what do you implement them as?
Lots of code in your changes need to be refactored for better structure (for example there is already a check field flag function so there's no need to write a new one). This is a minor issue and although I don't want to leave a million comments to tell you how to change them, I could have you re-open the PR against a feature branch so I can revise your changes before merging them to master.

Again, much appreciated effort as I could tell you studied the project's code structure and made as much appropriate changes as you can. But I still cannot merge them for the above reasons. Let me know if you have more questions.

chinapandaman / PyPDFForm