custom parameter is used when present even if disabled

[anthrotype]

The .glyphs fonts at this link https://github.com/clauseggers/Playfair/tree/master/sources contain "Axis Mappings" custom parameters whose checkboxews are disabled in the Glyphs UI. The plist for the custom parameter contains a disabled = 1 attribute. The sources also contain "Axis Location" parameters, so the intention is that the latter be used instead of the disabled Axis Mappings.
When both methods are present, glyphsLib prefers to use the "Axis Mappings" as it's the most explicit. However, if this is marked as disabled glyphsLib should treat it as if it is not set at all.
Currently it is still being read and applied, which may lead to unexpected effects if it is not set the same way as the Axis Location params.

However
Original issue reported by @clauseggers: googlefonts/fontmake#1002

[anthrotype]

this may be a larger issue because, while GSCustomParameter class does have a disabled attribute, it doesn't look like we do anything with it... If I search for "disabled" in glyphsLib/builders/custom_params.py or anywhere else really, I don't see it being used anywhere, so it's not just "Axis Mappings".
Most places where we look up custom parameters we get the value associated with the parameter's name (done in the ListDictionaryProxy.__getitem__ when the key is not an int or a slice, so presumably a str), but we do not check whether that parameter is disabled. To read that disabled attribute you need to have the GSCustomParameter instance, not just its value...

[anthrotype]

I could fix this immediate issue by simply doing:

diff --git a/Lib/glyphsLib/builder/axes.py b/Lib/glyphsLib/builder/axes.py
index cb3b7f2d..af33d7c1 100644
--- a/Lib/glyphsLib/builder/axes.py
+++ b/Lib/glyphsLib/builder/axes.py
@@ -158,7 +158,13 @@ def to_designspace_axes(self):
     regular_master = get_regular_master(self.font)
     assert isinstance(regular_master, classes.GSFontMaster)

-    custom_mapping = self.font.customParameters["Axis Mappings"]
+    # only use 'Axis Mappings' if actually enabled
+    # https://github.com/googlefonts/glyphsLib/issues/905
+    custom_mapping = None
+    for param in self.font.customParameters:
+        if param.name == "Axis Mappings" and not param.disabled:
+            custom_mapping = param.value
+            break

     for axis_def in get_axis_definitions(self.font):
         axis = self.designspace.newAxisDescriptor()

But this won't fix all the other unchecked uses of disabled custom parameters across the library.

If we change CustomParametersProxy.__getitem__ or get methods to only return the values for parameters whose disabled attribute is False, it would fix the building part but it most likely break the roundtripping (ufo2glyphs) side of things because we would treat any disabled parameters as if they aren't there.

I'm not sure how to proceed.

[khaledhosny]

See also #809

[anthrotype]

this old issue resurfaced again in googlefonts/fontc#985 (comment)

fontc is reading the disabled flag and ignoring the custom glyphOrder whereas fontmake is using it as if it were enabled (which is not), leading to huge ttx diffs.

Earlier I noted that if we were to treat disabled parameters as absent, we could break round-tripping (glyphs2ufo => ufo2glyphs) but actually this is already broken in the sense that the 'disabled' flag is not stored anywhere in the UFO and is completely lost when going back to .glyphs. If we care about round-tripping disabled flag we need to save it in the UFO lib.

A disabled custom parameter is effectively treated by Glyphs.app as if it was not set, so the current behavior which ignores the disabled flag is plainly wrong, both for the use case of building binary fonts from .glyphs sources and for round-tripping them to and from UFO (in the latter case a previously disabled parameter suddely become enabled after roundtrip).

I'm leaning towards the easy route and treating disabled as absent, which would fix the build binary font use case, and would not break an already broken round-trip use case...

[anthrotype]

now I also wonder.. what if there are projects out there use fontmake to build and rely on this broken behavior of ignoring the 'disabled'-ness of custom parameters? Do you think this is conceivable or know of any that we should be concerned about? Or am I over-thinking it?

[anthrotype]

a simple change like this one should be sufficient to skip exporting disabled custom parameters (including glyphOrder) when going from Glyphs to UFO:

diff --git a/Lib/glyphsLib/builder/custom_params.py b/Lib/glyphsLib/builder/custom_params.py
index c7cc850a..c09724d8 100644
--- a/Lib/glyphsLib/builder/custom_params.py
+++ b/Lib/glyphsLib/builder/custom_params.py
@@ -92,6 +92,8 @@ class GlyphsObjectProxy:
         self._glyphs_module = glyphs_module
         self._lookup = defaultdict(list)
         for param in glyphs_object.customParameters:
+            if param.disabled:
+                continue
             self._lookup[param.name].append(param.value)
         self._handled = set()

Interestingly, no existing unit test fails..

[khaledhosny]

We can restrict it to minimal=True like in #1027. I would not worry about fonts potentially depending on this broken behavior, as even of such fonts exist, they can easily be fixed by not disabling the parameter.

[anthrotype]

i believe this should be kept open, because #1036 only addressed custom params as exported to UFO lib or fontinfo, but there may be other cases where we read and use custom parameters that may be disabled and we should ignore instead.
This issue mentioned a disabled "Axis Mappings" which gets used to build the DS axes despite being disabled..

We stll need to check all the usages of custom parameters across the library and make them ineffective when disabled.