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

[outlineCompiler] Make space the 2nd glyph unless its order is explicitly set #881

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

khaledhosny
Copy link
Collaborator

@khaledhosny khaledhosny commented Oct 12, 2024

@khaledhosny
Copy link
Collaborator Author

The second commit is just in case someone wants this level of control, but I have no strong opinion here and I can do without it.

@anthrotype
Copy link
Member

I would only move the space to second place if there is not already an explicit glyphOrder in the UFO.
I don't see the point of having yet another custom parameter to "Keep GlyphOrder" in this situation. Either you don't care/define your own glyphOrder, in which case we do our best to place glyphs to maximise compatibility, or you do provide an explicit one and we just do what told.

@anthrotype
Copy link
Member

as for glyphsLib, which always sets a glyphOrder in the exported master UFOs (whether implicit in the order of the glyphs in the source file, or with an explicit glyphOrder custom parameter), it can deal itself with those Glyphs-specific things. It may decide to always place the space in second place (like Glyphs does now?) regardless of what the stated glyphOrder is, unless the user uses "no, really I mean it" custom parameter..

@khaledhosny
Copy link
Collaborator Author

Makes sense. I’ll do that.

@khaledhosny khaledhosny force-pushed the space-order branch 2 times, most recently from 9b4b5b0 to 6ac2555 Compare October 15, 2024 21:03
@khaledhosny
Copy link
Collaborator Author

Done.

@khaledhosny
Copy link
Collaborator Author

This does not work. We don’t actually know if public.glyphOrder is set or not, we ask the font for glyphOrder and it gives us one (defcon at least will make up something if public.glyphOrder is not set).

@khaledhosny khaledhosny force-pushed the space-order branch 5 times, most recently from cf7cc4b to 353501d Compare October 15, 2024 22:03
@khaledhosny
Copy link
Collaborator Author

I think it is working now.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

LGTM besides minor comments

Lib/ufo2ft/util.py Outdated Show resolved Hide resolved
@khaledhosny khaledhosny changed the title [outlineCompiler] Always make space the gid2 [outlineCompiler] Make space the 2nd glyph unless public.glyphOrder is set Oct 16, 2024
Lib/ufo2ft/util.py Outdated Show resolved Hide resolved
@khaledhosny khaledhosny force-pushed the space-order branch 2 times, most recently from 014779a to bab3a73 Compare October 16, 2024 08:28
@khaledhosny khaledhosny changed the title [outlineCompiler] Make space the 2nd glyph unless public.glyphOrder is set [outlineCompiler] Make space the 2nd glyph unless its order is explicitly set Oct 16, 2024
@khaledhosny khaledhosny force-pushed the space-order branch 2 times, most recently from 8ca4101 to 5d2a388 Compare October 16, 2024 08:30
@anthrotype
Copy link
Member

much better now, thanks!

@khaledhosny khaledhosny merged commit 5bb7139 into main Oct 16, 2024
9 checks passed
@khaledhosny khaledhosny deleted the space-order branch October 16, 2024 08:44
@anthrotype
Copy link
Member

@khaledhosny where did you see that "Keep GlyphOrder" custom parameter in Glyphs.app? I can't find it in either font custom parameters or variable font export settings. I am using Glyphs.app 3.3 (3324)

@khaledhosny
Copy link
Collaborator Author

@khaledhosny where did you see that "Keep GlyphOrder" custom parameter in Glyphs.app? I can't find it in either font custom parameters or variable font export settings. I am using Glyphs.app 3.3 (3324)

https://forum.glyphsapp.com/t/custom-glyphorder-is-not-respected/31555/4

@anthrotype
Copy link
Member

but it's not there... weird

@khaledhosny
Copy link
Collaborator Author

Yes, even if you type the name it does not recognize it and just adds a generic custom parameter, I guess it didn’t make it to release.

Comment on lines +35 to +36
the first glyph (at index 0). Also, if "space" is present in the font and
missing from glyphOrder, force it to be the second glyph (at index 1).
Copy link
Member

@anthrotype anthrotype Nov 7, 2024

Choose a reason for hiding this comment

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

I'm now thinking that this doesn't check that the glyph named 'space' is actually empty before putting it at index 1, and the whole point of doing this (which isn't even mentioned here, I suppose it should) was to make sure COLRv0 fonts work in old Windows. Maybe it should just move the first empty glyph it finds to second place, whatever its name. I think that's what gftools-fix does.

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.

For color fonts, make GID 1 the .null glyph
2 participants