Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert panoseDefault to namespace object #3201

Merged
merged 5 commits into from Jul 12, 2023

Conversation

NightFurySL2001
Copy link
Contributor

Fix #3197

@anthrotype
Copy link
Member

anthrotype commented Jul 7, 2023

I'd rather use the OS2 table module's Panose object which is used when decompiling or loading from XML. Just add an __init__ to that takes keyword-only arguments and initializes all self attributes to zeros, then fontBuilder can just use_panoseDefaults=Panose().

If one wishes to override any panose attributes when building OS/2 with fontBuilder they can pass the latter a Panose instance, and can easily construct one with the new constructor.

@anthrotype
Copy link
Member

a little test in fontBuilder_test.py would be nice too, e.g. constructing OS/2 once without overriding panose defaults, and another overriding with custom Panose instance, asserting the panose attribute is an object with expected attrs

for name in panoseValues:
assert getattr(fb.font["OS/2"].panose, name) == 0

fb.setupOS2(panoseValues)
Copy link
Member

Choose a reason for hiding this comment

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

no, I don't think setupOS2 works like that, you want to construct a panose = Panose(**panoseValues) instance beforehand, and pass that one to fb.setupOS2(panose=panose)

@@ -420,3 +420,49 @@ def test_unicodeVariationSequences(tmpdir):
fb.setupCharacterMap(cmap, uvs)
fb.save(outPath)
_verifyOutput(outPath, tables=["cmap"])


def test_setupPanose(is_ttf, keep_glyph_names, make_cff2, post_format):
Copy link
Member

Choose a reason for hiding this comment

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

you are not using these pytest fixture arguments, just define it as def test_setupPanose():

@anthrotype anthrotype merged commit 8846b57 into fonttools:main Jul 12, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fontBuilder] Default panose value is set to a dict
3 participants