Repository: simoncozens/fontFeatures

Issue:
Substitute [f f.ss01] [i i.ss01] -> [f_i f_i.ss01]; should be possible (ligature sub expansion)
I've already implemented this; this is a placeholder for a PR.

Discussion:

Patch:
diff --git a/fontFeatures/__init__.py b/fontFeatures/__init__.py
index e52523e..badf25b 100644
--- a/fontFeatures/__init__.py
+++ b/fontFeatures/__init__.py
@@ -218,8 +218,8 @@ class FontFeatures:
         return rv
 
     def setGlyphClassesFromFont(self, font):
-        for g in font.exportedGlyphs():
-            self.glyphclasses[g] = font[g].category
+        for g in font:
+            self.glyphclasses[g.name] = g.category
 
 
 class Routine:
diff --git a/fontFeatures/feaLib/Substitution.py b/fontFeatures/feaLib/Substitution.py
index 10741c1..6d2c174 100644
--- a/fontFeatures/feaLib/Substitution.py
+++ b/fontFeatures/feaLib/Substitution.py
@@ -2,20 +2,37 @@
 import fontTools.feaLib.ast as feaast
 from fontFeatures.ttLib.Substitution import lookup_type
 from itertools import cycle
+from typing import Union
 
-
-def glyphref(g):
+def glyphref(g) -> Union[feaast.GlyphName, feaast.GlyphClass]:
     if len(g) == 1:
         return feaast.GlyphName(g[0])
     return feaast.GlyphClass([feaast.GlyphName(x) for x in g])
 
+# Whether or not the input or the replacement has classes (so, entire statement).
+def has_classes(self) -> bool:
+    input_has_class = next((i for i,v in enumerate([len(x) for x in self.input]) if v>1), None)
+    replacement_has_class = next((i for i,v in enumerate([len(x) for x in self.replacement]) if v>1), None)
+
+    return (input_has_class is not None or replacement_has_class is not None)
+
+# Excepting single glyphs, if classes are defined, the input and replacement
+# all use classes of equal length.
+def all_classes_equal(self) -> bool:
+    input_lengths = [len(x) for x in self.input if len(x) != 1]
+    replacement_lengths = [len(x) for x in self.replacement if len(x) != 1]
+
+    if len(input_lengths) == 0 and len(replacement_lengths) == 0:
+        return True
+
+    return len(set(input_lengths+replacement_lengths)) == 1
 
-def is_paired(self):
-    # One of the substitution/replacements has all-but-one arity one,
-    # and both arities are the same
+# One of the substitution/replacements has all-but-one arity one,
+# and both arities are the same
+def is_paired(self) -> bool:
     input_lengths = [len(x) for x in self.input if len(x) != 1]
     replacement_lengths = [len(x) for x in self.replacement if len(x) != 1]
-    if not (len(input_lengths) == 1 and len(replacement_lengths) ==1):
+    if not (len(input_lengths) == 1 and len(replacement_lengths) == 1):
         return False
     if input_lengths[0] != replacement_lengths[0]:
         import warnings
@@ -23,7 +40,17 @@ def is_paired(self):
         return False
     return True
 
-def paired_ligature(self):
+# Expand ligature substitutions ending in a class, such that:
+#   * Substitute [f f.ss01] i -> [f_i f_i.ss01];
+# Expands to:
+#   * sub f i -> f_i;
+#   * sub f.ss01 i -> f_i.ss01;
+# Likewise:
+#   * Substitute [f f.ss01] [i i.ss01] -> [f_i f_i.ss01];
+# Expands to:
+#   * sub f i -> f_i;
+#   * sub f.ss01 i.ss01 -> f_i.ss01;
+def paired_ligature(self) -> feaast.LigatureSubstStatement:
     b = feaast.Block()
     inputs = []
     for i in self.input:
@@ -39,6 +66,7 @@ def paired_ligature(self):
         else:
             replacements.append(j)
     rhs = zip(*replacements)
+
     for l, r in zip(lhs,rhs):
         stmt = feaast.LigatureSubstStatement(
             [glyphref(x) for x in self.precontext],
@@ -55,7 +83,7 @@ def paired_ligature(self):
 # Becomes in FEA:
 #   * sub a by before_tail a.2 tail;
 #   * sub b by before_tail b.2 tail;
-def paired_mult(self):
+def paired_mult(self) -> feaast.MultipleSubstStatement:
     b = feaast.Block()
 
     input_lengths = [len(x) for x in self.input]
@@ -123,8 +151,8 @@ def asFeaAST(self):
             feaast.GlyphClass([feaast.GlyphName(x) for x in self.replacement[0]]),
         )
     elif lut == 4: # GSUB 4 Ligature Substitution
-        # Paired rules need to become a set of statements
-        if is_paired(self):
+        # Some rules with classes need to become a set of statements.
+        if has_classes(self) and all_classes_equal(self) and not len(self.replacement[0]) == 1:
             return paired_ligature(self)
 
         return feaast.LigatureSubstStatement(
diff --git a/fontFeatures/feeLib/Anchors.py b/fontFeatures/feeLib/Anchors.py
index 9013b6d..bbf6c0d 100644
--- a/fontFeatures/feeLib/Anchors.py
+++ b/fontFeatures/feeLib/Anchors.py
@@ -78,8 +78,7 @@ class Anchors:
 class LoadAnchors:
     @classmethod
     def action(self, parser):
-        for glyphname in parser.font.exportedGlyphs():
-            g = parser.font[glyphname]
+        for g in parser.font:
             for a in g.anchors:
                 if not g.name in parser.fontfeatures.anchors:
                     parser.fontfeatures.anchors[g.name] = {}
diff --git a/fontFeatures/feeLib/ClassDefinition.py b/fontFeatures/feeLib/ClassDefinition.py
index d2080cb..4a7b41c 100644
--- a/fontFeatures/feeLib/ClassDefinition.py
+++ b/fontFeatures/feeLib/ClassDefinition.py
@@ -101,7 +101,6 @@ will take any glyph selector and display its contents on standard error.
 
 import re
 from glyphtools import get_glyph_metrics, bin_glyphs_by_metric
-from bidict import ValueDuplicationError
 
 import warnings
 
@@ -136,13 +135,7 @@ class DefineClass:
         predicates = definition[1]
         for p in predicates:
             glyphs = list(filter(lambda x: self.meets_predicate(x, p, parser), glyphs))
-        try:
-            parser.fontfeatures.namedClasses[classname["classname"]] = tuple(glyphs)
-        except ValueDuplicationError as e:
-            import warnings
-            warnings.warn("Could not define class %s as it contains the same"
-                    " glyphs as an existing class %s" % (classname["classname"],
-                        parser.fontfeatures.namedClasses.inverse[tuple(glyphs)]))
+        parser.fontfeatures.namedClasses[classname["classname"]] = tuple(glyphs)
 
     @classmethod
     def resolve_definition(self, parser, primary):
diff --git a/fontFeatures/feeLib/__init__.py b/fontFeatures/feeLib/__init__.py
index b59eb23..a295695 100644
--- a/fontFeatures/feeLib/__init__.py
+++ b/fontFeatures/feeLib/__init__.py
@@ -61,7 +61,7 @@ class GlyphSelector:
     def resolve(self, fontfeatures, font, mustExist=True):
         returned = []
         assert isinstance(font, Font)
-        glyphs = font.exportedGlyphs()
+        glyphs = font.keys()
         if "barename" in self.selector:
             returned = [self.selector["barename"]]
         elif "unicodeglyph" in self.selector:


Test patch:
diff --git a/tests/test_substitution.py b/tests/test_substitution.py
index c6ae0c4..68ce8c7 100644
--- a/tests/test_substitution.py
+++ b/tests/test_substitution.py
@@ -28,6 +28,12 @@ class TestSubstitution(unittest.TestCase):
         self.assertEqual(s.involved_glyphs, set(["a", "b", "c"]))
         self.roundTrip(s)
 
+    def test_ligature_expansion(self):
+        s = Substitution([["f", "f.ss01"], ["i", "i.ss01"]], [["f_i", "f_i.ss01"]])
+        self.assertEqual(s.asFea(), "    sub f i by f_i;\n    sub f.ss01 i.ss01 by f_i.ss01;\n")
+        self.assertEqual(s.involved_glyphs, set(["f", "i", "f_i", "f.ss01", "i.ss01", "f_i.ss01"]))
+        self.roundTrip(s)
+
     def test_multiple(self):
         s = Substitution(["a"], ["b", "c"])
         self.assertEqual(s.asFea(), "sub a by b c;")