Corrected ttb text positioning #3856
Conversation
|
Thanks! Is there a way to get the libraqm version programatically? If not, shall we open an issue with libraqm to ask them to add some API? It may be useful in the future. |
|
To be clear, this is testing to see if libraqm 0.6 is present, just not through a version string - I do it by testing for the existence of an unrelated function that was added in 0.6. I've created HOST-Oman/libraqm#103 requesting a version identifier. |
|
Thanks! It could be that the unrelated feature is removed in 0.7 or some future version, and then breaking this fix. |
src/_imagingft.c
Outdated
| if (p_raqm.version == 1){ | ||
| for (i = 0; i < count; i++) { | ||
| for (i = 0; i < count; i++) { | ||
| if (p_raqm.version == 1) { |
There was a problem hiding this comment.
I think it would be faster to check the condition outside the loop.
There was a problem hiding this comment.
You're right. I've updated the commit
src/_imagingft.c
Outdated
| if (p_raqm.version == 1){ | ||
| for (i = 0; i < count; i++) { | ||
| for (i = 0; i < count; i++) { | ||
| if (p_raqm.version == 1) { |
There was a problem hiding this comment.
I think it would be faster to check the condition outside the loop.
There was a problem hiding this comment.
This looks like a duplicate comment.
Tests/test_imagefontctl.py
Outdated
| im = Image.new(mode='RGB', size=(100, 300)) | ||
| draw = ImageDraw.Draw(im) | ||
| try: | ||
| draw.text((0, 0), 'English عربي', font=ttf, fill=500, direction='ttb') |
There was a problem hiding this comment.
Wouldn’t it be better to actually test some CJK characters here?
There was a problem hiding this comment.
I'm having trouble finding a small CJK font that could be included within the Pillow license. Do you have any thoughts?
There was a problem hiding this comment.
Actually, I've found one that's not too large.
There was a problem hiding this comment.
You can also subset any font with pyftsubset (from FontTools) to include just the few characters used in the test.
src/_imagingft.c
Outdated
| if (horizontal_dir) { | ||
| x += glyph_info[i].x_advance; | ||
| } else { | ||
| y -= glyph_info[i].y_advance; |
There was a problem hiding this comment.
I think the condition is not needed, you can assign x and y unconditionally. In horizontal direction y_advance will be zero and in vertical direction x_advance will be zero.
There was a problem hiding this comment.
Okay. I've updated the commit.
|
Now HOST-Oman/libraqm#104 is merged and released in https://github.com/HOST-Oman/libraqm/releases/tag/v0.7.0, can we replace the version inference with a direct check for 0.7.0+? |
|
Okay, done. |
radarhere
force-pushed
the
ttb
branch
4 times, most recently
from
bae7549
to
40a7cf1
Compare
Helps #2656 and #3125
libraqm 0.4 and 0.5 throw an error for ttb - see HOST-Oman/libraqm@7088743. 0.3 works, but since there is no simple way to test for the difference between 0.3 and 0.4, for simplicity I'm saying that ttb requires libraqm 0.6 or later.