Text positioning fixes for odd fonts #1915
Conversation
|
Changes Unknown when pulling 599c3d0 on MicahChambers:issue-1660-text-bb into * on python-pillow:master*. |
| lineHeightPercent = 100 | ||
| try: | ||
| lineHeight = font.font.height | ||
| except Exception as exc: |
There was a problem hiding this comment.
exc is assigned to but unused.
Do we want to catch all exceptions, or would it be better just to catch some and let others go up?
There was a problem hiding this comment.
@v-python what got raised here? An AttributeError I'm guessing?
There was a problem hiding this comment.
Yes. When integrated with Pillow, that try/except may not be needed. The code was dealing with whether or not font.font.height was defined, which was a recent addition, but of course not quite as recent as this code is being added, so you can probably scoot line 333 to the left and drop the try and the whole except block.
|
@MicahChambers Thanks for the PR. Please can you summarise what this actually fixes/adds, and include unit tests? (It'd be nice for the code to follow pep8 standards where suitable: |
| font = self.getfont() | ||
| ink, fill = self._getink( fill ) | ||
| if ink is None: | ||
| inx = fill |
|
Thanks for the review! I'll look over it tonight. |
|
@v-python @hugovk I'll go through and do a little cleanup. I'll
The reason I didn't do this before when I was dumping @v-python 's code into a PR is that I have submitted PR's to open source projects in the past and gotten no response -- so I didn't want to spend a bunch of time if it wasn't going to get picked up. @hugovk you can probably ignore this for a week and I'll ping you when things are more "pythonic" |
|
Changes Unknown when pulling e3ad94e on MicahChambers:issue-1660-text-bb into * on python-pillow:master*. |
|
Which function? It's better to add new tests than replace existing ones. We want to keep existing ones to guard against regressions (but of course update any where functionality has changed). |
|
Oh, I mean, |
|
Current text() tests might be revised to test draw_at_pos(). But IIRC, text doesn't let you specify a baseline... it positions the top left corner, which is not totally useless, but it makes it really hard to line things up that don't want to all be the same font or same font size. So updating text() to have the function of draw_at_pos() would require a significant rework of the code of either or both... and it would lose the ease-of-use of having things be aligned on a particular baseline. |
radarhere
changed the title
|
I'm sorry this effort seems to have stalled, and bit-rotted. But now it seems Pillow doesn't even support text on Windows anymore. #3407 |
PR for #1660