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

Misleading deprecations documentation for getsize #7802

Closed
stweil opened this issue Feb 15, 2024 · 5 comments · Fixed by #7806
Closed

Misleading deprecations documentation for getsize #7802

stweil opened this issue Feb 15, 2024 · 5 comments · Fixed by #7806

Comments

@stweil
Copy link

stweil commented Feb 15, 2024

The documentation suggests that this old code

width, height = font.getsize("Hello world")

should be replaced by

left, top, right, bottom = font.getbbox("Hello world")
width, height = right - left, bottom - top

A 1:1 replacement would require different new code

left, top, right, bottom = font.getbbox("Hello world")
width, height = right - left, bottom

The documentation should mention that and maybe link to this comment.

@radarhere
Copy link
Member

I think the argument would be that the old methods were incorrect, which is why they were deprecated, and that we're encouraging users to use the correct new methods, with the new correct results.

Explaining to users how to keep things as they were seems to work against that purpose.

@stweil
Copy link
Author

stweil commented Feb 15, 2024

Yes, sure, but a short comment which explains that the old method was incorrect would not hurt, but help. There might be old code which circumvented the deficits of the old method, and there is code which requires additional changes, not simply the suggested replacement.

Belval/TextRecognitionDataGenerator#323 for example changes the code as suggested in the deprecations documentation, but that breaks the unit test of that software.

@radarhere
Copy link
Member

I've created #7806. See what you think.

@quark67
Copy link

quark67 commented Feb 19, 2024

I think that the documentation should include the transition code, as provided by OP. So old code can easily be converted without changing the output. And as it's not easy to understand that the old "height" is yet obtened by the new "bottom", the transition code added to the documentation can be very heplful. Or, if you would not add the transition code, perhaps a picture showing what is really the "bottom" in the font.getbbox("Hello world") can also help. Thanks. Big up to @stweil for his transition code in this issue. Very very very helpful.

@radarhere
Copy link
Member

radarhere commented Feb 19, 2024

The OP spoke about this change breaking unit tests in Belval/TextRecognitionDataGenerator#323. I don't personally think that necessitates simulating Pillow's old behaviour - I think updating the unit test is the more thought out solution.

In the case of uvipen/ASCII-generator#17, it looks like the code is drawing characters in order to evaluate the brightness. Pillow's old behaviour would have meant that some blank space was included above the character. I don't think you want to consider that in that calculation.

So I'm still not seeing why we should be encouraging users to continue doing things in a less-than-ideal way. I think this additional documentation should make it clear enough for users to figure it out how to do so on their own if they really want.


As for a picture, sure, I've added that to the PR.

https://pillow--7806.org.readthedocs.build/en/7806/deprecations.html#font-size-and-offset-methods
size_vs_bbox

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants