Repository: googlefonts/ufo2ft
Pull request #705 (2023-02-13 14:30:30)

Avoid overwriting classDefs
Ensure that we always reuse the name for class definitions for a given sequence of glyph names. This avoids duplicates and redefinitions. I validated the kerning on a body of Noto and proprietary font sources using https://github.com/daltonmaag/kerning-validator/tree/b2aafc740fbf33b503bac61a0a0237c7fab2aaf8.

The test case is extracted from a real font and pared down as much as I could.

Closes https://github.com/googlefonts/ufo2ft/issues/702

madig: Do I remember correctly that you want me to change the code back to returning classDefs, which we then insert manually, with an assert looking for duplicate names?
anthrotype: in general, I prefer when a function's inputs and outputs are clearly distinct instead of a mutable inout parameter.. whatever makes the code clearer

Patch:
diff --git a/Lib/ufo2ft/featureCompiler.py b/Lib/ufo2ft/featureCompiler.py
index ae35adc..b5e589b 100644
--- a/Lib/ufo2ft/featureCompiler.py
+++ b/Lib/ufo2ft/featureCompiler.py
@@ -47,7 +47,7 @@ def parseLayoutFeatures(font, includeDir=None):
         # the include directory to the parent of the UFO.
         ufoPath = os.path.normpath(ufoPath)
         buf.name = os.path.join(ufoPath, "features.fea")
-        includeDir = os.path.dirname(ufoPath) or "."
+        includeDir = os.path.dirname(ufoPath)
     glyphNames = set(font.keys())
     includeDir = os.path.normpath(includeDir) if includeDir else None
     try:
diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
index ad98e5c..e451a4d 100644
--- a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
@@ -398,12 +398,20 @@ class KernFeatureWriter(BaseFeatureWriter):
         bidiGlyphs = self.context.bidiGlyphs
         glyphScripts = self.context.glyphScripts
         kerningPerScript = splitKerning(pairs, glyphScripts)
+        side1Classes = self.context.kerning.side1Classes
+        side2Classes = self.context.kerning.side2Classes
 
-        classDefs, side1Classes, side2Classes = makeAllGlyphClassDefinitions(
+        newClassDefs, newSide1Classes, newSide2Classes = makeAllGlyphClassDefinitions(
             kerningPerScript, self.context, self.context.feaFile
         )
-        assert not classDefs.keys() & self.context.kerning.classDefs.keys()
-        self.context.kerning.classDefs.update(classDefs)
+        # NOTE: Consider duplicate names a bug, even if the classes would carry
+        # the same glyphs.
+        assert not self.context.kerning.classDefs.keys() & newClassDefs.keys()
+        self.context.kerning.classDefs.update(newClassDefs)
+        assert not side1Classes.keys() & newSide1Classes.keys()
+        side1Classes.update(newSide1Classes)
+        assert not side2Classes.keys() & newSide2Classes.keys()
+        side2Classes.update(newSide2Classes)
 
         for script, pairs in kerningPerScript.items():
             scriptLookups = lookups.setdefault(script, {})
@@ -597,39 +605,53 @@ def partitionByScript(
 
 
 def makeAllGlyphClassDefinitions(kerningPerScript, context, feaFile=None):
-    side1Classes = {}
-    side2Classes = {}
+    # Note: Refer to the context for existing classDefs and mappings of glyph
+    # class tuples to feaLib AST to avoid overwriting existing class names,
+    # because base and mark kerning pairs might be separate passes.
+    newClassDefs = {}
+    existingSide1Classes = context.kerning.side1Classes
+    existingSide2Classes = context.kerning.side2Classes
+    newSide1Classes = {}
+    newSide2Classes = {}
     side1Membership = context.side1Membership
     side2Membership = context.side2Membership
 
-    classDefs = {}
     if feaFile is not None:
         classNames = {cdef.name for cdef in ast.iterClassDefinitions(feaFile)}
     else:
         classNames = set()
+    classNames.update(context.kerning.classDefs.keys())
 
     # Generate common class names first so that common classes are correctly
     # named in other lookups.
     if COMMON_SCRIPT in kerningPerScript:
         common_pairs = kerningPerScript[COMMON_SCRIPT]
         for pair in common_pairs:
-            if pair.firstIsClass and pair.side1 not in side1Classes:
+            if (
+                pair.firstIsClass
+                and pair.side1 not in existingSide1Classes
+                and pair.side1 not in newSide1Classes
+            ):
                 addClassDefinition(
                     "kern1",
                     pair.side1,
-                    side1Classes,
+                    newSide1Classes,
                     side1Membership,
-                    classDefs,
+                    newClassDefs,
                     classNames,
                     COMMON_CLASS_NAME,
                 )
-            if pair.secondIsClass and pair.side2 not in side2Classes:
+            if (
+                pair.secondIsClass
+                and pair.side2 not in existingSide2Classes
+                and pair.side2 not in newSide2Classes
+            ):
                 addClassDefinition(
                     "kern2",
                     pair.side2,
-                    side2Classes,
+                    newSide2Classes,
                     side2Membership,
-                    classDefs,
+                    newClassDefs,
                     classNames,
                     COMMON_CLASS_NAME,
                 )
@@ -639,28 +661,36 @@ def makeAllGlyphClassDefinitions(kerningPerScript, context, feaFile=None):
         if script == COMMON_SCRIPT:
             continue
         for pair in pairs:
-            if pair.firstIsClass and pair.side1 not in side1Classes:
+            if (
+                pair.firstIsClass
+                and pair.side1 not in existingSide1Classes
+                and pair.side1 not in newSide1Classes
+            ):
                 addClassDefinition(
                     "kern1",
                     pair.side1,
-                    side1Classes,
+                    newSide1Classes,
                     side1Membership,
-                    classDefs,
+                    newClassDefs,
                     classNames,
                     script,
                 )
-            if pair.secondIsClass and pair.side2 not in side2Classes:
+            if (
+                pair.secondIsClass
+                and pair.side2 not in existingSide2Classes
+                and pair.side2 not in newSide2Classes
+            ):
                 addClassDefinition(
                     "kern2",
                     pair.side2,
-                    side2Classes,
+                    newSide2Classes,
                     side2Membership,
-                    classDefs,
+                    newClassDefs,
                     classNames,
                     script,
                 )
 
-    return classDefs, side1Classes, side2Classes
+    return newClassDefs, newSide1Classes, newSide2Classes
 
 
 def addClassDefinition(


Test patch:
diff --git a/tests/featureCompiler_test.py b/tests/featureCompiler_test.py
index 6d6bead..611e5e7 100644
--- a/tests/featureCompiler_test.py
+++ b/tests/featureCompiler_test.py
@@ -15,6 +15,8 @@ from ufo2ft.featureWriters import (
     ast,
 )
 
+from .testSupport import pushd
+
 
 class ParseLayoutFeaturesTest:
     def test_include(self, FontClass, tmpdir):
@@ -38,16 +40,14 @@ class ParseLayoutFeaturesTest:
 
         assert "# hello world" in str(fea)
 
-    def test_include_no_ufo_path(self, FontClass, tmpdir, monkeypatch):
+    def test_include_no_ufo_path(self, FontClass, tmpdir):
         ufo = FontClass()
         ufo.features.text = dedent(
             """\
             include(test.fea)
             """
         )
-        with monkeypatch.context() as context:
-            context.chdir(str(tmpdir))
-            ufo.save("Test.ufo")
+        with pushd(str(tmpdir)):
             with pytest.raises(IncludedFeaNotFound):
                 parseLayoutFeatures(ufo)
 
@@ -100,18 +100,6 @@ class ParseLayoutFeaturesTest:
 
         assert "# hello world" in str(fea)
 
-    def test_include_dir_cwd(self, FontClass, tmp_path, monkeypatch):
-        (tmp_path / "test.fea").write_text("# hello world", encoding="utf-8")
-        ufo = FontClass()
-        ufo.features.text = "include(test.fea)"
-
-        with monkeypatch.context() as context:
-            context.chdir(tmp_path)
-            ufo.save("Test.ufo")
-            fea = parseLayoutFeatures(ufo)
-
-        assert "# hello world" in str(fea)
-
 
 class DummyFeatureWriter:
     tableTag = "GPOS"
diff --git a/tests/featureWriters/kernFeatureWriter_test.py b/tests/featureWriters/kernFeatureWriter_test.py
index d406e21..bee7403 100644
--- a/tests/featureWriters/kernFeatureWriter_test.py
+++ b/tests/featureWriters/kernFeatureWriter_test.py
@@ -1840,6 +1840,117 @@ def test_kern_hira_kana_hrkt(FontClass):
     )
 
 
+# flake8: noqa: B950
+def test_defining_classdefs(FontClass):
+    """Check that we aren't redefining class definitions with different
+    content."""
+
+    glyphs = {
+        "halant-telugu": 0xC4D,  # Telu
+        "ka-telugu.below": None,  # Telu by substitution
+        "ka-telugu": 0xC15,  # Telu
+        "rVocalicMatra-telugu": 0xC43,  # Telu
+        "sha-telugu.below": None,  # Default
+        "ss-telugu.alt": None,  # Default
+        "ssa-telugu.alt": None,  # Telu by substitution
+        "ssa-telugu": 0xC37,  # Telu
+    }
+    groups = {
+        "public.kern1.sha-telugu.below": ["sha-telugu.below"],
+        # The following group is a mix of Telu and Default through its gylphs. The
+        # kerning for bases below will create a Telu and Default split group.
+        # Important for the NOTE below.
+        "public.kern1.ssa-telugu.alt": ["ssa-telugu.alt", "ss-telugu.alt"],
+        "public.kern2.ka-telugu.below": ["ka-telugu.below"],
+        "public.kern2.rVocalicMatra-telugu": ["rVocalicMatra-telugu"],
+    }
+    kerning = {
+        # The follwoing three pairs are base-to-base pairs:
+        ("public.kern1.sha-telugu.below", "public.kern2.ka-telugu.below"): 20,
+        ("public.kern1.ssa-telugu.alt", "public.kern2.ka-telugu.below"): 60,
+        ("public.kern1.ssa-telugu.alt", "sha-telugu.below"): 150,
+        # NOTE: This last pair kerns bases against marks, triggering an extra
+        # pass to make a mark lookup that will create new classDefs. This extra
+        # pass will work on just this one pair, and kern splitting won't split
+        # off a Default group from `public.kern1.ssa-telugu.alt`, you get just a
+        # Telu pair. Unless the writer keeps track of which classDefs it already
+        # generated, this will overwrite the previous `@kern1.Telu.ssatelugu.alt
+        # = [ssa-telugu.alt]` with `@kern1.Telu.ssatelugu.alt =
+        # [ss-telugu.alt]`, losing kerning.
+        ("public.kern1.ssa-telugu.alt", "public.kern2.rVocalicMatra-telugu"): 180,
+    }
+    features = """
+        feature blwf {
+            script tel2;
+            sub halant-telugu ka-telugu by ka-telugu.below;
+        } blwf;
+
+        feature psts {
+            script tel2;
+            sub ssa-telugu' [rVocalicMatra-telugu sha-telugu.below ka-telugu.below] by ssa-telugu.alt;
+        } psts;
+    """
+    ufo = makeUFO(FontClass, glyphs, groups, kerning, features)
+    ufo.lib["public.openTypeCategories"] = {
+        "halant-telugu": "mark",
+        "ka-telugu": "base",
+        "rVocalicMatra-telugu": "mark",
+        "ss-telugu.alt": "base",
+        "ssa-telugu.alt": "base",
+        "ssa-telugu": "base",
+    }
+
+    newFeatures = KernFeatureWriterTest.writeFeatures(ufo)
+
+    assert dedent(str(newFeatures)) == dedent(
+        """\
+        @kern1.Default.ssatelugu.alt = [ss-telugu.alt];
+        @kern1.Telu.shatelugu.below = [sha-telugu.below];
+        @kern1.Telu.ssatelugu.alt = [ssa-telugu.alt];
+        @kern2.Telu.katelugu.below = [ka-telugu.below];
+        @kern2.Telu.rVocalicMatratelugu = [rVocalicMatra-telugu];
+        
+        lookup kern_Telu {
+            lookupflag IgnoreMarks;
+            enum pos @kern1.Telu.ssatelugu.alt sha-telugu.below 150;
+            pos @kern1.Telu.shatelugu.below @kern2.Telu.katelugu.below 20;
+            pos @kern1.Default.ssatelugu.alt @kern2.Telu.katelugu.below 60;
+            pos @kern1.Telu.ssatelugu.alt @kern2.Telu.katelugu.below 60;
+        } kern_Telu;
+        
+        lookup kern_Telu_marks {
+            pos @kern1.Default.ssatelugu.alt @kern2.Telu.rVocalicMatratelugu 180;
+            pos @kern1.Telu.ssatelugu.alt @kern2.Telu.rVocalicMatratelugu 180;
+        } kern_Telu_marks;
+        
+        lookup kern_Default {
+            lookupflag IgnoreMarks;
+            enum pos @kern1.Default.ssatelugu.alt sha-telugu.below 150;
+        } kern_Default;
+        
+        feature kern {
+            script DFLT;
+            language dflt;
+            lookup kern_Default;
+        } kern;
+        
+        feature dist {
+            script tel2;
+            language dflt;
+            lookup kern_Default;
+            lookup kern_Telu;
+            lookup kern_Telu_marks;
+        
+            script telu;
+            language dflt;
+            lookup kern_Default;
+            lookup kern_Telu;
+            lookup kern_Telu_marks;
+        } dist;
+        """
+    )
+
+
 if __name__ == "__main__":
     import sys
 
diff --git a/tests/testSupport.py b/tests/testSupport.py
index a4ced95..5ab3b28 100644
--- a/tests/testSupport.py
+++ b/tests/testSupport.py
@@ -1,3 +1,5 @@
+import contextlib
+import os
 import sys
 import types
 
@@ -26,3 +28,13 @@ class _TempModule:
         else:
             del sys.modules[self.mod_name]
         self._saved_module = []
+
+
+@contextlib.contextmanager
+def pushd(target):
+    saved = os.getcwd()
+    os.chdir(target)
+    try:
+        yield saved
+    finally:
+        os.chdir(saved)