Update to score_family in font_manager.py #4689
Conversation
This PR fixes the score calculation for the case of matching a generic font name given in the `families` list. The returned score is now constrained by the position `i` of the match in the list. If the font being scored (`family2`) is the first choice for that generic name in rcParams, it will receive the same score as if it were an exact match for that position in the `families` list, namely `i / len(families)`. If the font being scored is in the alias list but not the first choice, the returned score will be greater than `i / len(families)` but less than `(i + 1) / len(families)`, ensuring that the preferred order of fonts listed in both `families` and rcParams font alias list are respected.
|
attn @mdboom |
| @@ -1117,12 +1118,11 @@ def score_family(self, families, family2): | |||
| options = [x.lower() for x in options] | |||
| if family2 in options: | |||
| idx = options.index(family2) | |||
| return ((0.1 * (idx / len(options))) * | |||
| ((i + 1) / len(families))) | |||
| return (i + idx / len(options)) * step | |||
There was a problem hiding this comment.
Can you add some () here to be explicit about the grouping intended?
|
|
||
| No match will return 1.0. | ||
| """ | ||
| if not isinstance(families, (list, tuple)): | ||
| families = [families] | ||
| family2 = family2.lower() | ||
| step = 1 / len(families) |
There was a problem hiding this comment.
I think we need a special case here for len(families) == 0. Unlikely, but technically possible.
There was a problem hiding this comment.
In which case, it should just return 1 immediately.
There was a problem hiding this comment.
Agreed, although it never checked before. Not a bad idea tho to be safe.
|
I think it would be helpful to evaluate this if you could provide some examples where it fails now and this PR fixes it. I certainly can see one case where there is a regression (commented above) where an alias match could trump an exact match. But I'd be curious to see what this solves and we can go from there. |
|
Here's one case where it fails currently: This will return 0 if 'bitstream vera sans' appears first in the 'sans-serif' alias list, and 'myfont' will never be found if it happens to appear later in the list of known fonts. |
|
Thanks. I think I get that now. This may trip up some who are relying on broken behavior -- so I'm not certain if we treat this as a simple bugfix (which it definitely is) or include it in the next major release as a change in behavior. @tacaswell: thoughts? But I'm 👍 from me on merging in general. |
|
|
||
| No match will return 1.0. | ||
| """ | ||
| if not isinstance(families, (list, tuple)): | ||
| families = [families] | ||
| elif len(families) == 0: |
There was a problem hiding this comment.
Now checking for empty families list here. The unchecked division by len(families) used to be inside the loop, where it was not reachable if len(families) == 0. Thanks for catching that!
Update to score_family in font_manager.py
This PR fixes the score calculation for the case of matching a generic font name given in the
familieslist.The returned score is now constrained by the position
iof the match in the list. If the font being scored (family2) is the first choice for that generic name in rcParams, it will receive the same score as if it were an exact match for that position in thefamilieslist, namelyi / len(families). If the font being scored is in the alias list but not the first choice, the returned score will be greater thani / len(families)but less than(i + 1) / len(families), ensuring that the preferred order of fonts listed in bothfamiliesand rcParams font alias list are respected.