Split the UFO builder into 1 file per "feature" #235

belluzj · 2017-10-17T15:52:49Z

This is a work-in-progress to implement the round-trip between Glyphs and UFO+designspace.

Proposed plan:

Split the current builder (Glyphs->UFO) into small files, one per "feature" (anchors, guidelines, kerning...)
For each feature, implement the other side (UFO->Glyphs) in the same file
Add features related to the designspace document (instances and glyph layers named with brackets https://glyphsapp.com/tutorials/alternating-glyph-shapes and other related tricks)
Add smart components, probably by instantiating each of their uses as a normal UFO components + metadata to recover the "smartness" in the other direction.

After reading the code and working on step 1, I have a few questions:

In a few places, the objects from Glyphs are "parsed"/decomposed into tuples, and then the tuples are used to fill in the UFO objects. I believe that this is a leftover from the previous builder that used raw dictionaries instead of nice objects, and that the builder should now use the objects directly, and get rid of the intermediary tuples?
It looks like the two modules "interpolation.py" and "util.py" are mostly related to the builder. Maybe they should be moved into the "builder" module? (for interpolation: I say that because the builder should be able to read/fill in a designspace document to link the produced UFOs together)
For testing and for the builder's "robustness", I see three levels of round-trip:
1. Glyphs->UFO->Glyphs without user intervention. If that works, it means that we have persisted all the info from Glyphs into the UFOs
2. Glyphs->UFO, user intervention, UFO->Glyphs, user intervention -> etc. I suppose that it is what most users of the lib will want to do? The goals are:
  - to recover as much as possible of the initial Glyphs "smartness" while still taking into account the user modifications. E.g. the user modifies a UFO component that was previously a smart component: maybe we cannot bring that one back as a smart component, but all the other untouched instances of smart components should come back as "smart" in Glyphs.
  - to keep special/custom UFO data from other UFO editing tools that will be used in the middle
  - to minimize the textual diffs both in the various versions of the .glyphs files and of the UFO files
  - to guard against the various UFOs getting out of sync, e.g. the user modifies the family name in one of the UFOs but not the others = what do we do when converting back to glyphs?
3. Random UFO from anywhere, any tool -> Glyphs. Is anyone going to want that? In that case, we won't be able to introduce any "smartness"? Also, could we encounter exotic stuff that cannot be modeled in Glyphs? Is this part of the goals of this library? Or is there already a UFO import tool in Glyphs? (maybe this third case is not very different from the second one)
I saw that some Glyphs data is stored as text in the features.fea file. In order to import it back, I guess that I should use a feature file parser. Can I use fonttools.feaLib for that?

schriftgestalt · 2017-10-17T19:42:55Z

Glyphs can read and write UFOs. So if someone has a UFO and needs to convert it to a .glyphs file, he most likely has Glyphs around and can just open the UFO.

One use/test case for the Glyphs<->UFO code could be to use it from within Glyphs to read/write UFO/designspace files. That would test the API compatibility/modularity quite well.

schriftgestalt · 2017-10-17T19:49:54Z

If you find ways to store more smart stuff in UFOs, I might use the same structure when exporting from Glyphs. I’m interested how you implement this. I’m not familiar enough with how private data is supposed to be stored in UFO to be able to connect it back to the base object. One simple problem is extra info for components, like the alignment setting. How to decide that the data needs to be used or not when the base object has changes.

anthrotype · 2017-10-18T15:39:27Z

Thanks Jany for the detailed plan you proposed.
I think the step 1. (splitting the current glyphs->ufo builder into smaller modules, and leaving the reverse to_glyphs_* functions as not-yet-implemented stubs) should be the object of an initial PR (e.g. this one), while the rest of the plan can be implemented in another PR. This way, we could merge this and release 2.0 now-ish, and then can keep working on the rest of the UFOs->glyphs builder, and aim to ship that in the following release.
There are lots of changes already on master, and I think to fully develop this ufos->glyphs builder it may take some time, and I'd like to be able to tag a release before that.

In a few places, the objects from Glyphs are "parsed"/decomposed into tuples, and then the tuples are used to fill in the UFO objects...

we can get rid of that, and use Glyphs objects directly

"interpolation.py" and "util.py" are mostly related to the builder... should be moved into the "builder"

ok

the user modifies the family name in one of the UFOs but not the others = what do we do when converting back

I guess it's ok to nicely fail in these cases

exotic stuff... random stuff that cannot be modeled in Glyphs

We do what we can do. Let's start with the easy part and see how far we get :)

Thanks!

belluzj · 2017-10-18T16:37:51Z

Thanks for your feedback. I pushed my latest code even though it has conflicts and probably does not run, just to show that I have further split the builder.

Tomorrow I will cleanup this PR so that the only diffs are the existing code moving around and the stub functions, and I will submit the new code for UFO->Glyphs in another PR.

moyogo · 2017-10-18T20:04:31Z

Sounds good!

belluzj · 2017-10-19T17:08:43Z

@anthrotype @moyogo I have moved the new code in another PR, this one is ready to be reviewed

anthrotype

I think True and 1 are equal in 🐍.

belluzj · 2017-10-20T17:15:38Z

Ugh, I need to un-learn Ruby

belluzj · 2017-10-20T17:19:35Z

I'm glad you pointed that out, because the test I just added was really moot

anthrotype · 2017-10-22T12:37:56Z

Lib/glyphsLib/builder/__init__.py

+
+# Existing
+from .kerning import add_glyph_to_groups, add_groups_to_ufo, load_kerning
+from .anchors import propagate_font_anchors, propagate_glyph_anchors


propagate_glyph_anchors is imported but not used here. I think it's a private function only used by propagate_font_anchros.

anthrotype · 2017-10-22T12:41:40Z

Lib/glyphsLib/builder/__init__.py

+    if varfont_origin:
+        instance_data[varfont_origin_key] = varfont_origin
+    if debug:
+        return clear_data(font)


I think we need to remove this debug argument as well as this clear_data function.
They no longer work with the class-based interface, as clear_data expects a dict or a list from which values are popped and the ones that are left are thus "unused".

anthrotype · 2017-10-22T12:42:46Z

Lib/glyphsLib/builder/anchors.py

+    for anchor in anchors:
+        x, y = anchor.position
+        anchor_dict = {'name': anchor.name, 'x': x, 'y': y}
+        glyph.appendAnchor(glyph.anchorClass(anchorDict=anchor_dict))


I'm pretty sure defcon's appendAnchor also accepts a raw dict directly. It will automatically instantiate it to the respective anchor class for you.

anthrotype · 2017-10-22T12:45:23Z

Lib/glyphsLib/builder/blue_values.py

+                        unicode_literals)
+
+
+def to_ufo_blue_values(_context, ufo, master):


why _context with an underscore? is it because it is not used in this function? I think it's fine to always call it context.

anthrotype · 2017-10-22T12:47:47Z

for the merge conflict, I think it should be fine to git rm Lib/glyphsLib/builder/ufo.py file, as the only change occurred with #249 was to cast appVersion to int, which was already the case in this PR.

anthrotype · 2017-10-22T15:27:40Z

Lib/glyphsLib/builder/context.py

+        self.defcon = defcon
+
+        self.ufos = OrderedDict()
+        """


any reasons why here you use """ multi-line string literals """ instead of simple # inline comment?

Because I read in a PEP that it was a way to document class attributes: https://www.python.org/dev/peps/pep-0257/#what-is-a-docstring

String literals occurring immediately after a simple assignment at the top level of a module, class, or init method are called "attribute docstrings".

Is there another way?

Interesting, I didn't know that...
However, if you try to do help(...) on that class, it only shows the first multi-line string.

maybe auto docs generators like sphinx will be able to retrieve those. I'm fine if we leave them. We should polish the documentation at some point.

anthrotype · 2017-10-22T15:31:55Z

Lib/glyphsLib/builder/custom_params.py

+from glyphsLib.util import bin_to_int_list
+from .filters import parse_glyphs_filter
+from .constants import GLYPHS_PREFIX, PUBLIC_PREFIX, CODEPAGE_RANGES, \
+    UFO2FT_FILTERS_KEY


let's stick to either parentheses or backslashes for multi-line import statements. I prefer the former

anthrotype · 2017-10-22T15:39:25Z

Lib/glyphsLib/builder/guidelines.py

+from __future__ import (print_function, division, absolute_import,
+                        unicode_literals)
+
+from glyphsLib.types import point


point is imported but unused

I will remove it.

It's here because it was used in the WIP implementation of to_glyphs_guidelines that I replaced with pass. Is it still the plan to have this MR merged quickly with stub functions for everything UFO -> Glyphs?

Is it still the plan to have this MR merged quickly with stub functions for everything UFO -> Glyphs?

yes!

anthrotype · 2017-10-22T16:17:36Z

Lib/glyphsLib/builder/context.py

+    """Builder context for Glyphs to UFO + designspace."""
+
+    def __init__(self, font, defcon):
+        """Create a context for the Glyphs to UFO + designspace builder.


I was thinking, instead of passing around a "context" object as argument to the to_ufo_* and to_glyphs_* functions, we could define two classes e.g. UFOBuilder and GlyphsBuilder that have those functions as instance methods and use self as the context.

We could still keep this modularized structure with the builder functions in their respective modules, and then when we define the builder classes, we would set these functions as class attributes, and they will be bound to a builder instance just like regular methods.

WDYT?

Using classes makes sense.

Yes, I will do that

belluzj · 2017-10-23T13:23:08Z

I fixed all the comments and rebased onto master.

moyogo · 2017-10-23T13:35:05Z

Lib/glyphsLib/builder/builders.py

+class UFOBuilder(object):
+    """Builder for Glyphs to UFO + designspace."""
+
+    def __init__(self, font, defcon, family_name=None, propagate_anchors=True):


Maybe ufo_module instead of defcon. And default to defcon when None.

moyogo · 2017-10-23T13:43:14Z

Lib/glyphsLib/builder/builders.py

+
+
+    @property
+    def master_ufos(self):


Just masters instead of master_ufos, then it will be the same as instances and designspace.

anthrotype · 2017-10-23T14:07:13Z

Lib/glyphsLib/builder/builders.py

+
+
+    # Implementation is spit into one file per feature
+    from .anchors import to_ufo_propagate_font_anchors, to_ufo_glyph_anchors


I was also thinking.. perhaps instead of naming the methods UFOBuilder.to_ufo_*, and GlyphsBuilder.to_glyphs_*, which seems a bit redundant, we could replace the to_ufo_* and to_glyphs_* prefixes with a generic build_*, using the as keyword in the import statements.
E.g. inside UFOBuilder scope, you would do from .anchors import to_ufo_glyph_anchors as build_glyph_anchors, whereas inside GlyphsBuilder, from .anchors import to_glyphs_glyph_anchors as build_glyph_anchors.
WDYT?

I don't mind the redundant naming because I think it makes things very clear inside the code: those names will only be used inside the implementations of the builders, with self. in front of it, and if you're looking at a file and stumble upon self.build_anchors, you will have to look around to find out in which direction the current function is going, whereas here it will always be very clear. Also having two names for the same function sounds confusing.

ok, let's keep it like that

moyogo · 2017-10-23T14:09:59Z

Lib/glyphsLib/builder/__init__.py

+    return result
+
+
+def to_glyphs(ufos, designspace=None, classes=classes):


glyphs_module instead of classes would be clearer, like ufo_module.

anthrotype

LGTM, thanks!

belluzj force-pushed the ufo_roundtrip branch from a99c9dc to a33eb16 Compare October 18, 2017 16:32

belluzj force-pushed the ufo_roundtrip branch 2 times, most recently from 9cbfb9c to 57780e1 Compare October 19, 2017 12:14

belluzj changed the title ~~WIP UFO roundtrip~~ Split the UFO builder into 1 file per "feature" Oct 19, 2017

belluzj mentioned this pull request Oct 19, 2017

UFO Rountrip #244

Merged

belluzj force-pushed the ufo_roundtrip branch from 57780e1 to 2e51ab4 Compare October 20, 2017 09:09

belluzj mentioned this pull request Oct 20, 2017

com.schriftgestaltung.useNiceNames should be boolean #247

Closed

anthrotype reviewed Oct 20, 2017

View reviewed changes

belluzj force-pushed the ufo_roundtrip branch from d3fa265 to ee1a017 Compare October 20, 2017 17:27

anthrotype reviewed Oct 22, 2017

View reviewed changes

belluzj added 4 commits October 23, 2017 14:20

Split the builder into smaller files

90d880c

Fix googlefonts#247 com.schriftgestaltung.useNiceNames should be boolean

feb3da9

Check that 'Disable Last Change' is cast to bool

45cbc92

Add builder classes instead of context, fix PR comments

98fb2dc

belluzj force-pushed the ufo_roundtrip branch from ee1a017 to 98fb2dc Compare October 23, 2017 13:21

Remove comments about the context

b7f3c9c

moyogo reviewed Oct 23, 2017

View reviewed changes

anthrotype reviewed Oct 23, 2017

View reviewed changes

moyogo reviewed Oct 23, 2017

View reviewed changes

Rename ufo_module, glyphs_module, masters

dcebcf3

anthrotype approved these changes Oct 23, 2017

View reviewed changes

belluzj merged commit cbc3d7b into googlefonts:master Oct 23, 2017

belluzj deleted the ufo_roundtrip branch October 23, 2017 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the UFO builder into 1 file per "feature" #235

Split the UFO builder into 1 file per "feature" #235

belluzj commented Oct 17, 2017 •

edited

Loading

schriftgestalt commented Oct 17, 2017

schriftgestalt commented Oct 17, 2017

anthrotype commented Oct 18, 2017 •

edited

Loading

belluzj commented Oct 18, 2017

moyogo commented Oct 18, 2017

belluzj commented Oct 19, 2017

anthrotype left a comment

belluzj commented Oct 20, 2017

belluzj commented Oct 20, 2017

anthrotype Oct 22, 2017

anthrotype Oct 22, 2017 •

edited

Loading

anthrotype Oct 22, 2017

anthrotype Oct 22, 2017

anthrotype commented Oct 22, 2017 •

edited

Loading

anthrotype Oct 22, 2017

belluzj Oct 23, 2017 •

edited

Loading

anthrotype Oct 23, 2017

anthrotype Oct 23, 2017

anthrotype Oct 22, 2017

anthrotype Oct 22, 2017

belluzj Oct 23, 2017

anthrotype Oct 23, 2017

anthrotype Oct 22, 2017 •

edited

Loading

moyogo Oct 22, 2017

belluzj Oct 23, 2017

belluzj commented Oct 23, 2017

moyogo Oct 23, 2017

moyogo Oct 23, 2017

anthrotype Oct 23, 2017

belluzj Oct 23, 2017

anthrotype Oct 23, 2017

moyogo Oct 23, 2017

anthrotype left a comment

		unicode_literals)


		def to_ufo_blue_values(_context, ufo, master):



		# Implementation is spit into one file per feature
		from .anchors import to_ufo_propagate_font_anchors, to_ufo_glyph_anchors

		return result


		def to_glyphs(ufos, designspace=None, classes=classes):

Split the UFO builder into 1 file per "feature" #235

Split the UFO builder into 1 file per "feature" #235

Conversation

belluzj commented Oct 17, 2017 • edited Loading

schriftgestalt commented Oct 17, 2017

schriftgestalt commented Oct 17, 2017

anthrotype commented Oct 18, 2017 • edited Loading

belluzj commented Oct 18, 2017

moyogo commented Oct 18, 2017

belluzj commented Oct 19, 2017

anthrotype left a comment

Choose a reason for hiding this comment

belluzj commented Oct 20, 2017

belluzj commented Oct 20, 2017

Choose a reason for hiding this comment

anthrotype Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype commented Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

belluzj Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belluzj commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype left a comment

Choose a reason for hiding this comment

belluzj commented Oct 17, 2017 •

edited

Loading

anthrotype commented Oct 18, 2017 •

edited

Loading

anthrotype Oct 22, 2017 •

edited

Loading

anthrotype commented Oct 22, 2017 •

edited

Loading

belluzj Oct 23, 2017 •

edited

Loading

anthrotype Oct 22, 2017 •

edited

Loading