Skip to content

Fix SCRIPT font family in Kiva #975

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.

Sign up for GitHub

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

Merged
corranwebster merged 2 commits into from
Aug 8, 2022
Merged

Fix SCRIPT font family in Kiva #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
864b4e0
Add a test for #971
corranwebster Aug 4, 2022
4b85db6
Fix bug and clean-up style.
corranwebster Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion kiva/fonttools/font.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class Font(object):
ROMAN: "serif",
MODERN: "sans-serif",
DECORATIVE: "fantasy",
SCRIPT: "script",
SCRIPT: "cursive",
TELETYPE: "monospace",
}

Expand Down
22 changes: 20 additions & 2 deletions kiva/fonttools/tests/test_font.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
import unittest

from kiva.constants import (
BOLD, BOLD_ITALIC, DEFAULT, ITALIC, MODERN, NORMAL, ROMAN, WEIGHT_BOLD,
WEIGHT_LIGHT, WEIGHT_NORMAL, SWISS,
BOLD, BOLD_ITALIC, DECORATIVE, DEFAULT, ITALIC, MODERN, NORMAL, ROMAN,
SCRIPT, TELETYPE, WEIGHT_BOLD, WEIGHT_LIGHT, WEIGHT_NORMAL, SWISS,
)
from kiva.fonttools._constants import font_family_aliases, preferred_fonts
from kiva.fonttools.tests._testing import patch_global_font_manager
from kiva.fonttools.font import (
DECORATIONS, FAMILIES, NOISE, STYLES, WEIGHTS, Font, str_to_font,
Expand Down Expand Up @@ -193,6 +194,23 @@ def test_font_query_warnings(self):
self.assertEqual(query.get_weight(), WEIGHT_LIGHT)
self.assertEqual(query.get_style(), "italic")

def test_family_queries(self):
# regression test for Enable #971
# this ensures every font family creates a valid query populated
# with query families that work
families = [
DECORATIVE, DEFAULT, MODERN, ROMAN, SCRIPT, SWISS, TELETYPE
]
for family in families:
with self.subTest(family=family):
font = Font(family=family)
query = font._make_font_query()
query_family = query.get_family()[0]

self.assertEqual(query_family, Font.familymap.get(family))
self.assertIn(query_family, font_family_aliases)
self.assertIn(query_family, preferred_fonts)
Comment on lines +211 to +212
Copy link
Copy Markdown
Member

@jwiggins jwiggins Aug 8, 2022

Choose a reason for hiding this comment

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

There are items in font_family_aliases which are not keys in preferred_fonts. Perhaps you should only check against preferred_fonts?

Copy link
Copy Markdown
Contributor Author

@corranwebster corranwebster Aug 8, 2022

Choose a reason for hiding this comment

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

We want to check it is in both, because of this line here:

enable/kiva/fonttools/_score.py

Line 38 in 4d51db2

if family1 in font_family_aliases:

If it is not on font_family_aliases then preferred_fonts will never be accessed.

Copy link
Copy Markdown
Member

@jwiggins jwiggins Aug 8, 2022

Choose a reason for hiding this comment

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

I'm saying that query_family in font_family_aliases can be True at the same time that query_family in preferred_fonts is False.

The more complete snippet from score_family...

enable/kiva/fonttools/_score.py

Lines 38 to 40 in 4d51db2

if family1 in font_family_aliases:
if family1 in {"sans", "sans serif", "modern"}:
family1 = "sans-serif"

... is not really being tested here because converting from the integer Kiva family constants via Font.familymap will never result in the values "sans", "sans serif", or "modern".

Is the point of this test to keep Font.familymap values in sync with font_family_aliases and preferred_fonts?

Copy link
Copy Markdown
Contributor Author

@corranwebster corranwebster Aug 8, 2022

Choose a reason for hiding this comment

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

The entries in familymap need to be in both, yes (and that's what I care about for this PR).

font_family_aliases (and presumably preferred_fonts) can have extra values which come from the face_name (this is what derailed #977). Eg. if someone names a font "sans" (ie. Font("sans", 12)) it will also go through the above codepath. That likely needs some re-engineering at some point, but it is outside the scope of this PR)



class TestSimpleParser(unittest.TestCase):

Expand Down