-
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
More alternate layer fixes #839
Conversation
@@ -2261,8 +2261,8 @@ def test_load_kerning_bracket(ufo_module): | |||
|
|||
ds = glyphsLib.to_designspace(font, minimize_glyphs_diffs=True) | |||
bracketed_groups = { | |||
"public.kern2.foo": ["a", "a.BRACKET.wght_300_1000"], | |||
"public.kern1.foo": ["x", "x.BRACKET.wght_300_1000", "x.BRACKET.wght_600_1000"], | |||
"public.kern2.foo": ["a", "a.BRACKET.varAlt01"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to keep both "BRACKET" and "varAlt" in the same name? Aren't the two labels supposed to convey the same meaning?
how about either just "BRACKET.01" or "varAlt01"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do lose a lot of the information that was previously baked in the (arguably quite long) glyph name before.
I wonder what order does this varAlt01, 02, 03, naming etc. follows? Is it deterministic at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to keep both, but I went this way because I think we want to keep something which is unmistakably bracket-related, for the various regex-based tests to work; and I went with varAlt01
because that's what Glyphs adds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if varAlt01 is what Glyphs.app uses, I'd say let's go for that one only, and fix related tests.
In what order are the digit suffix in varAlt01, 02, 03 assigned and incremented, in what order? Is it "don't ask too complicated doesn't actually matter" kind of order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, it depends on stably ordered stuff like glyph order, layer order and order of components within each layer, so at least it's deterministic, it just as eloquent as the previous naming scheme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest I just skimmed this one, I don't think I can do a thorough review. I trust Simon can fix any follow up issues that may arise, so FWIW here's my LGTM
@simoncozens if you want someone else to take a look, try ask the DaMa folks, otherwise feel free to merge, thanks
This fixes as much of #837 as I'm prepared to fix. It deals with the situation of parent glyphs which have no alternate layers but use component glyphs which do have alternate layers.
It doesn't deal with the situation of parent glyphs which have alternate layer setups which differ from those of their component glyphs, but it has a decent stab at it and warns the user. (Those are pretty rare, anyway. In the situation of Playfair 3 which had 633 "problematic" layers, only 8 of them were in this category.)
It also deals with my obviously short-sighted approach to glyph naming for alternate layers. Instead of producing long names like
_f_ydieresis.ligature.BRACKET.opsz_5_410.wdth_50_75.wght_690_900
, it now just numbers them.