Skip to content
This repository has been archived by the owner on Jan 28, 2019. It is now read-only.
This repository has been archived by the owner on Jan 28, 2019. It is now read-only.

should glifLib ignore glyph "name" attribute, and use the one from contents.plist instead? #34

Open
anthrotype opened this issue Jun 6, 2016 · 5 comments

Comments

@anthrotype
Copy link
Member

anthrotype commented Jun 6, 2016

The UFO Specification says that:

When reading GLIF files, the name attribute should be ignored, since manual editing may have caused a mismatch with the glyph name as stored in contents.plist...

However, in glifLib we do use the name attribute of the glyph element, and set the glyphObject.name with it:

def _readName(glyphObject, root):
    glyphName = root.get("name")
    if not glyphName:
        raise GlifLibError("Empty glyph name in GLIF.")
    if glyphName and glyphObject is not None:
        _relaxedSetattr(glyphObject, "name", glyphName)

https://github.com/unified-font-object/ufoLib/blob/master/Lib/ufoLib/glifLib.py#L972-L977

Shouldn't it be ignored, and the glyph name from the contents.plist be used instead to avoid mismatches?

@anthrotype
Copy link
Member Author

Pity that this answer from my previous self didn't get any reply. I think this is still an issue, or a discrepancy between the wording of the UFO spec and its reference implementation (ufoLib).

@anthrotype
Copy link
Member Author

I checked again the GLIF spec for all UFO formats from 1 to 3 and they all say that name attribute should be ignored when parsing GLIF

since manual editing may have caused a mismatch with the glyph name as stored in contents.plist

I just checked the defcon code, and I can see that Layer.loadGlyph is setting the name attribute immediately before calling GlyphSet.readGlyph method.

I think in fontTools.ufoLib we should change GlyphSet.readGlyph so that it no longer reads/sets the name attribute on the glyph object.
The most conservative approach would be to only set the name attribute when it is not already being set (if it exists and it is not None), otherwise ignore it.

The reason I'm insisting on this is that, in fontTools.ufoLib, I would like to make the name attribute of Glyph objects read-only, ie. it should only be possible to set once it in the Glyph(name="hello") constructor, and then access it via a read-only 'name' property, however attempting to modify it would raise AttributeError.

The rationale is that the parent object (Layer) is the one that contains the mapping between names and respective Glyph objects; making the name attribute read/write means that each Glyph objects must maintain a (circular) reference to their parent Layer, which I really wish to avoid.
It's best if we keep these classes really simple, just containers for the data, without any magic or extra syntactic sugar.

@typesupply
Copy link
Contributor

What you proposed seems fine to me. I want to drop the name attribute from GLIF but I don't think I have consensus on that yet.

@anthrotype
Copy link
Member Author

actually, since the UFOReader uses a _relaxedSetAttr function that skips when AttributeError is raised by the object, I didn't need to change it after all.. My read only name property would raise AttributeError and the UFOReader silently continues on.
We can still change it if you want.

@typesupply
Copy link
Contributor

Maybe we should change it just for clarity in the future? Whatever you think is best. I'm forgetful so I like to leave breadcrumbs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants