Repository: googlefonts/ufo2ft
Pull request #604 (2022-04-07 11:52:46)

filters/featureWriters: use '...' as placeholder for loading from UFO lib
Currently the `filters` and `featureWriters` parameters always override the ones defined in the UFO lib.plist, and it's impossible to extend instead of replace these, e.g. inserting some additional filter or writer either before or after.

This PR allows to pass a special value `...` (the actual [``Ellipsis`` singleton](https://medium.com/techtofreedom/3-uses-of-the-ellipsis-in-python-25795aac723d), not the str literal '...') in both these lists, which acts as a placeholder to signal one wants to extend the list of predefined filters/writers.

E.g.:

```python
filters=[..., DecomposeTransformedComponentsFilter(pre=True)]
```

The ellipsis is replaced by the custom filters defined in the UFO lib (if any), and the additional one is appended to the list.
Only one ellipsis is allowed in the list.
Same for the featureWriters.

Fontmake will also be updated to allow passing --filter='...' or --feature-writer='...' CLI options to use this feature.

Part of fixing https://github.com/googlefonts/fontmake/issues/872


Patch:
diff --git a/Lib/ufo2ft/featureCompiler.py b/Lib/ufo2ft/featureCompiler.py
index 05e151f..aa6d90d 100644
--- a/Lib/ufo2ft/featureCompiler.py
+++ b/Lib/ufo2ft/featureCompiler.py
@@ -17,6 +17,7 @@ from ufo2ft.featureWriters import (
     KernFeatureWriter,
     MarkFeatureWriter,
     ast,
+    isValidFeatureWriter,
     loadFeatureWriters,
 )
 
@@ -165,10 +166,17 @@ class FeatureCompiler(BaseFeatureCompiler):
               under the key "com.github.googlei18n.ufo2ft.featureWriters"
               (see loadFeatureWriters).
             - if that is not found, the default list of writers will be used:
-              [KernFeatureWriter, MarkFeatureWriter]. This generates "kern"
-              (or "dist" for Indic scripts), "mark" and "mkmk" features.
+              (see FeatureCompiler.defaultFeatureWriters, and the individual
+              feature writer classes for the list of features generated).
             If the featureWriters list is empty, no automatic feature is
             generated and only pre-existing features are compiled.
+            The ``featureWriters`` parameter overrides both the writers from
+            the UFO lib and the default writers list. To extend instead of
+            replace the latter, the list can contain a special value ``...``
+            (i.e. the ``ellipsis`` singleton, not the str literal '...')
+            which gets replaced by either the UFO.lib writers or the default
+            ones; thus one can insert additional writers either before or after
+            these.
         """
         BaseFeatureCompiler.__init__(self, ufo, ttFont, glyphSet)
 
@@ -184,10 +192,41 @@ class FeatureCompiler(BaseFeatureCompiler):
                 stacklevel=2,
             )
 
+    def _load_custom_feature_writers(self, featureWriters=None):
+        # Args:
+        #   ufo: Font
+        #   featureWriters: Optional[List[Union[FeatureWriter, EllipsisType]]])
+        # Returns: List[FeatureWriter]
+
+        # by default, load the feature writers from the lib or the default ones;
+        # ellipsis is used as a placeholder so one can optionally insert additional
+        # featureWriters=[w1, ..., w2] either before or after these, or override
+        # them by omitting the ellipsis.
+        if featureWriters is None:
+            featureWriters = [...]
+        result = []
+        seen_ellipsis = False
+        for writer in featureWriters:
+            if writer is ...:
+                if seen_ellipsis:
+                    raise ValueError("ellipsis not allowed more than once")
+                writers = loadFeatureWriters(self.ufo)
+                if writers is not None:
+                    result.extend(writers)
+                else:
+                    result.extend(self.defaultFeatureWriters)
+                seen_ellipsis = True
+            else:
+                klass = writer if isclass(writer) else type(writer)
+                if not isValidFeatureWriter(klass):
+                    raise TypeError(f"Invalid feature writer: {writer!r}")
+                result.append(writer)
+        return result
+
     def initFeatureWriters(self, featureWriters=None):
         """Initialize feature writer classes as specified in the UFO lib.
-        If none are defined in the UFO, the default feature writers are used:
-        currently, KernFeatureWriter and MarkFeatureWriter.
+        If none are defined in the UFO, the default feature writers are used
+        (see FeatureCompiler.defaultFeatureWriters).
         The 'featureWriters' argument can be used to override these.
         The method sets the `self.featureWriters` attribute with the list of
         writers.
@@ -197,10 +236,7 @@ class FeatureCompiler(BaseFeatureCompiler):
         used in the subsequent feature writers to resolve substitutions from
         glyphs with unicodes to their alternates.
         """
-        if featureWriters is None:
-            featureWriters = loadFeatureWriters(self.ufo)
-            if featureWriters is None:
-                featureWriters = self.defaultFeatureWriters
+        featureWriters = self._load_custom_feature_writers(featureWriters)
 
         gsubWriters = []
         others = []
diff --git a/Lib/ufo2ft/featureWriters/ast.py b/Lib/ufo2ft/featureWriters/ast.py
index ace7577..6fa8825 100644
--- a/Lib/ufo2ft/featureWriters/ast.py
+++ b/Lib/ufo2ft/featureWriters/ast.py
@@ -209,9 +209,9 @@ def getGDEFGlyphClasses(feaLib):
     """Return GDEF GlyphClassDef base/mark/ligature/component glyphs, or
     None if no GDEF table is defined in the feature file.
     """
-    for st in feaLib.statements:
-        if isinstance(st, ast.TableBlock) and st.name == "GDEF":
-            for st in st.statements:
+    for s in feaLib.statements:
+        if isinstance(s, ast.TableBlock) and s.name == "GDEF":
+            for st in s.statements:
                 if isinstance(st, ast.GlyphClassDefStatement):
                     return _GDEFGlyphClasses(
                         frozenset(st.baseGlyphs.glyphSet())
diff --git a/Lib/ufo2ft/featureWriters/markFeatureWriter.py b/Lib/ufo2ft/featureWriters/markFeatureWriter.py
index ba16b77..473541e 100644
--- a/Lib/ufo2ft/featureWriters/markFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/markFeatureWriter.py
@@ -201,15 +201,15 @@ def colorGraph(adjacency):
     """
     # Basic implementation
     # https://en.wikipedia.org/wiki/Greedy_coloring
-    color = dict()
+    colors = dict()
     # Sorted for reproducibility, probably not the optimal vertex order
     for node in sorted(adjacency):
         usedNeighbourColors = {
-            color[neighbour] for neighbour in adjacency[node] if neighbour in color
+            colors[neighbour] for neighbour in adjacency[node] if neighbour in colors
         }
-        color[node] = firstAvailable(usedNeighbourColors)
+        colors[node] = firstAvailable(usedNeighbourColors)
     groups = defaultdict(list)
-    for node, color in color.items():
+    for node, color in colors.items():
         groups[color].append(node)
     return list(groups.values())
 
diff --git a/Lib/ufo2ft/filters/__init__.py b/Lib/ufo2ft/filters/__init__.py
index e43223c..9b38c1d 100644
--- a/Lib/ufo2ft/filters/__init__.py
+++ b/Lib/ufo2ft/filters/__init__.py
@@ -85,7 +85,7 @@ def isValidFilter(klass):
     a '__call__' (bound method), with the signature matching the same method
     from the BaseFilter class:
 
-           def __call__(self, font, feaFile, compiler=None)
+           def __call__(self, font, glyphSet=None)
     """
     if not isclass(klass):
         logger.error(f"{klass!r} is not a class")
diff --git a/Lib/ufo2ft/preProcessor.py b/Lib/ufo2ft/preProcessor.py
index 4780361..c0d476e 100644
--- a/Lib/ufo2ft/preProcessor.py
+++ b/Lib/ufo2ft/preProcessor.py
@@ -1,14 +1,42 @@
+import itertools
+
 from ufo2ft.constants import (
     COLOR_LAYER_MAPPING_KEY,
     COLOR_LAYERS_KEY,
     COLOR_PALETTES_KEY,
 )
-from ufo2ft.filters import loadFilters
+from ufo2ft.filters import isValidFilter, loadFilters
 from ufo2ft.filters.decomposeComponents import DecomposeComponentsFilter
 from ufo2ft.fontInfoData import getAttrWithFallback
 from ufo2ft.util import _GlyphSet
 
 
+def _load_custom_filters(ufo, filters=None):
+    # Args:
+    #   ufo: Font
+    #   filters: Optional[List[Union[Filter, EllipsisType]]])
+    # Returns: List[Filter]
+
+    # by default, load the filters from the lib; ellipsis is used as a placeholder
+    # so one can optionally insert additional filters=[f1, ..., f2] either
+    # before or after these, or override them by omitting the ellipsis.
+    if filters is None:
+        filters = [...]
+    seen_ellipsis = False
+    result = []
+    for f in filters:
+        if f is ...:
+            if seen_ellipsis:
+                raise ValueError("ellipsis not allowed more than once")
+            result.extend(itertools.chain(*loadFilters(ufo)))
+            seen_ellipsis = True
+        else:
+            if not isValidFilter(type(f)):
+                raise TypeError(f"Invalid filter: {f!r}")
+            result.append(f)
+    return result
+
+
 class BasePreProcessor:
     """Base class for objects that performs pre-processing operations on
     the UFO glyphs, such as decomposing composites, removing overlaps, or
@@ -26,8 +54,17 @@ class BasePreProcessor:
     initialization of the default filters.
 
     Custom filters can be applied before or after the default filters.
-    These are specified in the UFO lib.plist under the private key
+    These can be specified in the UFO lib.plist under the private key
     "com.github.googlei18n.ufo2ft.filters".
+    Alternatively the optional ``filters`` parameter can be used. This is a
+    list of filter instances (subclasses of BaseFilter) that overrides
+    those defined in the UFO lib. The list can be empty, meaning no custom
+    filters are run. If ``filters`` contain the special value ``...`` (i.e.
+    the actual ``ellipsis`` singleton, not the str literal '...'), then all
+    the filters from the UFO lib are loaded in its place. This allows to
+    insert additional filters before or after those already defined in the
+    UFO lib, as opposed to discard/replace them which is the default behavior
+    when ``...`` is absent.
     """
 
     def __init__(
@@ -37,7 +74,7 @@ class BasePreProcessor:
         layerName=None,
         skipExportGlyphs=None,
         filters=None,
-        **kwargs
+        **kwargs,
     ):
         self.ufo = ufo
         self.inplace = inplace
@@ -46,11 +83,10 @@ class BasePreProcessor:
             ufo, layerName, copy=not inplace, skipExportGlyphs=skipExportGlyphs
         )
         self.defaultFilters = self.initDefaultFilters(**kwargs)
-        if filters is None:
-            self.preFilters, self.postFilters = loadFilters(ufo)
-        else:
-            self.preFilters = [f for f in filters if f.pre]
-            self.postFilters = [f for f in filters if not f.pre]
+
+        filters = _load_custom_filters(ufo, filters)
+        self.preFilters = [f for f in filters if f.pre]
+        self.postFilters = [f for f in filters if not f.pre]
 
     def initDefaultFilters(self, **kwargs):
         return []  # pragma: no cover
@@ -260,18 +296,9 @@ class TTFInterpolatablePreProcessor:
             self.defaultFilters.append([])
             _init_explode_color_layer_glyphs_filter(ufo, self.defaultFilters[-1])
 
-        self.preFilters, self.postFilters = [], []
-        if filters is None:
-            for ufo in ufos:
-                pre, post = loadFilters(ufo)
-                self.preFilters.append(pre)
-                self.postFilters.append(post)
-        else:
-            pre = [f for f in filters if f.pre]
-            post = [f for f in filters if not f.pre]
-            for _ in ufos:
-                self.preFilters.append(pre)
-                self.postFilters.append(post)
+        filterses = [_load_custom_filters(ufo, filters) for ufo in ufos]
+        self.preFilters = [[f for f in filters if f.pre] for filters in filterses]
+        self.postFilters = [[f for f in filters if not f.pre] for filters in filterses]
 
     def process(self):
         from cu2qu.ufo import fonts_to_quadratic


Test patch:
diff --git a/tests/featureCompiler_test.py b/tests/featureCompiler_test.py
index f2d728f..56d88b8 100644
--- a/tests/featureCompiler_test.py
+++ b/tests/featureCompiler_test.py
@@ -78,6 +78,13 @@ class ParseLayoutFeaturesTest:
         assert "change the file name in the include" in caplog.text
 
 
+class DummyFeatureWriter:
+    tableTag = "GPOS"
+
+    def write(self, font, feaFile, compiler=None):
+        pass
+
+
 class FeatureCompilerTest:
     def test_ttFont(self, FontClass):
         ufo = FontClass()
@@ -174,6 +181,24 @@ class FeatureCompilerTest:
         assert isinstance(compiler.featureWriters[0], KernFeatureWriter)
         assert "GPOS" in ttFont
 
+    def test_loadFeatureWriters_from_both_UFO_lib_and_argument(self, FontClass):
+        ufo = FontClass()
+        ufo.lib[FEATURE_WRITERS_KEY] = [{"class": "KernFeatureWriter"}]
+        compiler = FeatureCompiler(ufo, featureWriters=[..., DummyFeatureWriter])
+
+        assert len(compiler.featureWriters) == 2
+        assert isinstance(compiler.featureWriters[0], KernFeatureWriter)
+        assert isinstance(compiler.featureWriters[1], DummyFeatureWriter)
+
+    def test_loadFeatureWriters_from_both_defaults_and_argument(self, FontClass):
+        ufo = FontClass()
+        compiler = FeatureCompiler(ufo, featureWriters=[DummyFeatureWriter, ...])
+
+        assert len(compiler.featureWriters) == 1 + len(
+            FeatureCompiler.defaultFeatureWriters
+        )
+        assert isinstance(compiler.featureWriters[0], DummyFeatureWriter)
+
     def test_GSUB_writers_run_first(self, FontClass):
         class FooFeatureWriter(BaseFeatureWriter):
 
diff --git a/tests/preProcessor_test.py b/tests/preProcessor_test.py
index 328b668..11196f8 100644
--- a/tests/preProcessor_test.py
+++ b/tests/preProcessor_test.py
@@ -116,6 +116,21 @@ class TTFPreProcessorTest:
         # filter2, before overlaps were removed in a post-filter filter1
         assert len(glyphSets0["d"].components) == 0
 
+    def test_custom_filters_in_both_lib_and_argument_with_ellipsis(self, FontClass):
+        from ufo2ft.filters import TransformationsFilter
+
+        ufo = FontClass(getpath("TestFont.ufo"))
+        ufo.lib[FILTERS_KEY] = [
+            {"name": "transformations", "kwargs": {"OffsetX": 10}, "pre": True}
+        ]
+
+        glyphSet = TTFPreProcessor(
+            ufo, filters=[..., TransformationsFilter(OffsetY=-10)]
+        ).process()
+
+        a = glyphSet["a"]
+        assert (a[0][0].x, a[0][0].y) == (ufo["a"][0][0].x + 10, ufo["a"][0][0].y - 10)
+
 
 class TTFInterpolatablePreProcessorTest:
     def test_no_inplace(self, FontClass):
@@ -205,6 +220,34 @@ class TTFInterpolatablePreProcessorTest:
         # filter2, before overlaps were removed in a post-filter filter1
         assert len(glyphSets[0]["d"].components) == 0
 
+    def test_custom_filters_in_both_lib_and_argument_with_ellipsis(self, FontClass):
+        from ufo2ft.filters import TransformationsFilter
+
+        ufo1 = FontClass(getpath("TestFont.ufo"))
+        ufo1.lib[FILTERS_KEY] = [
+            {"name": "transformations", "kwargs": {"OffsetX": 10}, "pre": True}
+        ]
+
+        ufo2 = FontClass(getpath("TestFont.ufo"))
+        ufo2.lib[FILTERS_KEY] = [
+            {"name": "transformations", "kwargs": {"OffsetX": 20}, "pre": True}
+        ]
+
+        glyphSets = TTFInterpolatablePreProcessor(
+            [ufo1, ufo2], filters=[..., TransformationsFilter(OffsetY=-10)]
+        ).process()
+
+        a1 = glyphSets[0]["a"]
+        assert (a1[0][0].x, a1[0][0].y) == (
+            ufo1["a"][0][0].x + 10,
+            ufo1["a"][0][0].y - 10,
+        )
+        a2 = glyphSets[1]["a"]
+        assert (a2[0][0].x, a2[0][0].y) == (
+            ufo2["a"][0][0].x + 20,
+            ufo2["a"][0][0].y - 10,
+        )
+
 
 class SkipExportGlyphsTest:
     def test_skip_export_glyphs_filter(self, FontClass):