-
Notifications
You must be signed in to change notification settings - Fork 51
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
WIP: Support Glyphs3-style glyph data incl. writing direction #818
base: main
Are you sure you want to change the base?
Conversation
add .case and .direction
…uction getter/setter. This seems like a simple mistake
…ces to both names
…n error in the following loop in line 388, so it’s better to have get_glyph return "" instead of None
This is awkward, but glyphdata.py:556 relies on it to be None, so I kept it that way instead of having _get_glyph() return an unpropagated GlyphInfo object as the first return value
…into GlyphInfo3
add .case and .direction
…uction getter/setter. This seems like a simple mistake
…ces to both names
This is awkward, but glyphdata.py:556 relies on it to be None, so I kept it that way instead of having _get_glyph() return an unpropagated GlyphInfo object as the first return value
and some refactoring
d4cc7d9
to
2637a3c
Compare
I finished what I had planed to do. |
Note to self:
|
when you iterate over all names in the xml file you basically compare the .xml files. More interesting (and potentially error prone) is the info construction from ligatures and composite glyphs. |
Could you maybe share the code? Did you follow it for making this PR? |
The code in Glyphs and in glyphsLib is very different (the code in Glyphs is very old and does slightly more). But the behavior should be the same as shown by the tests. What I meant is that when you test the new code, one should focus in glyphs that are not in the glyphData and those are mostly ligatures. One thing I might need to explain: |
I'm cleaning Georg’s work up a bit. The linting test passes now. Could you please take a look at the 3.7 test? And then there's the regression check. That's whole bunch. |
the test expects None because the CustomGlyphData.xml does not contain an entry for glyph 'A'. So something has changed in this PR that makes the test fail. I think we want to keep the current behaviour as regards custom glyph data. |
My code should set the production name to "A", not the uni-name. I added a test to make sure. So the new behavior seems to come somewhere else? |
Can this be revived? The RTL<>LTR stuff is needed for g3 files round tripping and the other improvements are also good to have. |
I just created a PR from the 'GlyphInfo3' branch. @yanone @schriftgestalt what's the status of this?
Can you give this a better PR title and add a description of why/what this is about? thanks
related to #778 and googlefonts/ufo2ft#658