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

class of sub elements are lost #363

Closed
typemytype opened this issue Feb 19, 2020 · 8 comments
Closed

class of sub elements are lost #363

typemytype opened this issue Feb 19, 2020 · 8 comments

Comments

@typemytype
Copy link
Contributor

defcon has a handy to provide classes for sub elements, see fe the init of a Font class

ufo2ft makes a copy but uses a glyph class without any reference of sub element classes. This is wrong. As parent object could depend on custom method.

https://github.com/googlefonts/ufo2ft/blob/master/Lib/ufo2ft/util.py#L99

I would advice to use a simple defcon.Glyph while copying instead of the g.__class__ or use all the sub elements while init the glyph object like:

g.__class__

def newGlyph(name):
    g = cls(
        contourClass=g.contourClass, 
        pointClass =g. pointClass, 
        componentClass = g.componentClass,
        anchorClass = g.anchorClass,
        guidelineClass = g.guidelineClass,
        libClass = g.libClass,
        imageClass = g.imageClass
    )
    g.name = name
    return g
@typemytype
Copy link
Contributor Author

second thought: I guess there is no reason to keep the original class, a copy must maybe just be a simple defcon.Glyph object

def newGlyph(name)
    g = defcon.Glyph()
    g.name = name
    return g

and third thought: Its maybe not a good plan to make the api between the ufoLib2 and defcon so different. According the spec a glyph name is not required, so on init of a glyph object the name should not be a requirement. But ufoLib2 makes it a required argument, I think this is wrong... and makes packages like ufo2ft complex where it should not be...

@anthrotype
Copy link
Member

anthrotype commented Feb 19, 2020

defcon has a handy to provide classes for sub elements, see fe the init of a Font class

hm, I would think that such sub-elements custom classes should rather be attributes of a class, instead of a particular instance.

Could you maybe add a method to the defcon Glyph object that returns a callable that already embeds all the custom sub-element classes (using functools.partial to store them in the callable).

That way we can simply get that (instead of getting glyph.__class__) and initialize that for the new glyph.

According the spec a glyph name is not required, so on init of a glyph object the name should not be a requirement

How can a glyph without a name exist, it feels odd to me. If anything it must have a name as soon as it is inserted into a layer (as they name is its key). I'll think about that a bit.

@anthrotype
Copy link
Member

anthrotype commented Feb 19, 2020

I mean something like this

def getCustomClass(self):
    return functools.partial(self.__class__, contourClass=self.contourClass, pointClass=self.pointClass, ...)

I guess actually we can do that ourselves without needing a change on defcon side.

@anthrotype
Copy link
Member

Its maybe not a good plan to make the api between the ufoLib2 and defcon so different.

yeah, I'm also interested in improving that (see fonttools/ufoLib2#41).

@anthrotype
Copy link
Member

I guess there is no reason to keep the original class, a copy must maybe just be a simple defcon.Glyph object

Sorry but how is glyph.__class__ any different from defcon.Glyph()? Why would the latter fix the problem of referencing the sub-element classes while the former does not? I'm confused..

@typemytype
Copy link
Contributor Author

defcon has fontOrLayer.instantiateGlyphObject() which returns a proper glyph object with all the correct reference set.

a correction and improved patch:

def newGlyph(name):
    g = g.layer.instantiateGlyphObject()
    g.name = name
    return g

hm, I would think that such sub-elements custom classes should rather be attributes of a class, instead of a particular instance.

and when references are class attributes its not flexible enough... (not the issue here)

Sorry but how is glyph.class any different from defcon.Glyph()?

its missing the sub element class references, where glyph.__class__ requires the sub elements to be classes from the set sub element classes. This is how defcon is setup and how all sub elements are created.

@madig
Copy link
Collaborator

madig commented Feb 20, 2020

Hm. The code snippet won't easily work for ufoLib2, because its Glyph objects keep no reference to their parent layer. Renaming it should invoke layer.renameGlyph instead of setting the name attr directly.

@anthrotype
Copy link
Member

anthrotype commented Feb 20, 2020

Let's at least use that method in ufo2ft when using defcon backend

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

No branches or pull requests

3 participants