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

update Pillow to >=10.0.0 and update get_text_height api #323

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

lizhq
Copy link
Contributor

@lizhq lizhq commented Oct 18, 2023

update Pillow to >=10.0.0 and update get_text_height api

completely

#315
@Belval

@rtaylo45 rtaylo45 mentioned this pull request Nov 28, 2023
Copy link
Contributor

@stweil stweil left a comment

Choose a reason for hiding this comment

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

This change is necessary to fix a local installation from the source code which by default gets Pillow>=10.0.0.

getsize was deprecated in Pillow 9 and removed in Pillow 10. Since Pillow 8 the alternative getbbox exists.
See https://pillow.readthedocs.io/en/stable/releasenotes/10.0.0.html#font-size-and-offset-methods.

@@ -145,4 +145,5 @@ def get_text_height(image_font: ImageFont, text: str) -> int:
"""
Get the width of a string when rendered with a given font
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Get the width of a string when rendered with a given font
Get the height of a string when rendered with a given font

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unrelated fix of a copy+paste documentation bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pull request #334 fixes this comment.

@@ -145,4 +145,5 @@ def get_text_height(image_font: ImageFont, text: str) -> int:
"""
Get the width of a string when rendered with a given font
"""
return image_font.getsize(text)[1]
left, top, right, bottom = image_font.getbbox(text)
return bottom - top
Copy link

Choose a reason for hiding this comment

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

In my local testing with Pillow v10.2, this produces different results than before. Comparing the output of getsize and getbbox with Pillow 9.5.0 (which has both APIs):

trdg -c 5
...
text "a" get_bbox (0, 16, 16, 32) get_size (16, 32)

So counter-intuitively, get_size was returning the value of the bottom of the bounding box rather than bottom - top.

Choose a reason for hiding this comment

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

There is a working version of the fix at robertknight@5abfc4f.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, yes, you are right. Here is a similar fix.

The official Pillow documentation suggests the fix with bottom - top, but that would indeed give different results.

Copy link
Contributor

@stweil stweil left a comment

Choose a reason for hiding this comment

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

The pull request needs the change suggested by @robertknight. I tested the modifications with Python 3.7, and with that change all tests pass.

Comment on lines +148 to +149
left, top, right, bottom = image_font.getbbox(text)
return bottom - top
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
left, top, right, bottom = image_font.getbbox(text)
return bottom - top
left, top, right, bottom = image_font.getbbox(text)
return bottom

Copy link
Contributor

@stweil stweil Feb 15, 2024

Choose a reason for hiding this comment

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

Test results with original code:

Ran 63 tests in 161.579s
OK

Test result with the original pull request (return bottom - top):

Ran 63 tests in 159.993s
FAILED (failures=23)

Test result with the fix suggested by @robertknight (return bottom):

Ran 63 tests in 161.534s
OK

@stweil
Copy link
Contributor

stweil commented Feb 15, 2024

@Belval, please merge with the suggested change.

@Belval
Copy link
Owner

Belval commented Feb 15, 2024

Merged a bit too fast, master with latest commit has suggested changes.

Thank you for your contributions

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.

None yet

4 participants