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

Support using PDF, in addition to images, as the signature graphic #1181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Alkaid-Benetnash
Copy link

@Alkaid-Benetnash Alkaid-Benetnash commented Jun 15, 2024

Description of the new Feature

Support using PDF (vector graphics), in addition to images (i.e., png, jpg) as the visible signature.
I implemented this feature by adding a new function setSignaturePDF. Then I copy a configurable page from the given PDF as the signature and render it to the proper place.

Related Issue: #777

Unit-Tests for the new Feature/Bugfix

  • Unit-Tests changed for the added feature

Compatibilities Issues

Only added new methods. No existing method signature or functionality changes. Should have no compatibility issues.

Your real name

No conflict of interest.
I am a jsignpdf user, who wants to embed vector signatures into pdf, just like the other user in #777.
Prefer to not provide real name unless maintainer insists.

Testing details

Updated the PdfSignatureAppearance test to use both an image signature (old feature, never tested) and a PDF signature (new feature).
I also changed the jsignpdf application to make use of this new feature. Will update a reference here once the jsignpdf PR is created.

Copy link

sonarcloud bot commented Jun 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@Alkaid-Benetnash
Copy link
Author

For end-to-end testing with jsignpdf, please refer to this PR intoolswetrust/jsignpdf#185

Copy link
Member

@asturio asturio left a comment

Choose a reason for hiding this comment

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

Hi there,

at first sorry for the late reaction. Too much work at work.

In the original code there were to Blocks which differ a little bit, by the calculation of y. So the semantic is changed if both blocks are calling the same method now: renderSignatureGraphics().

There is this 15there. So probably something will break here.

Could you please take a look at that.

OpenPDF has a very bad test coverage, and caution must be taken.

New test for existing code (code you change) are very welcome.

// experimentation found this magic number to counteract Adobe's
// signature graphic, which
// offsets the y co-ordinate by 15 units
float y = -im.getScaledHeight() + 15;
Copy link
Member

Choose a reason for hiding this comment

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

Here is some kind of weird initialization.

@@ -674,48 +708,9 @@ public PdfTemplate getAppearance() throws DocumentException {

ct2.go();
} else if (render == SignatureRenderGraphicAndDescription) {
ColumnText ct2 = new ColumnText(t);
Copy link
Member

Choose a reason for hiding this comment

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

Block 1

} else if (this.render == SignatureRenderGraphic) {
ColumnText ct2 = new ColumnText(t);
Copy link
Member

Choose a reason for hiding this comment

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

Block 2

* @param t the new PDF object/layer to render on
*/
private void renderSignatureGraphic(PdfTemplate t, Rectangle signatureRect) {
if (this.signatureGraphic != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Content of Block 2 only

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.

2 participants