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

WIP: Support Glyphs3-style glyph data incl. writing direction #818

Open
wants to merge 96 commits into
base: main
Choose a base branch
from

Conversation

anthrotype
Copy link
Member

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

schriftgestalt and others added 30 commits April 20, 2022 15:51
add .case and .direction
…uction getter/setter. This seems like a simple mistake
…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
add .case and .direction
…uction getter/setter. This seems like a simple mistake
@schriftgestalt
Copy link
Collaborator

I finished what I had planed to do.

@madig
Copy link
Collaborator

madig commented Nov 10, 2022

Note to self:

  1. Iterate over all names and ensure that I get the same info from glyphsLib as from Glyphs 3
  2. Grab a bunch of .glyphs files and iterate over their names and test the same

@schriftgestalt
Copy link
Collaborator

schriftgestalt commented Nov 11, 2022

Iterate over all names and ensure that I get the same info from glyphsLib as from Glyphs 3

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.

@madig
Copy link
Collaborator

madig commented Nov 12, 2022

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?

@schriftgestalt
Copy link
Collaborator

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:
There is a difference between f_f.ss01 and f_f._ss01. The first glyph is a stylistic alternate of the f_f and the later is a ligature of f and f.ss01. So if the suffix has the same number of underscores, each substring between the underscores is the suffix for the corresponding glyph name, including empty strings.

@yanone
Copy link
Contributor

yanone commented Nov 15, 2022

@anthrotype

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? postscriptNames.get("A") returns uni0041 now instead of None which doesn't seem like a mistake to me.

And then there's the regression check. That's whole bunch.

@anthrotype
Copy link
Member Author

postscriptNames.get("A") returns uni0041 now instead of None which doesn't seem like a mistake to me.

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.

@schriftgestalt
Copy link
Collaborator

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?

@schriftgestalt
Copy link
Collaborator

Can this be revived? The RTL<>LTR stuff is needed for g3 files round tripping and the other improvements are also good to have.

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.

4 participants