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

Fixed bugs in calculating text size #3864

Merged
merged 2 commits into from Jun 19, 2019

Conversation

Projects
None yet
2 participants
@radarhere
Copy link
Member

commented May 23, 2019

Resolves #3745

@hugovk
Copy link
Member

left a comment

Please include a summary in PRs of what is changed and why. It makes them easier to review. Thanks!

@@ -630,7 +630,7 @@ font_getsize(FontObject* self, PyObject* args)


for (x = i = 0; i < count; i++) {
int index, error;
int index, error, offset, x_advanced;

This comment has been minimized.

Copy link
@hugovk

hugovk May 24, 2019

Member

What's the reason for moving these declarations before use?

This comment has been minimized.

Copy link
@radarhere

radarhere May 25, 2019

Author Member

x_advanced is new, not moved. I'm happy to move the declarations down if you like, I'm just going with the convention established in the code - I moved the offset declaration up because it is no longer inside a conditional block.

@radarhere

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

With English characters, each subsequent character ends horizontally after all of the previous ones. Pillow has a variable x, and it moves it forward with each character. Pillow presumes the last x is the maximum x value.

From the issue, there are two characters - the backwards S at the bottom, and the line at the top. The backwards S ends horizontally after the line at the top - the horizontal clipping occurs because Pillow stops the size after the line at the top. So instead, check each character to see if it has the maximum x value.

The first diagram at https://www.freetype.org/freetype2/docs/tutorial/step2.html is helpful in understanding the glyph properties.

With regards to changing the vertical clipping, I presume that Pillow is not correctly processing the fact that the line at the top does not start at the baseline - there is a gap. Flipping the signs when applying the offset fixes this problem, with the only affect on existing tests being that a text string is moved vertically up.

Let me know if anything is still unclear.

@radarhere radarhere force-pushed the radarhere:size branch from c004131 to 70a4c08 Jun 6, 2019

@hugovk

hugovk approved these changes Jun 19, 2019

Copy link
Member

left a comment

Please resolve the merge conflicts and let's merge.

@radarhere radarhere force-pushed the radarhere:size branch from 70a4c08 to e143727 Jun 19, 2019

@radarhere radarhere force-pushed the radarhere:size branch from e143727 to ea0f1c6 Jun 19, 2019

@hugovk hugovk merged commit 26182dd into python-pillow:master Jun 19, 2019

15 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
python-pillow.Pillow Build #20190619.14 succeeded
Details
python-pillow.Pillow (Lint Python37) Lint Python37 succeeded
Details
python-pillow.Pillow (alpine) alpine succeeded
Details
python-pillow.Pillow (amazon_1_amd64) amazon_1_amd64 succeeded
Details
python-pillow.Pillow (amazon_2_amd64) amazon_2_amd64 succeeded
Details
python-pillow.Pillow (arch) arch succeeded
Details
python-pillow.Pillow (centos_6_amd64) centos_6_amd64 succeeded
Details
python-pillow.Pillow (centos_7_amd64) centos_7_amd64 succeeded
Details
python-pillow.Pillow (debian_stretch_x86) debian_stretch_x86 succeeded
Details
python-pillow.Pillow (fedora_29_amd64) fedora_29_amd64 succeeded
Details
python-pillow.Pillow (fedora_30_amd64) fedora_30_amd64 succeeded
Details
python-pillow.Pillow (ubuntu_16_04_xenial_amd64) ubuntu_16_04_xenial_amd64 succeeded
Details
python-pillow.Pillow (ubuntu_18_04_bionic_amd64) ubuntu_18_04_bionic_amd64 succeeded
Details

@radarhere radarhere deleted the radarhere:size branch Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.