From 63741b95e17334cfffa1ba505fe142f755041699 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 3 Oct 2024 11:57:51 +0100 Subject: [PATCH 1/2] skip disabled custom parameters when glyphs=>ufo and minimal=True Fixes https://github.com/googlefonts/glyphsLib/issues/905 --- Lib/glyphsLib/builder/custom_params.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/Lib/glyphsLib/builder/custom_params.py b/Lib/glyphsLib/builder/custom_params.py index c7cc850ae..65b4312e0 100644 --- a/Lib/glyphsLib/builder/custom_params.py +++ b/Lib/glyphsLib/builder/custom_params.py @@ -84,7 +84,7 @@ 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 @@ -92,6 +92,8 @@ def __init__(self, glyphs_object, glyphs_module): 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) From 51c9aad4688cb680814db95d53d0be93c79e4d68 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 3 Oct 2024 14:11:11 +0100 Subject: [PATCH 2/2] add test for disabled custom parameter --- Lib/glyphsLib/classes.py | 4 ++-- tests/builder/custom_params_test.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/Lib/glyphsLib/classes.py b/Lib/glyphsLib/classes.py index 5cd002841..de9c62edd 100755 --- a/Lib/glyphsLib/classes.py +++ b/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}>" diff --git a/tests/builder/custom_params_test.py b/tests/builder/custom_params_test.py index 4230862ec..2467c897d 100644 --- a/tests/builder/custom_params_test.py +++ b/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