Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Font weight warning if found font sufficiently different#15615
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/font_manager.py Outdated
| @@ -1275,6 +1279,12 @@ def _findfont_cached(self, prop, fontext, directory, fallback_to_default, | |||
| best_font = font | |||
| if score == 0: | |||
| break | |||
| if best_font is not None: | |||
| if abs(map_weight_name_to_score(prop.get_weight()) | |||
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.
pep8 is pretty unhappy about the formatting here.
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.
Thanks! This should be better.
Could you also add a test that warnings are actually raised when we miss a weight? We will always have dejavu installed so maybe one of the very high or low weights? |
Thanks @colathro ! It was great working with you today! |
I think we should warn anytime there's no exact match, rather than with a threshold of 100. |
| @@ -86,6 +86,10 @@ | |||
| 'extra bold': 800, | |||
| 'black': 900, | |||
| } | |||
| def map_weight_name_to_score(weight): | |||
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 should be private
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.
When you say private, I think of other language access modifiers. In Python would that be just making this a class method?
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.
Just prepend an underscore to its name.
I'm not quite able to figure out the best way to do this. I can't quite get the output to appear in the warnings no matter what I try. Is this due to the logger not actually implementing Warnings? |
zufuliu commentedNov 14, 2019
The font weight dict looks incomplete, tested some fonts on Win10:
CJK fonts may not have same weight value compared to Latin fonts |
anntzer commentedNov 14, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The CSS font weight names are mentioned inhttps://www.w3.org/TR/css-fonts-4/#font-weight-prophttps://developer.mozilla.org/en-US/docs/Web/CSS/font-weight#Common_weight_name_mapping so the intermediate values shouldnot be included, Windows likely just maps them to the closest name. It actually looks like our weights name table is wrong... The full table in the CSS spec is 100 thin (aka hairline in MozDev) plus the vendor-specific ones fontconfig (https://gitlab.freedesktop.org/fontconfig/fontconfig/blob/master/src/fcweight.c) MozDev, and Windows (https://docs.microsoft.com/en-us/dotnet/api/system.windows.fontweights?view=netframework-4.8#remarks) |
Are those font weight inconsistencies handled by#16203? Where would this PR stand after that one? |
Users can still request e.g. Foobar Thin font and if that doesn't exist but Foobar Regular does then we will return that, and I think it still makes sense to warn in that case? |
I'm pushing this to 3.4 as there seems to be some discussion about when we should warn (on a non-perfect match or any miss-match). |
PR Summary
#15529
We check if the chosen font is +-100 from the desired user outcome. If so, we log a warning documenting the desired and the actual used.
Test is added to make sure new function for string mappings are correct.
PR Checklist