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

[#578] Add binding for AcroFormDocumentHelper::setFormFieldName method #580

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

amargerandhw
Copy link

resolves #578

Copy link
Member

@jbarlow83 jbarlow83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see other line item comments. Also, please add test cases to exercise any code you add.

(I didn't see this was a draft. Apologies if these remarks are premature.)

src/core/acroform.cpp Outdated Show resolved Hide resolved
src/pikepdf/_methods.py Outdated Show resolved Hide resolved
src/pikepdf/_methods.py Outdated Show resolved Hide resolved
src/pikepdf/objects.py Outdated Show resolved Hide resolved
py::arg("q")
)
.def(
"set_form_field_name",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be useful to anyone who say, wants to know what form fields are present before changing their names, you'll need to bind QPDFFormFieldObjectHelper as well. Then one should be able to retrieve form fields from the form and manipulate the field name from there.

I'm not going to accept a PR that solves your issue alone and isn't helpful to others. It needs to be of general use. All of the methods of QPDFFormFieldObjectHelper and QPDFAcroFormDocumentHelper should be exposed. You're probably going to need them anyway.

Please bind get/set functions as def_property which is equivalent to setting a @property with a pythonic getter and setter. Then one would use form.fields[...].name = 'MyName' etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the method getFormFields, although it is mandatory for the usage that I solve. You could simplify use it as I showed in the test.
I dont know how I should do the binding with def_property.
If I understand you would me to add it to the binding of the class QPDFFormFieldObjectHelper but the method is on the class QPDFAcroFormDocumentHelper.

Implementing all the methods would take me too much time as I'm completely a novice in python and just discovered how pybind11 works, we could merge the base that I implemented and someone could improve it in the future

@jbarlow83 jbarlow83 marked this pull request as ready for review May 8, 2024 08:31
Copy link
Member

@jbarlow83 jbarlow83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I cannot accept this PR in its current form.

As previously mentioned, get/set methods are not Pythonic and need to be replaced with pybind11 def_property.

The implementation is also lacking many features others would need.

@@ -895,6 +896,18 @@ void init_object(py::module_ &m)
.def_property_readonly("obj", [](QPDFObjectHelper &poh) -> QPDFObjectHandle {
return poh.getObjectHandle();
});
py::class_<QPDFDocumentHelper, std::shared_ptr<QPDFDocumentHelper>>(m, "DocumentHelper");
// .def(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out? It breaks existing functionality.

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.

[Feature] QPDFFormFieldObjectHelper setFormFieldName
2 participants