-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
PR: Return original font directory when font user installation load fails #273
base: master
Are you sure you want to change the base?
Conversation
# Regardless of the font file being actually removed | ||
# since the load failed we need to return here where | ||
# the font file is originally located. With that a | ||
# `FontError` can be eventually raised with useful |
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.
Instead of returning fonts_directory
below, why don't we raise a FontError
there, saying that it was not possible to load a specific font? The error message could also use get_fonts_info
to tell users what fonts they need to install manually and where they are located.
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.
After giving it some thought, if we raise FontError
here the fallback logic to use system wide fonts will never be reached (since then always an error here will be raised). Maybe we should show instead a warning here and only in case we are unable to detect the font that failed being loaded is available from the system fonts directory? What do you think @ccordoba12 ?
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.
Last commit implements the idea above but let me know what you think
Sorry for closing this PR (I don't know why it happened). |
Fixes #264