features: Do not include empty labels #970
Conversation
|
but why are there feature labels with empty string value? how are those created to begin with? Can the user just clean up their source files to make sure these empty labels are removed? |
Lib/glyphsLib/builder/features.py
Outdated
| @@ -138,7 +138,11 @@ def _to_ufo_features( | |||
| # Replace special chars backslash and doublequote for AFDKO syntax | |||
| name = name.replace("\\", r"\005c").replace('"', r"\0022") | |||
| feature_names = ["featureNames {", f' name "{name}";', "};"] | |||
| elif font.format_version == 3 and feature.labels: | |||
| elif ( | |||
| font.format_version == 3 | |||
There was a problem hiding this comment.
do we need to check the format_version? if there are any feature.labels we just use them..
There was a problem hiding this comment.
I don't think it's possible to have feature labels in v2 sources. Anyways, this part of the condition was there beforehand.
Lib/glyphsLib/builder/features.py
Outdated
| elif ( | ||
| font.format_version == 3 | ||
| and feature.labels | ||
| and all(l["value"] != "" for l in feature.labels) |
There was a problem hiding this comment.
maybe you meant any(l["value"] != "" for l in feature.labels)?
I think we want to skip the whole featureNames section if all the labels are empty (or add one if any is non empty)
Also below in the for label in feature.labels: loop, I think you want to do continue if the name == ""
There was a problem hiding this comment.
I've decided to drop any or all, far easier to just include them if they get appended to the feature_names list.
When the user adds a stylistic set feature, you get the UI in the above pic which includes an empty text box.
It doesn't look like it. If you want a stylistic set, the text box is there and you cannot remove it. |
|
it would be good to have a test case for this. That can ensure that my Glyphs3 branch will not regress on this. |
👍 /cc @m4rc1e |
|
I tested it on Pacifico: The fail disappeared, with only have |
In Glyphsapp, users can define names for Stylistic Sets.
Currently, if a user doesn't add a name, GlyphsLib will export the stylistic set with an empty name.
This PR won't add empty names which mimics the behaviour of GlyphsApp's exporter.
I discovered this issue while checking google/fonts#7104. Fontbakery is reporting that the font is missing nameIDs 256, 257 etc which stem from this issue, fonttools/fontbakery#3671 (comment)