Repository: googlefonts/glyphsLib

Issue:
Default feature writers heuristic is baffling
If the Glyphs file does not have a manual `kern` feature, glyphsLib does nothing special, and ufo2ft uses a default list of feature writers, which is

```
        KernFeatureWriter,
        MarkFeatureWriter,
        GdefFeatureWriter,
        CursFeatureWriter,
```

But if the Glyphs file has a manual `kern` feature, glyphsLib writes a `com.github.googlei18n.ufo2ft.featureWriters` lib entry in the designspace file:

https://github.com/googlefonts/glyphsLib/blob/609b1765096b016e7382b1155c4034888a0de878/Lib/glyphsLib/builder/user_data.py#L47-L55

glyphsLib's version of `DEFAULT_FEATURE_WRITERS` contains two writers: `KernFeatureWriter` and `MarkFeatureWriter`.

So an Arabic which has a manual kern feature (even if that feature just says `# Automatic code start`) will lose its cursive positioning feature. That was surprising!

I could fix this by adding more feature writers to glyphsLib's default list, but I don't think that's the right fix. I don't understand why we are we making our own "default" list, which we now have to keep in sync with ufo2ft, when *not doing anything at all* would cause ufo2ft to do the right thing.

Discussion:

Patch:
diff --git a/Lib/glyphsLib/builder/constants.py b/Lib/glyphsLib/builder/constants.py
index 815c7e74..de3f4340 100644
--- a/Lib/glyphsLib/builder/constants.py
+++ b/Lib/glyphsLib/builder/constants.py
@@ -42,6 +42,9 @@ ANONYMOUS_FEATURE_PREFIX_NAME = "<anonymous>"
 ORIGINAL_FEATURE_CODE_KEY = GLYPHLIB_PREFIX + "originalFeatureCode"
 ORIGINAL_CATEGORY_KEY = GLYPHLIB_PREFIX + "originalOpenTypeCategory"
 
+INSERT_FEATURE_MARKER_RE = r"\s*# Automatic Code.*"
+INSERT_FEATURE_MARKER_COMMENT = "# Automatic Code\n"
+
 APP_VERSION_LIB_KEY = GLYPHS_PREFIX + "appVersion"
 KEYBOARD_INCREMENT_KEY = GLYPHS_PREFIX + "keyboardIncrement"
 MASTER_ORDER_LIB_KEY = GLYPHS_PREFIX + "fontMasterOrder"
diff --git a/Lib/glyphsLib/builder/features.py b/Lib/glyphsLib/builder/features.py
index 2c821910..511e6d68 100644
--- a/Lib/glyphsLib/builder/features.py
+++ b/Lib/glyphsLib/builder/features.py
@@ -30,6 +30,8 @@ from .constants import (
     ORIGINAL_CATEGORY_KEY,
     LANGUAGE_MAPPING,
     REVERSE_LANGUAGE_MAPPING,
+    INSERT_FEATURE_MARKER_RE,
+    INSERT_FEATURE_MARKER_COMMENT,
 )
 from .tokens import TokenExpander, PassThruExpander
 
@@ -71,6 +73,11 @@ def _to_glyphs_language(langID):
     return REVERSE_LANGUAGE_MAPPING[langID]
 
 
+def _is_manual_kern_feature(feature):
+    """Return true if the feature is a manually written 'kern' features."""
+    return feature.name == "kern" and not feature.automatic
+
+
 def _to_ufo_features(
     font: GSFont,
     ufo: Font | None = None,
@@ -155,6 +162,15 @@ def _to_ufo_features(
             lines.extend("#" + line for line in code.splitlines())
         else:
             lines.append(code)
+
+        # Manual kern features in glyphs also have the automatic code added after them
+        # We make sure it gets added with an "Automatic Code..." marker if it doesn't
+        # already have one.
+        if _is_manual_kern_feature(feature) and not re.search(
+            INSERT_FEATURE_MARKER_RE, code
+        ):
+            lines.append(INSERT_FEATURE_MARKER_COMMENT)
+
         lines.append("} %s;" % feature.name)
         feature_defs.append("\n".join(lines))
     fea_str = "\n\n".join(feature_defs)
diff --git a/Lib/glyphsLib/builder/user_data.py b/Lib/glyphsLib/builder/user_data.py
index edcf3445..ece632f3 100644
--- a/Lib/glyphsLib/builder/user_data.py
+++ b/Lib/glyphsLib/builder/user_data.py
@@ -31,27 +31,12 @@ from .constants import (
 )
 
 
-def _has_manual_kern_feature(font):
-    """Return true if the GSFont contains a manually written 'kern' feature."""
-    return any(f for f in font.features if f.name == "kern" and not f.automatic)
-
-
 def to_designspace_family_user_data(self):
     if self.use_designspace:
         for key, value in dict(self.font.userData).items():
             if _user_data_has_no_special_meaning(key):
                 self.designspace.lib[key] = value
 
-        # only write our custom ufo2ft featureWriters settings if the font
-        # does have a manually written 'kern' feature; and if the lib key wasn't
-        # already set in font.userData (in which case we assume the user knows
-        # what she's doing).
-        if (
-            _has_manual_kern_feature(self.font)
-            and UFO2FT_FEATURE_WRITERS_KEY not in self.designspace.lib
-        ):
-            self.designspace.lib[UFO2FT_FEATURE_WRITERS_KEY] = DEFAULT_FEATURE_WRITERS
-
 
 def to_ufo_family_user_data(self, ufo):
     """Set family-wide user data as Glyphs does."""


Test patch:
diff --git a/tests/builder/features_test.py b/tests/builder/features_test.py
index 6030e369..d08f1bf8 100644
--- a/tests/builder/features_test.py
+++ b/tests/builder/features_test.py
@@ -17,9 +17,10 @@
 import os
 from textwrap import dedent
 
-from glyphsLib import to_glyphs, to_ufos, classes
+from glyphsLib import to_glyphs, to_ufos, classes, to_designspace
 from glyphsLib.builder.features import _build_public_opentype_categories
 
+from fontTools.designspaceLib import DesignSpaceDocument
 import pytest
 
 
@@ -741,3 +742,33 @@ def test_mark_class_used_as_glyph_class(tmpdir, ufo_module):
     # hence the following assertion would fail...
     # https://github.com/googlefonts/glyphsLib/issues/694#issuecomment-1117204523
     # assert rtufo.features.text == ufo.features.text
+
+
+def test_automatic_added_to_manual_kern(tmpdir, ufo_module):
+    """Test that when a Glyphs file has a manual kern feature,
+    automatic markers are added so that the source kerning also
+    gets applied.
+    """
+    font = classes.GSFont()
+    font.masters.append(classes.GSFontMaster())
+
+    (ufo,) = to_ufos(font)
+
+    assert "# Automatic Code" not in ufo.features.text
+
+    kern = classes.GSFeature(name="kern", code="pos a b 100;")
+    font.features.append(kern)
+    (ufo,) = to_ufos(font)
+
+    assert "# Automatic Code" in ufo.features.text
+
+    designspace = to_designspace(font, ufo_module=ufo_module)
+    path = str(tmpdir / "test.designspace")
+    designspace.write(path)
+    for source in designspace.sources:
+        source.font.save(str(tmpdir / source.filename))
+
+    designspace2 = DesignSpaceDocument.fromfile(path)
+    font2 = to_glyphs(designspace2, ufo_module=ufo_module)
+
+    assert len([f for f in font2.features if f.name == "kern"]) == 1
diff --git a/tests/builder/lib_and_user_data_test.py b/tests/builder/lib_and_user_data_test.py
index 8eeaa8c3..37c1121d 100644
--- a/tests/builder/lib_and_user_data_test.py
+++ b/tests/builder/lib_and_user_data_test.py
@@ -51,34 +51,6 @@ def test_designspace_lib_equivalent_to_font_user_data(tmpdir):
     assert designspace.lib["designspaceLibKey1"] == "designspaceLibValue1"
 
 
-def test_default_featureWriters_in_designspace_lib(tmpdir, ufo_module):
-    """Test that the glyphsLib custom featureWriters settings (with mode="append")
-    are exported to the designspace lib whenever a GSFont contains a manual 'kern'
-    feature. And that they are not imported back to GSFont.userData if they are
-    the same as the default value.
-    """
-    font = classes.GSFont()
-    font.masters.append(classes.GSFontMaster())
-    kern = classes.GSFeature(name="kern", code="pos a b 100;")
-    font.features.append(kern)
-
-    designspace = to_designspace(font, ufo_module=ufo_module)
-    path = str(tmpdir / "test.designspace")
-    designspace.write(path)
-    for source in designspace.sources:
-        source.font.save(str(tmpdir / source.filename))
-
-    designspace2 = DesignSpaceDocument.fromfile(path)
-
-    assert UFO2FT_FEATURE_WRITERS_KEY in designspace2.lib
-    assert designspace2.lib[UFO2FT_FEATURE_WRITERS_KEY] == DEFAULT_FEATURE_WRITERS
-
-    font2 = to_glyphs(designspace2, ufo_module=ufo_module)
-
-    assert not len(font2.userData)
-    assert len([f for f in font2.features if f.name == "kern"]) == 1
-
-
 def test_custom_featureWriters_in_designpace_lib(tmpdir, ufo_module):
     """Test that we can roundtrip custom user-defined ufo2ft featureWriters
     settings that are stored in the designspace lib or GSFont.userData.