robotools / robofab

RoboFab
http://robofab.org
Other
108 stars 31 forks source link

[ufo3k] Moving pens out of Robofab #52

Closed moyogo closed 7 years ago

moyogo commented 9 years ago

There’s some interest in moving pens out of Robofab, maybe to fonttools or a separate library. @adrientetar @anthrotype @behdad @brawer @graphicore @jamesgk Robofab-devs, could this happen?

jackjennings commented 8 years ago

I'll wait to hear from other package maintainers, but if there's no buy in for moving the abstract and base classes into a dependency I'm not sure that it's worth the effort to consolidate only pens that derive from them.

twardoch commented 8 years ago

Well, I see it as two steps: first create fontPens as a kind of a "plugin collection" while keeping the basic pens in fontTools but providing thin wrappers inside the fontPens package for the base pens.

At some point later, the reverse can be done: the actual implementation of the base pens can move to fontPens and the fontTools.pens subpackage can become thin wrappers.

anthrotype commented 8 years ago

No one wants to depend on a package that is not updated nor maintained. Traditionally robofab would depend on fonttools, which (being lower level) would not depend on anything else. Nobody ever complained about that. Now that robofab is dying (to be born again as fontParts), there is the problem of what to do with the pens, which is the only lingering dependency on robofab that other packages still have.

Another solution that is worth considering (and then I'll shut up! ;) is to move the pens to fontParts as well. I know it's got "fonts" in it, but where are the more general, non-font related implementations?

graphicore commented 8 years ago

The real use of the abstract classes is interface documentation. It may be enough to just link to them in the README.

The base classes, that's the most interesting stuff, because you really want to inherit from or use those. It would even make sense to make a lightweight "base pens" package with just BasePen, TransformPen, NullPen(?), PointToSegmentPen, SegmentToPointPen and a more heavy weight "pen plugin collection" as suggested by @twardoch. Then everything needed to get started with pens is in one (tiny) package. The "base pens" package is probably low effort to maintain, because it won't change much.

jackjennings commented 8 years ago

@graphicore That makes sense to me, as it sidesteps the issue of what is important enough to include in the core package to some extent. In the Ruby world, some packages have a corresponding semi-official "contrib" package for commonly used extensions, e.g. rack/rack-contrib, sinatra/sinatra-contrib.

typesupply commented 8 years ago

Let me make sure the terminology is clear:

Each of these have their merits and unique uses. The Adapter Pens environments to implement interfacing with one or the other protocol. For example, defcon is a point oriented environment so it draws to the PointPen protocol natively. The method that handles the Pen protocol imports an adapter pen, wraps the given Pen with that and passes the wrapper to the method that handles the Point Pen protocol. Easy.

I think everyone knows this. I just want to make sure it's clear.

Regarding the relationship between GLIF and Point Pen: The GLIF contour point structure follows the PointPen protocol. Point Pen actually came about because writing GLIF contour points was tricky with the Pen protocol.

In a perfect world, we would have a fontPens packages that would contain the abstract and base implementations of both the Pen and Point Pen protocols. This would allow us to have a single source for documentation about and example implementations of the protocols. That would eliminate the confusion about where the adapters should go, the issues of two dependencies and probably other stuff.

This would also eliminate another issue that I've run into. When I started implementing UFO 3 in defcon, I realized that the Pen protocol didn't allow point identifiers. That would have required a fontTools change and I was reluctant to do that. So, Point Pen -> Pen as of UFO 3 is a lossy translation. If the protocols were both owned by the same package, they could more easily be kept in sync.

The only problem with moving the Pen protocol out of fontTools is that it would create a new dependency for fontTools. If that's a problem, perhaps fontTools could just be considered an independent implementation of the protocol.

anthrotype commented 8 years ago

Thanks Tal for the detailed reply. FontTools does not depend on its own pens. The pens subpackage is quite independent from the rest of the library. If you search the codebase there are only a couple of places where a NullPen or a BoundsPen is used, but it's not crucial. So I think that is not a problem as far as I'm concerned. /cc @behdad

behdad commented 8 years ago

Yes, the pens in fontTools are mostly for user to use, not for fontTools itself. We can created yet another repo, but I believe both Just and I agree, that we might as well put them all in fonttools.

The reason I've been silent so far is because I have ideas that I want to suggest for the new penBox, but I have not had time to flesh them out yet.

In short, I like to suggest we move away from the current BasePen model that implements moveTo, etc and expects subclasses to implement _moveTo etc; to a mixin model, whereas there are sink pens that implement a few methods, and there are mixin pens that implement some meyhods and might or might not call super() in those.

Then the adaptor pens become mixins. The quad-to-cubic becomes a mixin. The track-current-point becomes a mixin. When writin a new sink pen, eg one to compute area of a glyph, if I need current point attack ng and need quadratic-to-cubic handling, I'll explicitly pull in those mixins as superclasses.

We can alao have a one line way of testing a pen against all kinds of input (None first point, etc) for a given protocol so people can be sure their pen is complete.

I also like to define a ValuePen protocol, which is for sink pens that calculate a value (glyph area, perimeter, hits, ...). They should all store the result in pen.value. This allows for writing adaptor pens for ValuePens. For example a ByContour adaptor that calculates values for each contour separately. The value pen class can also provide a reduce method as well as initial value of value.

Those are my current thoughts. As you can see, not fleshed out.

jackjennings commented 8 years ago

@behdad I actually agree that it would be interesting to reimagine what the internal API for building pens. What you're describing is a pattern that I like a lot, and leads to a really flexible Interface.

That being said, that's a far more drastic change than simply consolidating some shared functionality into a small dependency. If anything, centralizing some of the pen functionality in one place would provide a place to explore this in a safer and more considered environment before deploying a pen 2.0 interface directly into live code.

adrientetar commented 8 years ago

to a mixin model,

Is there an actual need for that? It seems a little over-engineering to me.

twardoch commented 8 years ago

If we consolidate all the basic pens into a fontPens package, then fontTools.pens can be made into a "temporary" passthrough subpackage (thin wrapper) which just does from fontPens.pens import * or something like that, so "legacy" pens don't fail. Over time, pen developers could patch their inheritance to use fontPens.pens instead of fontTools.pens or the RF pointPen. The thin "pens" warapper could live inside fontTools as a legacy thing.

fontTools proper doesn't really use pens, the core of fontTools really is about SFNT, and frankly, I cannot imagine that the decision to put the original "pens" subpackage inside fontTools was "designed" by Just very consciously. It seems Just put pens in there because at the time he started fontTools, there wasn't another good location for such a tiny package.

I think we should use the roboFab->fontParts transition to clean it up. I don't think the pens belong structurally to fontTools, ufoLib or fontParts. A separate repo is actually better. There are many situations where one wants to use a pen and doesn't want to use fontTools, fontParts or ufoLib.

For example, I use a pen to move glyphs between FLS5 and the imported fontforge package, i.e. two "native" font editing environments, without using any otherwise pythonic structures.

If we go by the argument "you don't need to create a pen by inheritance", then actually it does not matter where the basic pens are stored, so we can dump the base pen classes completely :) But I don't think this is a serious argument. Sure, you don't need to inherit, but inheritance is more pythonic and elegant, and it helps documentation and introspection.

BTW, to compile my list of pens I actually searched Github for the inheritance patterns. So it was useful even now.

twardoch commented 8 years ago

Somewhat off-topic: at the Typo Labs Berlin 2016 event (May 10-11) @jenskutilek is giving a workshop about pens: http://typotalks.com/labs/schedule/talk/?tid=35743&et=TYPO%20Labs%202016

Perhaps it'd be great if people on this thread make a decision about the future direction by that time, then Jens could incorporate a brief “future plans” statement into his workshop. Or alternatively, since a few of the interested parties will attend TYPO Labs, a brief informal discussion about this could be held there.

I don’t feel I have enough stakes in pens myself. I’ve voiced my opinion but I trust the maintainers of the relevant packages to come to an agreement about the future direction.

behdad commented 8 years ago

Typo Labs Berlin 2016

It would be the excellent venue to brainstorm the future API, as in 2.0. In the mean time, someone should just move the robofab pens somewhere else and call this issue closed.

twardoch commented 8 years ago

Seems like it started: https://github.com/robofab-developers/fontPens

typesupply commented 8 years ago

Seems like it started: https://github.com/robofab-developers/fontPens

Yeah, I started that. Still thinking about what should go into it. I'm in fontParts land right now and then I'll come back to this.

behdad commented 8 years ago

to a mixin model,

Is there an actual need for that? It seems a little over-engineering to me.

Right now the BasePen does a bunch of things, including quadratic-to-cubic conversion and tracking current point. There are more desirable things that a base pen can provide. But then we end stuffing a set of arbitrary things into a base class. Since most of these things are independent, we may as well put them in separate classes and use multiple inheritance to pull in exactly the features you need. That's exactly what mixins are.

typemytype commented 7 years ago

finally moved robofab pens to fontPens

https://github.com/robofab-developers/fontPens/commit/02ce611119e54a21c8b6f6e2b9658ad7f1b7cc9f

@anthrotype could you add those doc testing workers?

also needed to add a setup.py and update the readme, anyone?

typemytype commented 7 years ago

further discussions and issues should be posted on https://github.com/robofab-developers/fontPens/issues