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

Skip dangling layers and layers without a name on UFO building #291

Merged
merged 1 commit into from
Dec 2, 2017
Merged

Skip dangling layers and layers without a name on UFO building #291

merged 1 commit into from
Dec 2, 2017

Conversation

madig
Copy link
Collaborator

@madig madig commented Dec 1, 2017

This change detects and skips layers that are not associated with an
existing master and layers without a name. The former are a sort of data
corruption. Empty layer names are an error as per the UFO specification
and choke ufoLib. Both may happen when you copy glyphs from a different
font in Glyphs, Glyphs simply ignores them.

All invalid layers now get a warning printed and skipped. Before, they
could have been silently ignored.

Add tests for both types of bogus layers.

This change detects and skips layers that are not associated with an
existing master and layers without a name. The former are a sort of data
corruption. Empty layer names are an error as per the UFO specification
and choke ufoLib. Both may happen when you copy glyphs from a different
font in Glyphs, Glyphs simply ignores them.

All invalid layers now get a warning printed and skipped. Before, they
could have been silently ignored.

Add tests for both types of bogus layers.
@schriftgestalt
Copy link
Collaborator

I just made some tests. If you open a .glyphs file in Glyphs and there is no associatedMasterId, then the masterID from the first master is used.
Empty names don't do anything, those layers are just keep around.

@madig
Copy link
Collaborator Author

madig commented Dec 1, 2017

(This is a new PR as I messed up the old one: #283 -- see there for context)

@anthrotype
Copy link
Member

anthrotype commented Dec 1, 2017

Empty names don't do anything, those layers are just keep around.

so if Glyphs.app keeps them, maybe we should also keep them when exporting to UFO? (e.g. by giving them a special __no_name__ name, maybe with a incrementing digit suffix to make them unique so they don't clash on the filesystem). Just a thought, not saying we should do this here right now`

@anthrotype
Copy link
Member

If you open a .glyphs file in Glyphs and there is no associatedMasterId, then the masterID from the first master is used

... whereas in this PR, they are dropped. shouldn't we do the same as Glyphs.app does, according to what Georg sais (i.e. implicitly associate them with first master)?

I wonder, is it possible for multiple layers to be associated with the same master? and what does that actually mean? 😕

@madig
Copy link
Collaborator Author

madig commented Dec 1, 2017

I wonder, is it possible for multiple layers to be associated with the same master?

I think so, just make multiple copies of the same master layer in Glyphs, they're all named by date by default :)

@schriftgestalt
Copy link
Collaborator

maybe with a incrementing digit suffix to make them unique so they don't clash on the filesystem

that would be a general problems. Layer names are not required to be unique.

@anthrotype
Copy link
Member

Layer names are not required to be unique

but the layers' folder names inside a UFO must be unique

@anthrotype
Copy link
Member

anthrotype commented Dec 1, 2017

so for an un-named layer in glyphs, the corresponding layer name in the UFO would be __no_name__, and the folder name that this is mapped to in the layercontents.plist could be __no_name__, __no_name__#1, etc.

@anthrotype
Copy link
Member

just for reference, this is the issue that triggered this PR
#282

I just noticed one of Noto fonts (noto-source/src/NotoSansTamil-MM.glyphs) also has one of these layers without a name. The glyphsLib < 2.0 would not choke on these, probably because it was not creating UFO3 layers but UFO2 robofont-style layers-inside-the-glyph-lib.

It's still not clear to me how these nameless layers actually happen, whether it's the user that deletes the name and leaves an empty string, or whether this is the result of some other action like pasting from a different font, like someone reported.

In any case, I believe for now it's OK if we warn and drop them from the UFO3, so I'm going to merge this.

Later we can see how to preserve them for the sake of roundtrippability.

@anthrotype anthrotype merged commit fde3213 into googlefonts:master Dec 2, 2017
@madig
Copy link
Collaborator Author

madig commented Dec 2, 2017

If empty layer names are indeed user error, I'd petition @schriftgestalt to add guards against them in Glyphs.app.

@madig madig deleted the skip-invalid-layers branch December 2, 2017 12:50
@schriftgestalt
Copy link
Collaborator

I wonder, is it possible for multiple layers to be associated with the same master? and what does that actually mean?

Yes. You can have as many layers associated with a master. There should only be one "Master" layer.

Each layer needs to know certain value that are defined in the masters. That is where the associatedMasterID is used.

@anthrotype
Copy link
Member

thanks for the explanation!

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.

3 participants