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

[builder] Reorder .notdef and space glyphs in public.glyphOrder #1038

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

khaledhosny
Copy link
Collaborator

Unless "Keep GlyphOrder" (or the older "TrueType Keep GlyphOrder") custom parameter is set to True.

Unless "Keep GlyphOrder" (or the older "TrueType Keep GlyphOrder")
custom parameter is set to True.
@khaledhosny
Copy link
Collaborator Author

The regression tests failure is expected, it is all re-ordered .notdef and space.

@khaledhosny
Copy link
Collaborator Author

Any objection, should I merge this PR?

@anthrotype
Copy link
Member

no objection from me, I just didn't have the time to review, but feel free to merge without me

@khaledhosny khaledhosny merged commit 050ef62 into main Nov 1, 2024
9 of 10 checks passed
@khaledhosny khaledhosny deleted the space-glyphOrder branch November 1, 2024 21:58
@anthrotype
Copy link
Member

anthrotype commented Nov 5, 2024

@khaledhosny I still cannot even add this "Keep GlyphOrder" custom parameter, it's not shown in the list when try to add one.. Is one supposed to just add a seemingly unsupported parameter with that name? And what value? The string "1"?

Maybe we rushed a bit in adding support for this in glyphsLib if even the latest beta 3.3 doesn't have it.

@khaledhosny
Copy link
Collaborator Author

Any value that glyphsLib considers True should work, or alternatively you can use “TrueType Keep GlyphOrder” which current version of Glyphs support (It does not show up when you type the name, but if you add it anyway, Glyphs recognizes it as a boolean custom parameter).

@anthrotype
Copy link
Member

they really don't want you to keep the glyph order 😅

@khaledhosny
Copy link
Collaborator Author

Apparently

@schriftgestalt
Copy link
Collaborator

The two "Keep Order" parameters are not in the list but can be still added by typing in the search file and click Add (They are kind of private, I really like to keep people from adding them without a very good reason).
The version from yesterday will display a checkbox instead of the text field.

@anthrotype
Copy link
Member

Don't you also find a bit weird that now "space" will take the second position even when the gid1 was specified to be the old "CR" glyph and the font's glyphOrder custom parameter is following the old TrueType convention about the first-four glyphs (.notdef, CR, NULL, space)?
Note that CR is also an empty glyph and would work just fine to prevent the old bug in Windows with COLRv0 fonts that prompted this whole space-glyph-must-be-gid1 business.

I really think this is too much hand-holding. If a font developer has bothered setting a glyphOrder custom parameter (which is already optional), why not just simply obeying that?...

$ diff -u /Users/clupo/oss/fontc/build/default/fontc.GlyphOrder.ttx /Users/clupo/oss/fontc/build/default/fontmake.GlyphOrder.ttx
--- /Users/clupo/oss/fontc/build/default/fontc.GlyphOrder.ttx	2024-11-07 12:44:21
+++ /Users/clupo/oss/fontc/build/default/fontmake.GlyphOrder.ttx	2024-11-07 12:44:21
@@ -1,9 +1,9 @@
 <GlyphOrder>
     <!-- The 'id' attribute is only for humans; it is ignored when parsed. -->
     <GlyphID id="0" name=".notdef"/>
-    <GlyphID id="1" name="CR"/>
-    <GlyphID id="2" name="NULL"/>
-    <GlyphID id="3" name="space"/>
+    <GlyphID id="1" name="space"/>
+    <GlyphID id="2" name="CR"/>
+    <GlyphID id="3" name="NULL"/>
     <GlyphID id="4" name="nbspace"/>
     <GlyphID id="5" name="A"/>
     <GlyphID id="6" name="Aacute"/>
\ No newline at end of file

I'm reaaaaly tempted to just revert this. I really don't like this.

@anthrotype
Copy link
Member

actually this particular font (https://github.com/googlefonts/praise-script/blob/master/sources/Praise-Pro.glyphs) actually has them in the "wrong" order, NULL should be second and CR third (not the other way around) according to the old Apple Truetype spec, but it doesn't matter. NULL also doesn't have any contours and would be fine to have a second glyph as far as that Windows COLRv0 bug is concerned.

@anthrotype
Copy link
Member

anthrotype commented Nov 7, 2024

wait.. I just tried to export a TTF from https://github.com/googlefonts/praise-script/blob/master/sources/Praise-Pro.glyphs directly from Glyphs.app (3.3 3325, the latest) and it does NOT reorder the space glyph in this particular case.

it has the following glyphOrder custom parameter, and it does not have that Keep GlyphOrder paramter for it's too new:

image

so glyphsLib is reordering the space, but Glyphs.app isn't.

$ ttx -o - -t GlyphOrder /Users/clupo/Downloads/Praise-Regular.ttf | less
Dumping "/Users/clupo/Downloads/Praise-Regular.ttf" to "<stdout>"...
Dumping 'GlyphOrder' table...
<?xml version="1.0" encoding="UTF-8"?>
<ttFont sfntVersion="\x00\x01\x00\x00" ttLibVersion="4.54">

  <GlyphOrder>
    <!-- The 'id' attribute is only for humans; it is ignored when parsed. -->
    <GlyphID id="0" name=".notdef"/>
    <GlyphID id="1" name="NULL"/>
    <GlyphID id="2" name="nonmarkingreturn"/>
    <GlyphID id="3" name="space"/>
    <GlyphID id="4" name="uni00A0"/>
    <GlyphID id="5" name="A"/>

@schriftgestalt is there some extra logic that we're missing? Does Glyphs.app only attempts to place space glyph as second when the glyph that is already in second place is not empty?

anthrotype added a commit that referenced this pull request Nov 7, 2024
This reverts commit 050ef62, reversing
changes made to 2c6181a.
@anthrotype
Copy link
Member

the following archive contains two simple glyphs sources, one with the other wihout glyphOrder custom parameter.
When exported with Glyphs.app 3.3 3325, neither of them has 'space' placed in second position.

space-glyph-order.zip

is it the names I used or the fact the would-be second glyph is empty, I don't know.. But this example already produces a different output from glyphsLib 6.9.3, and I think a revert is in order here.

anthrotype added a commit that referenced this pull request Nov 7, 2024
Revert "Merge pull request #1038 from googlefonts/space-glyphOrder"
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.

None yet

3 participants