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

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

Merged
merged 1 commit into from
Feb 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion requirements-hw.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pillow>=7.0.0
Pillow>=10.0.0
numpy>=1.16.4,<1.17
requests>=2.20.0
opencv-python>=4.2.0.32
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pillow>=7.0.0
Pillow>=10.0.0
requests>=2.20.0
opencv-python>=4.2.0.32
tqdm>=4.23.0
Expand Down
3 changes: 2 additions & 1 deletion trdg/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"""
return image_font.getsize(text)[1]
left, top, right, bottom = image_font.getbbox(text)
return bottom - top
Copy link

@robertknight robertknight Feb 7, 2024 •
edited
Loading

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.

Copy link

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.

Comment on lines +148 to +149
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 •
edited
Loading

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