Merge pull request #1036 from googlefonts/skip-disabled-params

skip disabled custom parameters when glyphs=>ufo and minimal=True

Lib/glyphsLib/builder/custom_params.py

@@ -84,14 +84,16 @@ def identity(value):
 class GlyphsObjectProxy:
     """Accelerate and record access to the glyphs object's custom parameters"""
 
-    def __init__(self, glyphs_object, glyphs_module):
+    def __init__(self, glyphs_object, glyphs_module, ignore_disabled=False):
         self._owner = glyphs_object
         # This is a key part to be used in UFO lib keys to be able to choose
         # between master and font attributes during roundtrip
         self.sub_key = glyphs_object.__class__.__name__ + "."
         self._glyphs_module = glyphs_module
         self._lookup = defaultdict(list)
         for param in glyphs_object.customParameters:
+            if ignore_disabled and param.disabled:
+                continue
             self._lookup[param.name].append(param.value)
         self._handled = set()
 
@@ -1063,8 +1065,25 @@ def to_glyphs(self, glyphs, ufo):
 
 
 def to_ufo_custom_params(self, ufo, glyphs_object, set_default_params=True):
-    # glyphs_module=None because we shouldn't instanciate any Glyphs classes
-    glyphs_proxy = GlyphsObjectProxy(glyphs_object, glyphs_module=None)
+    # glyphs_module=None because we shouldn't instanciate any Glyphs classes.
+
+    # In 'minimal' mode (enabled e.g. by fontmake when converting .glyphs => .ufo
+    # for compiling OpenType fonts) we treat disabled custom parameters as if
+    # they are absent.
+    # Note that the custom parameters' 'disabled' status is still not saved in the
+    # UFO so converting back to .glyphs will mark them as enabled.
+    # https://github.com/googlefonts/glyphsLib/issues/905
+
+    # `self` is normally a UFOBuilder instance, but it can be None when this
+    # is called by `glyphsLib.builder.instances.apply_instance_data_to_ufo`.
+    # The latter function is only ever used by fontmake (to apply instance custom
+    # parameters to interpolated instance UFOs) and never in glyphsLib's own
+    # glyphs=>ufo=>glyphs code, therefore we assume that when self is None,
+    # a 'minimal' build is desired.
+    ignore_disabled = getattr(self, "minimal", True)
+    glyphs_proxy = GlyphsObjectProxy(
+        glyphs_object, glyphs_module=None, ignore_disabled=ignore_disabled
+    )
     ufo_proxy = UFOProxy(ufo)
 
     glyphs_proxy.mark_handled(UFO_FILENAME_CUSTOM_PARAM)

Lib/glyphsLib/classes.py

@@ -1370,10 +1370,10 @@ def _serialize_to_plist(self, writer):
     )
     _CUSTOM_DICT_PARAMS = frozenset("GASP Table")
 
-    def __init__(self, name="New Value", value="New Parameter"):
+    def __init__(self, name="New Value", value="New Parameter", disabled=False):
         self.name = name
         self.value = value
-        self.disabled = False
+        self.disabled = disabled
 
     def __repr__(self):
         return f"<{self.__class__.__name__} {self.name}: {self._value}>"

tests/builder/custom_params_test.py

@@ -44,6 +44,9 @@
 from glyphsLib.classes import GSFont, GSFontMaster, GSCustomParameter, GSGlyph, GSLayer
 from glyphsLib.types import parse_datetime
 
+import pytest
+
+
 DATA = os.path.join(os.path.dirname(os.path.dirname(__file__)), "data")
 
 
@@ -734,3 +737,28 @@ def test_mutiple_params(ufo_module):
 
     assert instance.customParameters[0].value == "ccmp;sub space by space;"
     assert instance.customParameters[1].value == "liga;sub space space by space;"
+
+
+@pytest.mark.parametrize("disabled", [False, True])
+def test_disabled_glyphOrder_custom_params(ufo_module, disabled):
+    # With minimal=True, 'disabled' custom parameters should be ignored
+    # https://github.com/googlefonts/glyphsLib/issues/905
+    # https://github.com/googlefonts/fontc/issues/985
+    font = GSFont()
+    font.masters.append(GSFontMaster())
+
+    implicit_glyph_order = [".notdef", "A", "B", "C"]
+    for glyph_name in implicit_glyph_order:
+        font.glyphs.append(GSGlyph(glyph_name))
+
+    custom_glyph_order = [".notdef", "C", "B", "A"]
+    font.customParameters.append(
+        GSCustomParameter("glyphOrder", custom_glyph_order, disabled=disabled)
+    )
+
+    ufo = to_ufos(font, ufo_module=ufo_module, minimal=True)[0]
+
+    if disabled:
+        assert ufo.lib["public.glyphOrder"] == implicit_glyph_order
+    else:
+        assert ufo.lib["public.glyphOrder"] == custom_glyph_order