Skip to content

Commit

Permalink
WIP refactor master to ufo code
Browse files Browse the repository at this point in the history
  • Loading branch information
@schriftgestalt
schriftgestalt committed Nov 7, 2017
1 parent 8d455ea commit 8825fbe
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 85 deletions.
4 changes: 2 additions & 2 deletions Lib/glyphsLib/builder/custom_params.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@
UFO2FT_FILTERS_KEY)


def to_ufo_custom_params(self, ufo, master):
def to_ufo_custom_params(master, ufo):
misc = ['DisplayStrings', 'disablesAutomaticAlignment', 'disablesNiceNames']
custom_params = parse_custom_params(self.font, misc)
custom_params = parse_custom_params(master.parent, misc)
set_custom_params(ufo, parsed=custom_params)
# the misc attributes double as deprecated info attributes!
# they are Glyphs-related, not OpenType-related, and don't go in info
Expand Down
76 changes: 2 additions & 74 deletions Lib/glyphsLib/builder/font.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,83 +29,11 @@ def to_ufo_font_attributes(self, family_name):
Modifies the list of UFOs in the UFOBuilder (self) in-place.
"""

font = self.font

# "date" can be missing; Glyphs.app removes it on saving if it's empty:
# https://github.com/googlei18n/glyphsLib/issues/134
date_created = getattr(font, 'date', None)
if date_created is not None:
date_created = to_ufo_time(date_created)
units_per_em = font.upm
version_major = font.versionMajor
version_minor = font.versionMinor
copyright = font.copyright
designer = font.designer
designer_url = font.designerURL
manufacturer = font.manufacturer
manufacturer_url = font.manufacturerURL

for master in font.masters:
ufo = self.ufo_module.Font()

if date_created is not None:
ufo.info.openTypeHeadCreated = date_created
ufo.info.unitsPerEm = units_per_em
ufo.info.versionMajor = version_major
ufo.info.versionMinor = version_minor

if copyright:
ufo.info.copyright = copyright
if designer:
ufo.info.openTypeNameDesigner = designer
if designer_url:
ufo.info.openTypeNameDesignerURL = designer_url
if manufacturer:
ufo.info.openTypeNameManufacturer = manufacturer
if manufacturer_url:
ufo.info.openTypeNameManufacturerURL = manufacturer_url

ufo.info.ascender = master.ascender
ufo.info.capHeight = master.capHeight
ufo.info.descender = master.descender
ufo.info.xHeight = master.xHeight

horizontal_stems = master.horizontalStems
vertical_stems = master.verticalStems
italic_angle = -master.italicAngle
if horizontal_stems:
ufo.info.postscriptStemSnapH = horizontal_stems
if vertical_stems:
ufo.info.postscriptStemSnapV = vertical_stems
if italic_angle:
ufo.info.italicAngle = italic_angle

width = master.width
weight = master.weight
if weight:
ufo.lib[GLYPHS_PREFIX + 'weight'] = weight
if width:
ufo.lib[GLYPHS_PREFIX + 'width'] = width
for number in ('', '1', '2', '3'):
custom_name = getattr(master, 'customName' + number)
if custom_name:
ufo.lib[GLYPHS_PREFIX + 'customName' + number] = custom_name
custom_value = getattr(master, 'customValue' + number)
if custom_value:
ufo.lib[GLYPHS_PREFIX + 'customValue' + number] = custom_value

self.to_ufo_names(ufo, master, family_name)
self.to_ufo_blue_values(ufo, master)
self.to_ufo_family_user_data(ufo)
self.to_ufo_master_user_data(ufo, master)
self.to_ufo_guidelines(ufo, master)
self.to_ufo_custom_params(ufo, master)

master_id = master.id
ufo.lib[GLYPHS_PREFIX + 'fontMasterID'] = master_id
ufo = master.ufo_object()
# FIXME: (jany) in the future, yield this UFO (for memory, laze iter)
self._ufos[master_id] = ufo
self._ufos[master.id] = ufo


def to_glyphs_font_attributes(self, ufo, master, is_initial):
Expand Down
8 changes: 4 additions & 4 deletions Lib/glyphsLib/builder/glyph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from .common import to_ufo_time
from .constants import (GLYPHLIB_PREFIX, GLYPHS_COLORS, GLYPHS_PREFIX,
PUBLIC_PREFIX)

from .guidelines import to_ufo_guidelines

def to_ufo_glyph(self, ufo_glyph, layer, glyph_data):
"""Add .glyphs metadata, paths, components, and anchors to a glyph."""
Expand Down Expand Up @@ -88,7 +88,7 @@ def to_ufo_glyph(self, ufo_glyph, layer, glyph_data):
self.to_ufo_glyph_anchors(ufo_glyph, layer.anchors)


def to_ufo_glyph_background(self, glyph, background):
def to_ufo_glyph_background(background, glyph):
"""Set glyph background."""

if not background:
Expand All @@ -115,8 +115,8 @@ def to_ufo_glyph_background(self, glyph, background):
def to_ufo_glyph_libdata(self, glyph, layer):
"""Add to a glyph's lib data."""

self.to_ufo_guidelines(glyph, layer)
self.to_ufo_glyph_background(glyph, layer.background)
to_ufo_guidelines(layer, glyph)
to_ufo_glyph_background(glyph, layer.background)
for key in ['annotations', 'hints']:
try:
value = getattr(layer, key)
Expand Down
2 changes: 1 addition & 1 deletion Lib/glyphsLib/builder/guidelines.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
unicode_literals)


def to_ufo_guidelines(self, ufo_obj, glyphs_obj):
def to_ufo_guidelines(glyphs_obj, ufo_obj):
"""Set guidelines."""
guidelines = glyphs_obj.guides
if not guidelines:
Expand Down
6 changes: 3 additions & 3 deletions Lib/glyphsLib/builder/user_data.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@
MASTER_USER_DATA_KEY = GLYPHS_PREFIX + 'fontMaster.userData'


def to_ufo_family_user_data(self, ufo):
def to_ufo_family_user_data(font, ufo):
"""Set family-wide user data as Glyphs does."""
user_data = self.font.userData
user_data = font.userData
for key in user_data.keys():
ufo.lib[key] = user_data[key]


def to_ufo_master_user_data(self, ufo, master):
def to_ufo_master_user_data(master, ufo):
"""Set master-specific user data as Glyphs does."""
user_data = master.userData
if user_data:
Expand Down
93 changes: 92 additions & 1 deletion Lib/glyphsLib/classes.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@
from fontTools.misc.py23 import unicode, basestring, UnicodeIO, unichr, open
from glyphsLib.affine import Affine

from builder.common import to_ufo_time
from builder.constants import GLYPHS_PREFIX
from builder.names import to_ufo_names
from builder.blue_values import to_ufo_blue_values
from builder.user_data import to_ufo_family_user_data, to_ufo_master_user_data
from builder.guidelines import to_ufo_guidelines
from builder.custom_params import to_ufo_custom_params

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -1282,7 +1289,91 @@ def width(self):
@width.setter
def width(self, value):
self._width = value



def ufo_object(self):
ufo = None
try:
ufo = self.parent.ufo_module.Font()
except:
import defcon
ufo = defcon.Font()


font = self.parent

family_name = font.familyName

# "date" can be missing; Glyphs.app removes it on saving if it's empty:
# https://github.com/googlei18n/glyphsLib/issues/134
date_created = getattr(font, 'date', None)
if date_created is not None:
date_created = to_ufo_time(date_created)
units_per_em = font.upm
version_major = font.versionMajor
version_minor = font.versionMinor
copyright = font.copyright
designer = font.designer
designer_url = font.designerURL
manufacturer = font.manufacturer
manufacturer_url = font.manufacturerURL

if date_created is not None:
ufo.info.openTypeHeadCreated = date_created
ufo.info.unitsPerEm = units_per_em
ufo.info.versionMajor = version_major
ufo.info.versionMinor = version_minor

if copyright:
ufo.info.copyright = copyright
if designer:
ufo.info.openTypeNameDesigner = designer
if designer_url:
ufo.info.openTypeNameDesignerURL = designer_url
if manufacturer:
ufo.info.openTypeNameManufacturer = manufacturer
if manufacturer_url:
ufo.info.openTypeNameManufacturerURL = manufacturer_url

ufo.info.ascender = self.ascender
ufo.info.capHeight = self.capHeight
ufo.info.descender = self.descender
ufo.info.xHeight = self.xHeight

horizontal_stems = self.horizontalStems
vertical_stems = self.verticalStems
italic_angle = -self.italicAngle
if horizontal_stems:
ufo.info.postscriptStemSnapH = horizontal_stems
if vertical_stems:
ufo.info.postscriptStemSnapV = vertical_stems
if italic_angle:
ufo.info.italicAngle = italic_angle

width = self.width
weight = self.weight
if weight:
ufo.lib[GLYPHS_PREFIX + 'weight'] = weight
if width:
ufo.lib[GLYPHS_PREFIX + 'width'] = width
for number in ('', '1', '2', '3'):
custom_name = getattr(self, 'customName' + number)
if custom_name:
ufo.lib[GLYPHS_PREFIX + 'customName' + number] = custom_name
custom_value = getattr(self, 'customValue' + number)
if custom_value:
ufo.lib[GLYPHS_PREFIX + 'customValue' + number] = custom_value

to_ufo_names(self, ufo, self, family_name)
to_ufo_blue_values(self, ufo, self)
to_ufo_family_user_data(font, ufo)
to_ufo_master_user_data(self, ufo)
to_ufo_guidelines(self, ufo)
to_ufo_custom_params(self, ufo)

ufo.lib[GLYPHS_PREFIX + 'fontMasterID'] = self.id

return ufo

class GSNode(GSBase):
_rx = '([-.e\d]+) ([-.e\d]+) (LINE|CURVE|QCURVE|OFFCURVE|n/a)'\
Expand Down

4 comments on commit 8825fbe

@belluzj
Copy link
Collaborator

@belluzj belluzj commented on 8825fbe Nov 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schriftgestalt This is the start of the refactoring that you mention in #278, right?

I'm not sure that this is going in a more maintainable direction that what is currently going on in "WIP UFO roundtrip" #244, so I'm starting a discussion about both approaches:

In WIP UFO roundtrip, my intent is to have:

  • two classes, UFOBuilder and GlyphsBuilder, one for each direction in which we want to translate objects
  • ideally to methods to_ufo_something and to_glyphs_something, for each something being an object or a concept to translate: style names, anchors, guidelines, layers...
  • all the to_ufo_* methods are indeed methods of the class UFOBuilder, that's why they have self as first argument, and are attached to the UFOBuilder class with imports inside the class body. Same for GlyphsBuilder
  • the pairs of methods to_ufo_something and to_glyphs_something have been split into one file per something. Since they usually use the same data, they usually have the same arguments in the same order (I say that because I see that your are reordering arguments) and their implementation are symmetrical, which should be easy to check because they are next to each other.

The good points are that

  • the code in the builder is mostly dumb code that moves data around, and the only business logic in it is concerned with the differences between Glyphs.app and UFO
  • it only depends on the public interfaces of GS* classes and defcon classes. That way, the UFOBuilder should be usable inside Glyphs.app, if given a native GSFont as argument. (and symmetrically can use any defcon-like class hierarchy).

The bad points are that

  • the UFOBuilder and GlyphsBuilder classes are big (even though they are split into small files). All the methods were grouped into those two classes so that they can always access some common data and configuration through their "self" argument. But there may be a better way?
  • you can only use them to translate the whole font to UFO. There may however be a way to split those classes into a composition of smaller "SomethingBuilder" classes.

On the other hand, I see in what you've started that you want to move the "to_ufo" methods into the GS* classes. I foresee two main problems:

  • the builder code will start to rely on private implementation details of the GS* classes, which I think is not desirable, because it will make it more complex to refactor the GS* classes in the future if needed. For example, this commit has started using GSFontMaster::parent although it is not documented in https://docu.glyphsapp.com/#gsfontmaster
  • the methods will not be available on native Glyphs.app classes, and so can not be used inside Glyphs.app in the future.

Also the classes.py is already very long, it could also use a split into smaller files (but that's not a fundamental issue).

What is your take on this refactoring?

@anthrotype
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cast my vote for Jany's solution. Not 🍝 at all.

@schriftgestalt
Copy link
Collaborator Author

@schriftgestalt schriftgestalt commented on 8825fbe Nov 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is just me playing around with the builder code. I’m not insisting on doing it like that.

My idea was to have the to_ufo... and from_ufo in the classes themselves.
So instead of builder.to_ufo_master(ufo, master) you do master.to_ufo(ufo) So each object knows how to get in and out of ufo. So the implementation details of the input and output to ufo is handled in the same place.

Maybe we can use the same file structure, but instead of side loading the methods into the Builder object, we could stick them at the GS* classes? That would keep the classes.py file clean.

For example, this commit has started using GSFontMaster::parent although it is not documented in https://docu.glyphsapp.com/#gsfontmaster

That is missing. But basically all GS* classes in Glyphs.app have a .parent property. For the same reason as needed in this commit.

@anthrotype
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the separation of concerns between the builder classes, on the one hand, and the GS* classes and the (defcon) UFO classes, on the other. The former act as mediators between the latter two, and only use public API of each; while each of the latter does not need to know about the other.

Please sign in to comment.