FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Font weight warning if found font sufficiently different by colt-1 · Pull Request #15615 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Font weight warning if found font sufficiently different - #15615

Closed
colt-1 wants to merge 6 commits into
matplotlib:masterfrom
colt-1:master
ConversationCommits (6)ChecksFiles changed
Closed

Font weight warning if found font sufficiently different#15615
colt-1 wants to merge 6 commits into
matplotlib:masterfrom
colt-1:master

Conversation

colt-1 commented Nov 6, 2019

Copy link
Copy Markdown

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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

tacaswell reviewed Nov 6, 2019
View reviewed changes
Comment thread doc/api/next_api_changes/behaviour.rst Outdated
tacaswell reviewed Nov 6, 2019
View reviewed changes
Comment thread lib/matplotlib/font_manager.py Outdated
break

if best_font is not None:
if abs(map_weight_name_to_score(prop.get_weight())

tacaswell Nov 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

pep8 is pretty unhappy about the formatting here.

colt-1 Nov 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! This should be better.

tacaswell added this to the v3.3.0 milestone Nov 6, 2019

tacaswell commented Nov 6, 2019

Copy link
Copy Markdown
Member

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?

tacaswell commented Nov 6, 2019

Copy link
Copy Markdown
Member

Thanks @colathro ! It was great working with you today!

anntzer commented Nov 6, 2019

Copy link
Copy Markdown
Contributor

I think we should warn anytime there's no exact match, rather than with a threshold of 100.

anntzer reviewed Nov 14, 2019
View reviewed changes
Comment thread lib/matplotlib/font_manager.py
'black': 900,
}

def map_weight_name_to_score(weight):

anntzer Nov 14, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this should be private

colt-1 Nov 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

When you say private, I think of other language access modifiers. In Python would that be just making this a class method?

anntzer Nov 14, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just prepend an underscore to its name.

colt-1 commented Nov 14, 2019

Copy link
Copy Markdown
Author

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?

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 commented Nov 14, 2019

Copy link
Copy Markdown

The font weight dict looks incomplete, tested some fonts on Win10:

font weight weight remarks
Source Code Pro ExtraLight 200 missing
Segoe UI Light 300 normal
Segoe UI Semilight 350 missing
(Simplified Chinese) Microsoft YaHei Light 290 CJK
(Simplified Chinese) Microsoft YaHei UI Light 290 CJK
(Traditional Chinese) Microsoft JhengHei Light 290 CJK
(Traditional Chinese) Microsoft JhengHei UI Light 290 CJK

CJK fonts may not have same weight value compared to Latin fonts

anntzer commented Nov 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

The CSS font weight names are mentioned in https://www.w3.org/TR/css-fonts-4/#font-weight-prop https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight#Common_weight_name_mapping so the intermediate values should not 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)
200 extralight/ultralight (we currently map ultralight to 100)
300 light (we currently map light to 200)
400 normal (aka regular in MozDev, fontconfig, Windows)
500 medium
600 semibold/demibold
700 bold
800 extrabold/ultrabold
900 black/heavy (we currently map heavy to 800)

plus the vendor-specific ones

fontconfig (https://gitlab.freedesktop.org/fontconfig/fontconfig/blob/master/src/fcweight.c)
350 demilight
380 book (we currently map book to 400)
1000 extrablack

MozDev, and Windows (https://docs.microsoft.com/en-us/dotnet/api/system.windows.fontweights?view=netframework-4.8#remarks)
950 extrablack/ultrablack

QuLogic commented May 5, 2020

Copy link
Copy Markdown
Member

Are those font weight inconsistencies handled by #16203? Where would this PR stand after that one?

anntzer commented May 5, 2020

Copy link
Copy Markdown
Contributor

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?

tacaswell modified the milestones: v3.3.0, v3.4.0 Jun 2, 2020

tacaswell commented Jun 2, 2020

Copy link
Copy Markdown
Member

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).

jklymak marked this pull request as draft September 10, 2020 15:41
QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 23, 2021
tacaswell deleted the branch matplotlib:master October 20, 2021 19:53
tacaswell closed this Oct 20, 2021
QuLogic removed this from the v3.6.0 milestone Sep 9, 2022
QuLogic mentioned this pull request Jul 8, 2025
Merged
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments
anntzer anntzer left review comments

Assignees

No one assigned

Labels

mentored: hackathon status: needs rebase status: orphaned PR topic: text/fonts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL