-
Notifications
You must be signed in to change notification settings - Fork 712
Add support for loading TrueType font files. #1960
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
Conversation
I think this is a good feature to have, only two questions:
|
|
Thanks! Could you please update the documentation of the parameter (in the docstring of |
Yes, of course! |
I tested it and cannot instantiate new font objects with different styles because it throws an OSError. |
…e user specified a variable font in "font_name" argument.
Sorry for the long inactivity, I wasn't available at the moment. |
This also looks good to me, but there's no test for it (nor do I see how this can be tested easily.) @birkenfeld , @jean-abou-samra , any concerns with that? I'm not super happy to merge code without tests but I don't see an easy way to test image based output. |
return font | ||
except ValueError: | ||
pass | ||
except OSError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indicates an invalid font, right? Shouldn't there be a warning or an error?
font.set_variation_by_name(style_name) | ||
return font | ||
except ValueError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this case for? I don't see ValueError
mentioned at
https://pillow.readthedocs.io/en/stable/reference/ImageFont.html#PIL.ImageFont.FreeTypeFont.set_variation_by_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it guarantee that no ValueError
is raised? I'd guess some of the code could do that ... alternative would be to catch only OSError
and pass in all other cases?
@@ -254,6 +290,8 @@ class ImageFormatter(Formatter): | |||
The font name to be used as the base font from which others, such as | |||
bold and italic fonts will be generated. This really should be a | |||
monospace font to look sane. | |||
If a filename or a file-like object is specified, the user must | |||
provide different styles of the font. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, a typo in the file name will result in font_name
being understood as the name of a font and not a path, and the errors will read 'No usable fonts named ...' or something like that depending on the system. Can we make it explicit rather than implicit and use a separate font_path
parameter?
No, I don't see one either, especially that font stuff is so platform-dependent :( So I think it's OK if this doesn't have tests. |
Merged for 2.16. Let's see if this works in the wild, thanks for the contribution! |
In the ImageFormatter class, we can load ttf fonts that already exists in the operating system.
However, the "font_name" argument does not accept a custom font (a filename or a file-like object containing a ttf font).
I hope that this pull request solves this problem.