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
Fix shy #1313 #1319
Fix shy #1313 #1319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Henning!
Note that the next release is 4.14 so any It would be good if there were a test because everything that isn't tested can be broken later without anyone noticing until much later... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes looks good!
Please add the @since-attribute to your new public methods.
I added |
Regarding "It would be good if there were a test ...". Of course you are right. I think what the BIRT project really needs is a regression test that allows to find visual differences in output PDFs automatically. If there is a visual difference, then something has changed, which might be a regression or an improvement. In the same way, the text for every page of the PDF should be extracted and the resulting output should be compared to the previous build. Unfortunately I don't know such a tool which is free and good and fast enough. I don't think that any other way of writing tests is sensible, because the internal code for the rendering is quite a mess and depends on so many things, it is not suited for unit tests. |
There are places where things appear to be rendered to an image and then the images are compared. E.g., these I don't claim to know much about how this works nor do I think we should hold things up because there is no test. It's likely that creating a good test would take more time than implementing the feature... |
I think for LTR text, this PR is definitely an improvement.
So, from my POV, I think it is reasonable to merge this and see if someone should notice an unintended behavior change, we can fix it then. |
Yes, that's the case here. |
As described in #1313, in some cases the PDF rendering of text was not correct, in particular if the text contains soft hyphens.
This PR fixes the errors regarding soft hyphens, and it fixes a subtle rendering error where left-most white space was not omitted.
Output with this PR (justified text):
See how the alignment is working (also works for left-aligned, right-aligned, and centered text).
Note that some of the issues mentioned in #1313 are still present:
Missing letter spacing between letters of the same word with different styles. This also causes an offset in the justification.
A different report, where the text starts with a space, but it is omitted although the dynamic text item is plain text: