Repository: googlefonts/ufo2ft
Pull request #328 (2019-05-02 12:39:28)

[kernFeatureWriter] don't set ignoreMarks when kern pairs contains marks
Basically cleaning up https://github.com/googlefonts/ufo2ft/pull/314

Also, updated expected test results;
reverted unnecessary changes to _groupScriptsByTagAndDirection;
removed unused addLookupReference function in ast module.

/cc @punchcutter


Patch:
diff --git a/Lib/ufo2ft/featureWriters/ast.py b/Lib/ufo2ft/featureWriters/ast.py
index 58f8fa8..5cf0321 100644
--- a/Lib/ufo2ft/featureWriters/ast.py
+++ b/Lib/ufo2ft/featureWriters/ast.py
@@ -181,20 +181,6 @@ def addLookupReferences(
             )
 
 
-def addLookupReference(
-    feature, lookup, script=None, languages=None, exclude_dflt=False
-):
-    """Shortcut for addLookupReferences, but for a single lookup.
-    """
-    return addLookupReferences(
-        feature,
-        (lookup,),
-        script=script,
-        languages=languages,
-        exclude_dflt=exclude_dflt,
-    )
-
-
 _GDEFGlyphClasses = collections.namedtuple(
     "_GDEFGlyphClasses", "base ligature mark component"
 )
diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
index 216f8e0..ed44786 100644
--- a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
@@ -213,7 +213,9 @@ class KernFeatureWriter(BaseFeatureWriter):
         )
         ctx.gdefClasses = ast.getGDEFGlyphClasses(feaFile)
         ctx.kerning = self.getKerningData(font, feaFile, self.getOrderedGlyphSet())
-        ctx.scriptGroups = self._groupScriptsByTagAndDirection(feaFile)
+
+        feaScripts = ast.getScriptLanguageSystems(feaFile)
+        ctx.scriptGroups = self._groupScriptsByTagAndDirection(feaScripts)
 
         return ctx
 
@@ -259,8 +261,8 @@ class KernFeatureWriter(BaseFeatureWriter):
             statements.append(ast.Comment(""))
 
         # finally add the lookup and feature blocks
-        for _, lookup in sorted(lookups.items()):
-            statements.append(lookup)
+        for _, lookupGroup in sorted(lookups.items()):
+            statements.extend(lookupGroup)
         if "kern" in features:
             statements.append(features["kern"])
         if "dist" in features:
@@ -352,24 +354,20 @@ class KernFeatureWriter(BaseFeatureWriter):
         return allKeys
 
     @staticmethod
-    def _groupScriptsByTagAndDirection(feaFile):
+    def _groupScriptsByTagAndDirection(feaScripts):
         # Read scripts/languages defined in feaFile's 'languagesystem'
         # statements and group them by the feature tag (kern or dist)
         # they are associated with, and the global script's horizontal
         # direction (DFLT is excluded)
-        feaScripts = ast.getScriptLanguageSystems(feaFile)
         scriptGroups = {}
-        tags = ['kern']
         for scriptCode, scriptLangSys in feaScripts.items():
             direction = unicodedata.script_horizontal_direction(scriptCode)
             if scriptCode in DIST_ENABLED_SCRIPTS:
-                if 'feature kern' in feaFile.asFea():
-                    tags.append('dist')
-                else:
-                    tags = ['dist']
-            for tag in tags:
-                scriptGroups.setdefault(tag, {}).setdefault(direction, []).extend(
-                    scriptLangSys
+                tag = "dist"
+            else:
+                tag = "kern"
+            scriptGroups.setdefault(tag, {}).setdefault(direction, []).extend(
+                scriptLangSys
             )
         return scriptGroups
 
@@ -394,7 +392,9 @@ class KernFeatureWriter(BaseFeatureWriter):
             enumerated=enumerated,
         )
 
-    def _makeKerningLookup(self, name, pairs, exclude=None, rtl=False, ignoreMarks=True):
+    def _makeKerningLookup(
+        self, name, pairs, exclude=None, rtl=False, ignoreMarks=True
+    ):
         assert pairs
         rules = []
         for pair in pairs:
@@ -429,24 +429,7 @@ class KernFeatureWriter(BaseFeatureWriter):
         else:
             shouldSplit = False
 
-        # If there are pairs with a mix of mark/base then the IgnoreMarks
-        # flag is unnecessary and should not be set
-        base_to_base = []
-        base_to_mark = []
-        for pair in self.context.kerning.pairs:
-            hasMarks = False
-            for glyph in pair.side1.glyphSet() + pair.side2.glyphSet():
-                if isinstance(glyph, ast.GlyphName):
-                    glyph = glyph.glyphSet()[0]
-                if self.context.gdefClasses.mark is not None:
-                    if glyph in self.context.gdefClasses.mark:
-                        hasMarks = True
-                        break
-            if hasMarks:
-                base_to_mark.append(pair)
-            else:
-                base_to_base.append(pair)
-
+        marks = self.context.gdefClasses.mark
         lookups = {}
         if shouldSplit:
             # make one DFLT lookup with script-agnostic characters, and two
@@ -463,100 +446,89 @@ class KernFeatureWriter(BaseFeatureWriter):
             if not pairs:
                 return lookups
 
-            dfltKern = self._makeKerningLookup(
-                "kern_dflt",
-                pairs,
-                exclude=(
-                    lambda pair: bool({"LTR", "RTL"}.intersection(pair.directions))
-                                 or pair in base_to_mark
-                ),
-                rtl=False,
-            )
-            if dfltKern:
-                lookups["DFLT"] = dfltKern
-
-            ltrKern = self._makeKerningLookup(
-                "kern_ltr",
-                pairs,
-                exclude=(
-                    lambda pair: (not pair.directions
-                    or "RTL" in pair.directions)
-                                 or pair in base_to_mark
-                ),
-                rtl=False,
-            )
-            if ltrKern:
-                lookups["LTR"] = ltrKern
-
-            rtlKern = self._makeKerningLookup(
-                "kern_rtl",
-                pairs,
-                exclude=(
-                    lambda pair: (not pair.directions
-                    or "LTR" in pair.directions)
-                                 or pair in base_to_mark
-                ),
-                rtl=True,
-            )
-            if rtlKern:
-                lookups["RTL"] = rtlKern
-
-            if base_to_mark:
-                dfltKernHasMarks = self._makeKerningLookup(
-                    "kern_dflt_marks",
-                    pairs,
-                    exclude=(
-                        lambda pair: bool({"LTR", "RTL"}.intersection(pair.directions))
-                                     and pair not in base_to_mark
-                    ),
-                    rtl=False,
-                    ignoreMarks=False,
-                )
-                if dfltKernHasMarks:
-                    lookups["DFLTHasMarks"] = dfltKernHasMarks
-
-                ltrKernHasMarks = self._makeKerningLookup(
-                    "kern_ltr_marks",
-                    pairs,
-                    exclude=(
-                        lambda pair: pair not in base_to_mark or (not pair.directions
-                        or "RTL" in pair.directions)
-                    ),
-                    rtl=False,
-                    ignoreMarks=False,
-                )
-                if ltrKernHasMarks:
-                    lookups["LTRHasMarks"] = ltrKernHasMarks
-
-                rtlKernHasMarks = self._makeKerningLookup(
-                    "kern_rtl_marks",
-                    pairs,
-                    exclude=(
-                        lambda pair: pair not in base_to_mark or (not pair.directions
-                        or "LTR" in pair.directions)
-                    ),
-                    rtl=True,
-                    ignoreMarks=False,
-                )
-                if rtlKernHasMarks:
-                    lookups["RTLHasMarks"] = rtlKernHasMarks
+            if self.options.ignoreMarks:
+                # If there are pairs with a mix of mark/base then the IgnoreMarks
+                # flag is unnecessary and should not be set
+                basePairs, markPairs = self._splitBaseAndMarkPairs(pairs, marks)
+                self._makeSplitDirectionKernLookups(lookups, basePairs)
+                if markPairs:
+                    self._makeSplitDirectionKernLookups(
+                        lookups, markPairs, ignoreMarks=False, suffix="_marks"
+                    )
+            else:
+                self._makeSplitDirectionKernLookups(lookups, pairs)
         else:
             # only make a single (implicitly LTR) lookup including all base/base pairs
-            # and a single lookup including all base/mark pairs if necessary
-            lookups["LTR"] = self._makeKerningLookup(
-                "kern_ltr",
-                self.context.kerning.pairs,
-                exclude=(lambda pair: pair in base_to_mark),
-            )
-            if base_to_mark:
-                lookups["LTRHasMarks"] = self._makeKerningLookup(
-                    "kern_ltr_marks",
-                    self.context.kerning.pairs,
-                    exclude=(lambda pair: pair in base_to_base),
-                    ignoreMarks=False,
-                )
+            # and a single lookup including all base/mark pairs (if any)
+            pairs = self.context.kerning.pairs
+            if self.options.ignoreMarks:
+                basePairs, markPairs = self._splitBaseAndMarkPairs(pairs, marks)
+                lookups["LTR"] = [self._makeKerningLookup("kern_ltr", basePairs)]
+                if markPairs:
+                    lookups["LTR"].append(
+                        self._makeKerningLookup(
+                            "kern_ltr_marks",
+                            markPairs,
+                            ignoreMarks=False,
+                        )
+                    )
+            else:
+                lookups["LTR"] = [self._makeKerningLookup("kern_ltr", pairs)]
         return lookups
 
+    def _splitBaseAndMarkPairs(self, pairs, marks):
+        basePairs, markPairs = [], []
+        if marks:
+            for pair in pairs:
+                if any(glyph in marks for glyph in pair.glyphs):
+                    markPairs.append(pair)
+                else:
+                    basePairs.append(pair)
+        else:
+            basePairs[:] = pairs
+        return basePairs, markPairs
+
+    def _makeSplitDirectionKernLookups(
+        self, lookups, pairs, ignoreMarks=True, suffix=""
+    ):
+        dfltKern = self._makeKerningLookup(
+            "kern_dflt" + suffix,
+            pairs,
+            exclude=(
+                lambda pair: {"LTR", "RTL"}.intersection(pair.directions)
+            ),
+            rtl=False,
+            ignoreMarks=ignoreMarks,
+        )
+        if dfltKern:
+            lookups.setdefault("DFLT", []).append(dfltKern)
+
+        ltrKern = self._makeKerningLookup(
+            "kern_ltr" + suffix,
+            pairs,
+            exclude=(
+                lambda pair: not pair.directions
+                or "RTL" in pair.directions
+            ),
+            rtl=False,
+            ignoreMarks=ignoreMarks,
+        )
+        if ltrKern:
+            lookups.setdefault("LTR", []).append(ltrKern)
+
+        rtlKern = self._makeKerningLookup(
+            "kern_rtl" + suffix,
+            pairs,
+            exclude=(
+                lambda pair: not pair.directions
+                or "LTR" in pair.directions
+            ),
+            rtl=True,
+            ignoreMarks=ignoreMarks,
+        )
+        if rtlKern:
+            lookups.setdefault("RTL", []).append(rtlKern)
+
     def _makeFeatureBlocks(self, lookups):
         features = {}
         if "kern" in self.context.todo:
@@ -573,40 +545,33 @@ class KernFeatureWriter(BaseFeatureWriter):
 
     def _registerKernLookups(self, feature, lookups):
         if "DFLT" in lookups:
-            ast.addLookupReference(feature, lookups["DFLT"])
+            ast.addLookupReferences(feature, lookups["DFLT"])
+
         scriptGroups = self.context.scriptGroups
         if "dist" in self.context.todo:
-            distScripts = scriptGroups.get("dist")
+            distScripts = scriptGroups["dist"]
         else:
             distScripts = {}
         kernScripts = scriptGroups.get("kern", {})
         ltrScripts = kernScripts.get("LTR", [])
         rtlScripts = kernScripts.get("RTL", [])
-        ltrLkp = lookups.get("LTR")
-        rtlLkp = lookups.get("RTL")
-        dfltLkpHasMarks = lookups.get("DFLTHasMarks")
-        ltrLkpHasMarks = lookups.get("LTRHasMarks")
-        rtlLkpHasMarks = lookups.get("RTLHasMarks")
-        ltr_lookups = [ltrLkp]
-        rtl_lookups = [rtlLkp]
-        if ltrLkpHasMarks is not None:
-            ltr_lookups.append(ltrLkpHasMarks)
-        if rtlLkpHasMarks is not None:
-            rtl_lookups.append(rtlLkpHasMarks)
-        if ltrLkp and rtlLkp:
+
+        ltrLookups = lookups.get("LTR")
+        rtlLookups = lookups.get("RTL")
+        if ltrLookups and rtlLookups:
             if ltrScripts and rtlScripts:
                 for script, langs in ltrScripts:
-                    ast.addLookupReferences(feature, ltr_lookups, script, langs)
+                    ast.addLookupReferences(feature, ltrLookups, script, langs)
                 for script, langs in rtlScripts:
-                    ast.addLookupReferences(feature, rtl_lookups, script, langs)
+                    ast.addLookupReferences(feature, rtlLookups, script, langs)
             elif ltrScripts:
-                ast.addLookupReference(feature, rtlLkp, script="DFLT")
+                ast.addLookupReferences(feature, rtlLookups, script="DFLT")
                 for script, langs in ltrScripts:
-                    ast.addLookupReferences(feature, ltr_lookups, script, langs)
+                    ast.addLookupReferences(feature, ltrLookups, script, langs)
             elif rtlScripts:
-                ast.addLookupReference(feature, ltrLkp, script="DFLT")
+                ast.addLookupReferences(feature, ltrLookups, script="DFLT")
                 for script, langs in rtlScripts:
-                    ast.addLookupReferences(feature, rtl_lookups, script, langs)
+                    ast.addLookupReferences(feature, rtlLookups, script, langs)
             else:
                 if not (distScripts.get("LTR") and distScripts.get("RTL")):
                     raise ValueError(
@@ -614,38 +579,28 @@ class KernFeatureWriter(BaseFeatureWriter):
                         "lookups; add 'languagesystems' to features for at "
                         "least one LTR or RTL script using the kern feature"
                     )
-        elif ltrLkp:
+        elif ltrLookups:
             if not (rtlScripts or distScripts):
-                ast.addLookupReferences(feature, ltr_lookups)
+                ast.addLookupReferences(feature, ltrLookups)
             else:
-                ast.addLookupReferences(feature, ltr_lookups, script="DFLT")
+                ast.addLookupReferences(feature, ltrLookups, script="DFLT")
                 for script, langs in ltrScripts:
-                    ast.addLookupReferences(feature, ltr_lookups, script, langs)
-        elif rtlLkp:
+                    ast.addLookupReferences(feature, ltrLookups, script, langs)
+        elif rtlLookups:
             if not (ltrScripts or distScripts):
-                ast.addLookupReferences(feature, rtl_lookups)
+                ast.addLookupReferences(feature, rtlLookups)
             else:
-                ast.addLookupReferences(feature, rtl_lookups, script="DFLT")
+                ast.addLookupReferences(feature, rtlLookups, script="DFLT")
                 for script, langs in rtlScripts:
-                    ast.addLookupReferences(feature, rtl_lookups, script, langs)
+                    ast.addLookupReferences(feature, rtlLookups, script, langs)
 
     def _registerDistLookups(self, feature, lookups):
-        scripts = self.context.scriptGroups.get("dist")
-        ltrLkp = lookups.get("LTR")
-        if ltrLkp:
+        scripts = self.context.scriptGroups["dist"]
+        ltrLookups = lookups.get("LTR")
+        if ltrLookups:
             for script, langs in scripts.get("LTR", []):
-                ast.addLookupReference(feature, ltrLkp, script, langs)
-
-        ltrLkpHasMarks = lookups.get("LTRHasMarks")
-        if ltrLkpHasMarks:
-            for script, langs in scripts.get("LTRHasMarks", []):
-                ast.addLookupReference(feature, ltrLkpHasMarks, script, langs)
-        rtlLkp = lookups.get("RTL")
-        if rtlLkp:
+                ast.addLookupReferences(feature, ltrLookups, script, langs)
+        rtlLookups = lookups.get("RTL")
+        if rtlLookups:
             for script, langs in scripts.get("RTL", []):
-                ast.addLookupReference(feature, rtlLkp, script, langs)
-
-        rtlLkpHasMarks = lookups.get("RTLHasMarks")
-        if rtlLkpHasMarks:
-            for script, langs in scripts.get("RTLHasMarks", []):
-                ast.addLookupReference(feature, rtlLkpHasMarks, script, langs)
+                ast.addLookupReferences(feature, rtlLookups, script, langs)


Test patch:
diff --git a/tests/featureWriters/kernFeatureWriter_test.py b/tests/featureWriters/kernFeatureWriter_test.py
index 9b21889..e0bb005 100644
--- a/tests/featureWriters/kernFeatureWriter_test.py
+++ b/tests/featureWriters/kernFeatureWriter_test.py
@@ -163,11 +163,11 @@ class KernFeatureWriterTest(FeatureWriterTest):
                 lookupflag IgnoreMarks;
                 pos B C -30;
             } kern_ltr;
-            
+
             lookup kern_ltr_marks {
                 pos A acutecomb -55;
             } kern_ltr_marks;
-            
+
             feature kern {
                 lookup kern_ltr;
                 lookup kern_ltr_marks;
@@ -175,6 +175,20 @@ class KernFeatureWriterTest(FeatureWriterTest):
             """
         )
 
+        feaFile = self.writeFeatures(font, ignoreMarks=False)
+        assert str(feaFile) == dedent(
+            """
+            lookup kern_ltr {
+                pos A acutecomb -55;
+                pos B C -30;
+            } kern_ltr;
+
+            feature kern {
+                lookup kern_ltr;
+            } kern;
+            """
+        )
+
     def test_mode(self, FontClass):
         ufo = FontClass()
         for name in ("one", "four", "six", "seven"):
@@ -269,8 +283,9 @@ class KernFeatureWriterTest(FeatureWriterTest):
         )
 
         feaFile = parseLayoutFeatures(font)
+        scripts = ast.getScriptLanguageSystems(feaFile)
         scriptGroups = KernFeatureWriter._groupScriptsByTagAndDirection(
-            feaFile
+            scripts
         )
 
         assert "kern" in scriptGroups
@@ -287,46 +302,6 @@ class KernFeatureWriterTest(FeatureWriterTest):
             ("dev2", ["dflt"]),
         ]
 
-    def test__groupScriptsByTagAndDirectionDistAndKern(self, FontClass):
-        font = FontClass()
-        font.newGlyph('ka-sidd').unicode = 0x1158E
-        font.newGlyph('ga-sidd').unicode = 0x11590
-        font.features.text = dedent(
-            """
-            languagesystem DFLT dflt;
-            languagesystem sidd dflt;
-            
-            @kern1.ka = [ka-sidd];
-            @kern2.ga = [ga-sidd];
-
-            lookup kern_ltr {
-                lookupflag IgnoreMarks;
-                enum pos @kern1.ka @kern2.ga -40;
-            } kern_ltr;
-
-            feature kern {
-                lookup kern_ltr;
-                script sidd;
-            } kern;
-            
-            """
-        )
-
-        feaFile = parseLayoutFeatures(font)
-        scriptGroups = KernFeatureWriter._groupScriptsByTagAndDirection(
-            feaFile
-        )
-
-        assert "kern" in scriptGroups
-        assert list(scriptGroups["kern"]["LTR"]) == [
-            ("sidd", ["dflt"])
-        ]
-
-        assert "dist" in scriptGroups
-        assert list(scriptGroups["dist"]["LTR"]) == [
-            ("sidd", ["dflt"]),
-        ]
-
     def test_getKerningClasses(self, FontClass):
         font = FontClass()
         for i in range(65, 65 + 6):  # A..F
@@ -487,7 +462,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         ufo = makeUFO(FontClass, glyphs, groups, kerning, features)
 
-        newFeatures = self.writeFeatures(ufo)
+        newFeatures = self.writeFeatures(ufo, ignoreMarks=False)
 
         assert str(newFeatures) == dedent(
             """\
@@ -496,17 +471,14 @@ class KernFeatureWriterTest(FeatureWriterTest):
             @kern2.alef = [alef-ar alef-ar.isol];
 
             lookup kern_dflt {
-                lookupflag IgnoreMarks;
                 pos seven four -25;
             } kern_dflt;
 
             lookup kern_ltr {
-                lookupflag IgnoreMarks;
                 enum pos @kern1.A V -40;
             } kern_ltr;
 
             lookup kern_rtl {
-                lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
                 pos reh-ar.fina lam-ar.init <-80 0 -80 0>;
                 pos @kern1.reh @kern2.alef <-100 0 -100 0>;
@@ -606,12 +578,6 @@ class KernFeatureWriterTest(FeatureWriterTest):
                 pos seven four -25;
             } kern_dflt;
 
-            lookup kern_dflt_marks {
-                pos V acutecomb 70;
-                pos reh-ar fatha-ar 80;
-                pos seven four -25;
-            } kern_dflt_marks;
-
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 enum pos @kern1.A V -40;
@@ -705,21 +671,17 @@ class KernFeatureWriterTest(FeatureWriterTest):
             """\
             @kern1.reh = [reh-ar zain-ar reh-ar.fina];
             @kern2.alef = [alef-ar alef-ar.isol];
-            
-            lookup kern_dflt_marks {
-                pos reh-ar fatha-ar 80;
-            } kern_dflt_marks;
-            
+
             lookup kern_rtl {
                 lookupflag IgnoreMarks;
                 pos reh-ar.fina lam-ar.init <-80 0 -80 0>;
                 pos @kern1.reh @kern2.alef <-100 0 -100 0>;
             } kern_rtl;
-            
+
             lookup kern_rtl_marks {
                 pos reh-ar fatha-ar <80 0 80 0>;
             } kern_rtl_marks;
-            
+
             feature kern {
                 lookup kern_rtl;
                 lookup kern_rtl_marks;