Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard

Misleading deprecations documentation for getsize

Open stweil opened this issue 1 year ago • 5 comments
trafficstars

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.

stweil avatar Feb 15 '24 10:02 stweil

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.

radarhere avatar Feb 15 '24 11:02 radarhere

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.

https://github.com/Belval/TextRecognitionDataGenerator/pull/323 for example changes the code as suggested in the deprecations documentation, but that breaks the unit test of that software.

stweil avatar Feb 15 '24 11:02 stweil

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

radarhere avatar Feb 17 '24 00:02 radarhere

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.

quark67 avatar Feb 19 '24 02:02 quark67

The OP spoke about this change breaking unit tests in https://github.com/Belval/TextRecognitionDataGenerator/pull/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 https://github.com/uvipen/ASCII-generator/issues/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

radarhere avatar Feb 19 '24 06:02 radarhere