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

Revert "Merge pull request #1038 from googlefonts/space-glyphOrder" #1044

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

anthrotype
Copy link
Member

This reverts PR #1038

This reverts commit 050ef62, reversing
changes made to 2c6181a.
@anthrotype
Copy link
Member Author

Until we understand how this is exactly supposed to work I suggest we revert #1038

When in fontc_crater (a tool we use to compare fontc vs fontmake output against all of googlefonts library) we bumped glyphsLib to 6.9.3, immediately hundreds of fonts that were identical before started showing diffs in the "GlyphOrder" ttx pseudo-table, with the space shifted to second position. I don't think one would expect to see this in a patch version bump.

@khaledhosny
Copy link
Collaborator

fontc_crater could normalize glyphOrder if it does not wish to follow glyphsLib here. I don’t think that is a reason to revert this change. And even if we learnt more about how this is supposed to work, it will still be a change compared to fontc.

@khaledhosny
Copy link
Collaborator

On a second thought, it does not really matter that much to me, so feel free to merge this PR.

@anthrotype
Copy link
Member Author

I mentioned fontc_crater merely because it compares hundreds of sources and is therefore sensible to changes such as this. I presume other fontmake users will be surprised to see their glyphOrder suddenly change under their feet.

@khaledhosny
Copy link
Collaborator

You probably should revert the ufo2ft commit as well.

@anthrotype
Copy link
Member Author

ufo2ft would only put space after .notdef if no public.glyphOrder is defined, which is a clear signal the font developer doesn't care about a specific ordering and the compiler can decide on their behalf.

You did ask if anyone had any objections and I too hastily said I didn't, so I'll take the blame :)

@anthrotype anthrotype merged commit 3fc6177 into main Nov 7, 2024
9 of 10 checks passed
@anthrotype anthrotype deleted the revert-1038 branch November 7, 2024 13:39
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.

2 participants