Skip to content

formatters/img: use new API for bounding box for Pillow 9.2 #2198

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

Merged
merged 2 commits into from
Aug 15, 2022
Merged

Conversation

birkenfeld
Copy link
Member

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@birkenfeld birkenfeld requested review from Anteru and jeanas July 30, 2022 07:13
@jeanas
Copy link
Contributor

jeanas commented Jul 30, 2022

I looked at this a bit and was confused by how characters are measured. Is this use of getbbox equivalent to how getsize was used before, e.g. does it handle the height of descenders (as in ‘q’ or ‘g’) the same way? If so LGTM.

@birkenfeld
Copy link
Member Author

TBH I just looked at https://pillow.readthedocs.io/en/stable/_modules/PIL/ImageFont.html#ImageFont.getsize which shows that all of the three methods (getsize, getbbox, getlength) use the same underlying API...

return self.fonts['NORMAL'].getsize(text)
font = self.fonts['NORMAL']
if hasattr(font, 'getbbox'): # Pillow >= 9.2.0
return font.getbbox(text)[2:4]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return font.getbbox(text)[2:4]
return font.getlength(text)

Copy link
Member Author

Choose a reason for hiding this comment

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

If I read the docs correctly, getlength only returns a single value, while getsize returns (width, height).

Copy link
Collaborator

@Anteru Anteru left a comment

Choose a reason for hiding this comment

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

What was the reason to pick getbox over getlength? The underlying API is the same and this avoids unpacking the tuple.

@Anteru Anteru added the A-formatting area: changes to formatters label Aug 15, 2022
@Anteru Anteru added this to the 2.13.0 milestone Aug 15, 2022
@Anteru Anteru merged commit 97a2f18 into master Aug 15, 2022
@Anteru Anteru deleted the pillow92 branch August 15, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-formatting area: changes to formatters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants