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.

Sign up for GitHub

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

Jump to bottom

Update PdfBoxGraphics2DFontTextDrawer.java #52

Closed
wants to merge 4 commits into from
Closed

Update PdfBoxGraphics2DFontTextDrawer.java #52

wants to merge 4 commits into from

Conversation

fransbouwmans
Copy link

Support AttributedString wit Family, Weight, and Posture attributes

@fransbouwmans
Update PdfBoxGraphics2DFontTextDrawer.java
96043c7 
Support AttributedString wit Family, Weight, and Posture attributes
@rototor
Copy link
Owner

rototor commented Jun 21, 2023

Thanks for the PullRequest. I'll try to look into integrating this and release a new version tomorrow evening.

@fransbouwmans
Copy link
Author

fransbouwmans commented Jun 21, 2023 via email

@rototor
Copy link
Owner

rototor commented Jun 21, 2023

This works by using the issue number like #51 in the title and/or commit message. That way you get the link to the issue. So it's just using markup for that.

@fransbouwmans
Copy link
Author

Had a look at the super/subscript and there is an easy way to get the Font with transform for this. However the current use of the transform does not seem to be correct. i.e. a text with several segments having different attributes and with a transform does not continue properly.

  • An attribute transform is a temporary transform, i.e. displacement in y (e.g. subscript) is not cummulative, once done with subscript it should return to 0.
  • Currently the currentposition is added to the transform in order to keep the contextstream transform good for the next character, however this cannot be used to calculate the delta position (which it does).

So in order for this to work, we need:

  1. baseline transform that is used to position each segment in the forward movement
  2. context transform that is used to position the actual text, (i.e. font transform is applied)
  3. text transform that is used to calculate the forward movement of a text segment (scaling)

I will continue to look and see if there is a solution

@sonatype-lift
Copy link

sonatype-lift bot commented Jun 25, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/rototor/pdfbox-graphics2d/52.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/rototor/pdfbox-graphics2d/52.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser. ↩

@fransbouwmans
Copy link
Author

Updated handling of attributeFont with transform (such as superscript and subscript). This showed some issues with current implementation. Not a clean solution as the tranform matrix from the font is applied to the matrix of the contentstream. As a result moving forward on the baseline is not correct. This should now be resolved, however testing was limited to specific transforms introduced by superscript and subscript. Unclear what other transforms can happen and how they may impact the result.

@fransbouwmans
Copy link
Author

Example of panel with text drawn by default graphics2D:
image
And then in pdf:
image

@fransbouwmans
Copy link
Author

I do note that the specific source file has a deviating source formatting, an automatic source format changed a lot. I undid most to allow for a better review, but it would be good to apply standard java (or eclipse) formatting to the source file.

@rototor @sonatype-lift
Apply suggestions from sonartype-lift bot
d698430 
Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>
@rototor rototor self-requested a review June 25, 2023 19:42
@rototor
Copy link
Owner

rototor commented Jun 25, 2023

The source formating looks a bit strange, but it's the coding convention of PDFBox.

@rototor
Copy link
Owner

rototor commented Jun 25, 2023

This code is all very tricky. And still not 100% right. And I can get no super or sub script to work at all.

Would you mind providing your test code? ==> There is a class FontTest in the tests. Just Copy&Paste the first testAntonioFont() method and insert your code there. And rename the outfile (second argument of exportGraphics). Then you get a target/test/fonts/.png and a target/test/fonts/.pdf. The png is generated using a BufferedImage, i.e. it's the "so it should look like" case. And the PDF contains multiply pages with different font rendering settings and scalings of the form (because sometimes gradients look different depending on the outside scaling; Yes thats another bug I still have in this project)

Your changes change the (already not correct) matrix support code. E.g. the FontTest::testTransformedFont case. Before it was
image
but with your changes this looks like this
image

But it should really look like
attributed_transformed_text

==> Please provide a test method in FontTest. As I can't get superscript and subscript it to work with this branch. See this new testmethod in master.

You don't need to fix the font transformed case, but just a test case that I can see that your changes really work.

@fransbouwmans
Copy link
Author

fransbouwmans commented Jun 25, 2023 via email

@fransbouwmans
Copy link
Author

Not sure how to merge the updated master, close and will try a new pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift[bot] sonatype-lift[bot] left review comments

@rototor rototor Awaiting requested review from rototor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fransbouwmans @rototor