Repository: googlefonts/glyphsLib
Pull request #545 (2019-08-15 15:16:12)

Save UFO filenames for masters and instances in custom parameter
Closes https://github.com/googlefonts/glyphsLib/issues/319.


Patch:
diff --git a/Lib/glyphsLib/builder/constants.py b/Lib/glyphsLib/builder/constants.py
index 2c3f7c1f..2ee6a9c4 100644
--- a/Lib/glyphsLib/builder/constants.py
+++ b/Lib/glyphsLib/builder/constants.py
@@ -21,6 +21,7 @@ ROBOFONT_PREFIX = "com.typemytype.robofont."
 UFO2FT_FILTERS_KEY = "com.github.googlei18n.ufo2ft.filters"
 UFO2FT_USE_PROD_NAMES_KEY = "com.github.googlei18n.ufo2ft.useProductionNames"
 COMPONENT_INFO_KEY = GLYPHLIB_PREFIX + "ComponentInfo"
+UFO_FILENAME_CUSTOM_PARAM = "UFO Filename"
 
 MASTER_CUSTOM_PARAM_PREFIX = GLYPHS_PREFIX + "customParameter.GSFontMaster."
 FONT_CUSTOM_PARAM_PREFIX = GLYPHS_PREFIX + "customParameter.GSFont."
diff --git a/Lib/glyphsLib/builder/instances.py b/Lib/glyphsLib/builder/instances.py
index 26a84da5..b9e828e9 100644
--- a/Lib/glyphsLib/builder/instances.py
+++ b/Lib/glyphsLib/builder/instances.py
@@ -20,7 +20,7 @@ import logging
 from fontTools.misc.py23 import basestring
 from glyphsLib.util import build_ufo_path
 from glyphsLib.classes import WEIGHT_CODES, GSCustomParameter
-from .constants import GLYPHS_PREFIX, GLYPHLIB_PREFIX
+from .constants import GLYPHS_PREFIX, GLYPHLIB_PREFIX, UFO_FILENAME_CUSTOM_PARAM
 from .names import build_stylemap_names
 from .axes import (
     get_axis_definitions,
@@ -77,11 +77,10 @@ def _to_designspace_instance(self, instance):
         ufo_instance.familyName = self.family_name
     ufo_instance.styleName = instance.name
 
-    # TODO: (jany) investigate the possibility of storing a relative path in
-    #   the `filename` custom parameter. If yes, drop the key below.
-    #   Maybe do the same for masters?
-    #   https://github.com/googlefonts/glyphsLib/issues/319
-    fname = instance.customParameters[FULL_FILENAME_KEY]
+    fname = (
+        instance.customParameters[UFO_FILENAME_CUSTOM_PARAM]
+        or instance.customParameters[FULL_FILENAME_KEY]
+    )
     if fname is not None:
         if self.instance_dir:
             fname = self.instance_dir + "/" + os.path.basename(fname)
@@ -139,6 +138,7 @@ def _to_designspace_instance(self, instance):
             "postscriptFontName",
             "fileName",
             FULL_FILENAME_KEY,
+            UFO_FILENAME_CUSTOM_PARAM,
         ):
             # These will be stored in the official descriptor attributes
             continue
@@ -262,8 +262,8 @@ def to_glyphs_instances(self):  # noqa: C901
             for name, value in ufo_instance.lib[CUSTOM_PARAMETERS_KEY]:
                 instance.customParameters.append(GSCustomParameter(name, value))
 
-        if self.minimize_ufo_diffs:
-            instance.customParameters[FULL_FILENAME_KEY] = ufo_instance.filename
+        if ufo_instance.filename and self.minimize_ufo_diffs:
+            instance.customParameters[UFO_FILENAME_CUSTOM_PARAM] = ufo_instance.filename
 
         # FIXME: (jany) cannot `.append()` because no proxy => no parent
         self.font.instances = self.font.instances + [instance]
diff --git a/Lib/glyphsLib/builder/masters.py b/Lib/glyphsLib/builder/masters.py
index 2a0345a0..c92a9e4f 100644
--- a/Lib/glyphsLib/builder/masters.py
+++ b/Lib/glyphsLib/builder/masters.py
@@ -17,7 +17,7 @@ from __future__ import print_function, division, absolute_import, unicode_litera
 import os
 
 from .axes import font_uses_new_axes, get_axis_definitions
-from .constants import GLYPHS_PREFIX, GLYPHLIB_PREFIX
+from .constants import GLYPHS_PREFIX, GLYPHLIB_PREFIX, UFO_FILENAME_CUSTOM_PARAM
 
 MASTER_ID_LIB_KEY = GLYPHS_PREFIX + "fontMasterID"
 UFO_FILENAME_KEY = GLYPHLIB_PREFIX + "ufoFilename"
@@ -92,9 +92,11 @@ def to_glyphs_master_attributes(self, source, master):
         master.id = ufo_master_id_lib_key
 
     if source.filename is not None and self.minimize_ufo_diffs:
-        master.userData[UFO_FILENAME_KEY] = source.filename
+        master.customParameters[UFO_FILENAME_CUSTOM_PARAM] = source.filename
     elif ufo.path and self.minimize_ufo_diffs:
-        master.userData[UFO_FILENAME_KEY] = os.path.basename(ufo.path)
+        # Don't be smart, we don't know where the UFOs come from so we can't make them
+        # relative to anything.
+        master.customParameters[UFO_FILENAME_CUSTOM_PARAM] = os.path.basename(ufo.path)
 
     master.ascender = ufo.info.ascender
     master.capHeight = ufo.info.capHeight
diff --git a/Lib/glyphsLib/builder/sources.py b/Lib/glyphsLib/builder/sources.py
index ef67bf8f..3ced4e01 100644
--- a/Lib/glyphsLib/builder/sources.py
+++ b/Lib/glyphsLib/builder/sources.py
@@ -23,6 +23,7 @@ from glyphsLib.util import build_ufo_path
 
 from .masters import UFO_FILENAME_KEY
 from .axes import get_axis_definitions, get_regular_master, font_uses_new_axes, interp
+from .constants import UFO_FILENAME_CUSTOM_PARAM
 
 
 logger = logging.getLogger(__name__)
@@ -76,16 +77,17 @@ def _to_designspace_source(self, master, is_regular):
     # UFO_SOURCE_NAME_KEY
     source.name = "{} {}".format(source.familyName, source.styleName)
 
-    if UFO_FILENAME_KEY in master.userData:
+    if UFO_FILENAME_CUSTOM_PARAM in master.customParameters:
+        source.filename = master.customParameters[UFO_FILENAME_CUSTOM_PARAM]
+    elif UFO_FILENAME_KEY in master.userData:
         source.filename = master.userData[UFO_FILENAME_KEY]
     else:
-        # TODO: (jany) allow another naming convention?
         source.filename = build_ufo_path("", source.familyName, source.styleName)
 
     # Make sure UFO filenames are unique, lest we overwrite masters that
     # happen to have the same weight name. Careful, name clashes can also happen when
-    # the masters have a UFO_FILENAME_KEY: when the user duplicates a master in Glyphs
-    # but forgets to change the UFO filename key in the master's userData (ha!).
+    # the masters have a UFO_FILENAME_CUSTOM_PARAM: when the user duplicates a master
+    # in Glyphs but forgets to change the custom parameter.
     # Attention: The following is done regardless of where source.filename came from.
     # Depending on the duplicate's order in the master list, this could lead to the
     # legitimate master's filename getting an underscore appended!
@@ -96,13 +98,18 @@ def _to_designspace_source(self, master, is_regular):
     ):
         filename_stem, filename_ext = os.path.splitext(source.filename)
         source.filename = "{}#{}{}".format(filename_stem, n, filename_ext)
-        if UFO_FILENAME_KEY in master.userData:
+        if (
+            UFO_FILENAME_CUSTOM_PARAM in master.customParameters
+            or UFO_FILENAME_KEY in master.userData
+        ):
             logger.warning(
                 "The master with the style name '{}' (ID {}) would be written to the "
                 "same destination as another master. Change the "
-                "master.userData['com.schriftgestaltung.Glyphs.ufoFilename'] "
-                "parameter to select a different destination. Proceeding, but appending"
-                " '#{}' to the filename on disk.".format(source.styleName, master.id, n)
+                "master's custom parameter '{}' "
+                "to select a different destination. Proceeding, but appending"
+                " '#{}' to the filename on disk.".format(
+                    source.styleName, master.id, UFO_FILENAME_CUSTOM_PARAM, n
+                )
             )
         else:
             logger.warning(
diff --git a/Lib/glyphsLib/cli.py b/Lib/glyphsLib/cli.py
index 8fcc7ebf..2df0e127 100644
--- a/Lib/glyphsLib/cli.py
+++ b/Lib/glyphsLib/cli.py
@@ -184,8 +184,8 @@ def glyphs2ufo(options):
         )
 
     # If options.instance_dir is None, instance UFO paths in the designspace
-    # file will either use the value in customParameter's FULL_FILENAME_KEY or be
-    # made relative to "instance_ufos/".
+    # file will either use the value in customParameter's UFO_FILENAME_CUSTOM_PARAM or
+    # be made relative to "instance_ufos/".
     glyphsLib.build_masters(
         options.glyphs_file,
         options.output_dir,


Test patch:
diff --git a/tests/builder/custom_params_test.py b/tests/builder/custom_params_test.py
index cf229329..85f19390 100644
--- a/tests/builder/custom_params_test.py
+++ b/tests/builder/custom_params_test.py
@@ -18,6 +18,7 @@ from __future__ import print_function, division, absolute_import, unicode_litera
 
 from textwrap import dedent
 import unittest
+import os
 
 # unittest.mock is only available for python 3.3+
 try:
@@ -36,9 +37,14 @@ from glyphsLib.builder.constants import (
     UFO2FT_USE_PROD_NAMES_KEY,
     FONT_CUSTOM_PARAM_PREFIX,
     MASTER_CUSTOM_PARAM_PREFIX,
+    UFO_FILENAME_CUSTOM_PARAM,
 )
+from glyphsLib.builder.masters import UFO_FILENAME_KEY
+from glyphsLib.builder.instances import FULL_FILENAME_KEY
 from glyphsLib.classes import GSFont, GSFontMaster, GSCustomParameter
 
+DATA = os.path.join(os.path.dirname(os.path.dirname(__file__)), "data")
+
 
 class SetCustomParamsTest(unittest.TestCase):
     def setUp(self):
@@ -408,3 +414,53 @@ class SetCustomParamsTest(unittest.TestCase):
         )
         ufo_rt = glyphsLib.to_ufos(font_rt)[0]
         self.assertEqual(ufo_rt.lib[UFO2FT_FILTERS_KEY], ufo_filters)
+
+
+def test_ufo_filename_custom_param():
+    """Test that new-style UFO_FILENAME_CUSTOM_PARAM is written instead of
+    (UFO_FILENAME_KEY|FULL_FILENAME_KEY)."""
+    font = glyphsLib.GSFont(os.path.join(DATA, "UFOFilenameTest.glyphs"))
+    ds = glyphsLib.to_designspace(font, minimize_glyphs_diffs=True)
+    assert ds.sources[0].filename == "MyFontMaster.ufo"
+    assert ds.instances[0].filename == "../build/instance_ufos/MyFont.ufo"
+    assert "com.schriftgestaltung.customParameters" not in ds.instances[0].lib
+
+    font_rt = glyphsLib.to_glyphs(ds, minimize_ufo_diffs=True)
+    assert (
+        font_rt.masters[0].customParameters[UFO_FILENAME_CUSTOM_PARAM]
+        == "MyFontMaster.ufo"
+    )
+    assert UFO_FILENAME_KEY not in font_rt.masters[0].userData
+    assert (
+        font_rt.instances[0].customParameters[UFO_FILENAME_CUSTOM_PARAM]
+        == "../build/instance_ufos/MyFont.ufo"
+    )
+    assert FULL_FILENAME_KEY not in font_rt.instances[0].customParameters
+
+    ds_rt = glyphsLib.to_designspace(font_rt, minimize_glyphs_diffs=True)
+    assert ds_rt.sources[0].filename == "MyFontMaster.ufo"
+    assert ds_rt.instances[0].filename == "../build/instance_ufos/MyFont.ufo"
+
+
+def test_ufo_filename_custom_param_plus_legacy():
+    """Test that new-style UFO_FILENAME_CUSTOM_PARAM overrides legacy
+    (UFO_FILENAME_KEY|FULL_FILENAME_KEY)."""
+    font = glyphsLib.GSFont(os.path.join(DATA, "UFOFilenameTest.glyphs"))
+    font.masters[0].customParameters[UFO_FILENAME_CUSTOM_PARAM] = "aaa.ufo"
+    font.instances[0].customParameters[UFO_FILENAME_CUSTOM_PARAM] = "bbb.ufo"
+
+    ds = glyphsLib.to_designspace(font, minimize_glyphs_diffs=True)
+    assert ds.sources[0].filename == "aaa.ufo"
+    assert ds.instances[0].filename == "bbb.ufo"
+
+
+def test_ufo_filename_custom_param_instance_empty():
+    font = glyphsLib.GSFont(os.path.join(DATA, "UFOFilenameTest.glyphs"))
+    font.masters[0].customParameters[UFO_FILENAME_CUSTOM_PARAM] = "aaa.ufo"
+    del font.instances[0].customParameters[UFO_FILENAME_CUSTOM_PARAM]
+    del font.instances[0].customParameters[FULL_FILENAME_KEY]
+
+    ds = glyphsLib.to_designspace(font, minimize_glyphs_diffs=True)
+    assert ds.sources[0].filename == "aaa.ufo"
+    # Instance filename should be whatever the default is.
+    assert ds.instances[0].filename == "instance_ufos/NewFont-Regular.ufo"
diff --git a/tests/data/UFOFilenameTest.glyphs b/tests/data/UFOFilenameTest.glyphs
new file mode 100644
index 00000000..f19b0110
--- /dev/null
+++ b/tests/data/UFOFilenameTest.glyphs
@@ -0,0 +1,56 @@
+{
+.appVersion = "1230";
+date = "2019-08-15 11:39:54 +0000";
+familyName = "New Font";
+fontMaster = (
+{
+ascender = 800;
+capHeight = 700;
+descender = -200;
+id = "F8C94C2E-1D94-453C-B1DE-0473CD97C963";
+userData = {
+com.schriftgestaltung.Glyphs.ufoFilename = MyFontMaster.ufo;
+};
+xHeight = 500;
+}
+);
+glyphs = (
+{
+glyphname = A;
+layers = (
+{
+layerId = "F8C94C2E-1D94-453C-B1DE-0473CD97C963";
+width = 600;
+}
+);
+unicode = 0041;
+},
+{
+glyphname = space;
+layers = (
+{
+layerId = "F8C94C2E-1D94-453C-B1DE-0473CD97C963";
+width = 600;
+}
+);
+unicode = 0020;
+}
+);
+instances = (
+{
+customParameters = (
+{
+name = com.schriftgestaltung.Glyphs.fullFilename;
+value = ../build/instance_ufos/MyFont.ufo;
+}
+);
+instanceInterpolations = {
+"F8C94C2E-1D94-453C-B1DE-0473CD97C963" = 1;
+};
+name = Regular;
+}
+);
+unitsPerEm = 1000;
+versionMajor = 1;
+versionMinor = 0;
+}