Repository navigation
Log a warning if selected font weight differs from requested - #30272
dstansby merged 1 commit into
Conversation
Sorry, something went wrong.
| X11FontDirectories: list[str] | ||
| OSXFontDirectories: list[str] | ||
|
|
||
| def _normalize_weight(weight: str | Number) -> Number: ... |
There was a problem hiding this comment.
Typical font weights are multiples of 100. Can / should this be any Number or can we specialize to int?
| def _normalize_weight(weight: str | Number) -> Number: ... | |
| def _normalize_weight(weight: str | int) -> int: ... |
If we want a general Number, there should be a test case for it.
Sorry, something went wrong.
There was a problem hiding this comment.
Looking at the OpenType spec, it does appear to be integral only: https://learn.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass
Sorry, something went wrong.
There was a problem hiding this comment.
Also, I think the original PR just used Number because it was already imported in font_manager.py.
Sorry, something went wrong.
| def test_map_weights(): | ||
| assert _normalize_weight('ultralight') == 100 |
There was a problem hiding this comment.
Let's reframe this to test the helper instead of the mapping, so that we can also test the passthrough behavior
| def test_map_weights(): | |
| assert _normalize_weight('ultralight') == 100 | |
| def test_normalize_weight(): | |
| assert _normalize_weight(300) == 300 # passthough | |
| assert _normalize_weight('ultralight') == 100 |
Maybe also add an error case e.g. "italic"
Sorry, something went wrong.
| if best_font is not None: | ||
| if (_normalize_weight(prop.get_weight()) != | ||
| _normalize_weight(best_font.weight)): |
There was a problem hiding this comment.
Why are these nested ifs instead of if A and B ?
Sorry, something went wrong.
dstansby
left a comment
There was a problem hiding this comment.
One minor suggestion to improve the changelog - take it or leave it, and feel free to self merge.
Sorry, something went wrong.
| ``font_manager.findfont`` logs if selected font weight does not match requested | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
| ``font_manager.findfont`` logs if selected font weight does not match requested | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| ``font_manager.findfont`` warns if selected the font weight does not match requested | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
Sorry, something went wrong.
There was a problem hiding this comment.
No, it's not a warning.
Sorry, something went wrong.
| def test_font_match_warning(caplog): | ||
| findfont(FontProperties(family=["DejaVu Sans"], weight=750)) | ||
| logs = [rec.message for rec in caplog.records] | ||
| assert 'findfont: Failed to find font weight 750, now using 700.' in logs |
There was a problem hiding this comment.
Just for my own interest, is there a reason to check the logs, instead of using a pytest.warns context?
Sorry, something went wrong.
There was a problem hiding this comment.
Same reason as above.
Sorry, something went wrong.
PR summary
This is a rebase of the orphaned #15615, but with @anntzer's comments handled.
PR checklist