Merge pull request #625 from googlefonts/fix-filters-component-order

filters: sort glyphs by decreasing component depth to avoid order-dependent issues

Lib/ufo2ft/filters/base.py

@@ -3,7 +3,7 @@
 
 from fontTools.misc.loggingTools import Timer
 
-from ufo2ft.util import _GlyphSet, _LazyFontName
+from ufo2ft.util import _GlyphSet, _LazyFontName, getMaxComponentDepth
 
 logger = logging.getLogger(__name__)
 
@@ -186,9 +186,16 @@ def __call__(self, font, glyphSet=None):
         include = self.include
         modified = context.modified
 
+        # process composite glyphs in decreasing component depth order (i.e. composites
+        # with more deeply nested components before shallower ones) to avoid
+        # order-dependent interferences while filtering glyphs with nested components
+        # https://github.com/googlefonts/ufo2ft/issues/621
+        orderedGlyphs = sorted(
+            glyphSet.keys(), key=lambda g: -getMaxComponentDepth(glyphSet[g], glyphSet)
+        )
+
         with Timer() as t:
-            # we sort the glyph names to make loop deterministic
-            for glyphName in sorted(glyphSet.keys()):
+            for glyphName in orderedGlyphs:
                 if glyphName in modified:
                     continue
                 glyph = glyphSet[glyphName]

Lib/ufo2ft/filters/propagateAnchors.py

@@ -39,11 +39,16 @@ def filter(self, glyph):
         if not glyph.components:
             return False
         before = len(glyph.anchors)
-        _propagate_glyph_anchors(self.context.glyphSet, glyph, self.context.processed)
+        _propagate_glyph_anchors(
+            self.context.glyphSet,
+            glyph,
+            self.context.processed,
+            self.context.modified,
+        )
         return len(glyph.anchors) > before
 
 
-def _propagate_glyph_anchors(glyphSet, composite, processed):
+def _propagate_glyph_anchors(glyphSet, composite, processed, modified):
     """
     Propagate anchors from base glyphs to a given composite
     glyph, and to all composite glyphs used in between.
@@ -69,7 +74,7 @@ def _propagate_glyph_anchors(glyphSet, composite, processed):
                 "in glyph {}".format(component.baseGlyph, composite.name)
             )
         else:
-            _propagate_glyph_anchors(glyphSet, glyph, processed)
+            _propagate_glyph_anchors(glyphSet, glyph, processed, modified)
             if any(a.name.startswith("_") for a in glyph.anchors):
                 mark_components.append(component)
             else:
@@ -110,6 +115,9 @@ def _propagate_glyph_anchors(glyphSet, composite, processed):
             # fontParts API
             composite.appendAnchor(name, (x, y))
 
+    if to_add:
+        modified.add(composite.name)
+
 
 def _get_anchor_data(anchor_data, glyphSet, components, anchor_name):
     """Get data for an anchor from a list of components."""

Lib/ufo2ft/util.py

@@ -546,3 +546,32 @@ def ensure_all_sources_have_names(doc: DesignSpaceDocument) -> None:
             source.name = f"temp_master.{counter}"
             counter += 1
         used_names.add(source.name)
+
+
+def getMaxComponentDepth(glyph, glyphSet, maxComponentDepth=0):
+    """Return the height of a composite glyph's tree of components.
+
+    This is equal to the depth of its deepest node, where the depth
+    means the number of edges (component references) from the node
+    to the tree's root.
+
+    For glyphs that contain no components, only contours, this is 0.
+    Composite glyphs have max component depth of 1 or greater.
+    """
+    if not glyph.components:
+        return maxComponentDepth
+
+    maxComponentDepth += 1
+
+    initialMaxComponentDepth = maxComponentDepth
+    for component in glyph.components:
+        try:
+            baseGlyph = glyphSet[component.baseGlyph]
+        except KeyError:
+            continue
+        componentDepth = getMaxComponentDepth(
+            baseGlyph, glyphSet, initialMaxComponentDepth
+        )
+        maxComponentDepth = max(maxComponentDepth, componentDepth)
+
+    return maxComponentDepth

tests/filters/decomposeTransformedComponents_test.py

@@ -21,7 +21,7 @@ def test_transformed_components(self, FontClass):
         pen = c.getPen()
         pen.addComponent("six.lf", (1, 0, 0, 1, 0, 0))
 
-        # nine.lf has one transformed component of a componenent
+        # nine.lf has one transformed component of a component
         b = ufo.newGlyph("nine.lf")
         b.width = 300
         pen = b.getPen()

tests/filters/propagateAnchors_test.py

@@ -123,6 +123,35 @@ def font(request, FontClass):
     return font
 
 
+EXPECTED = {
+    # single component glyph
+    "a-cyr": ([("bottom", 175, 0), ("top", 175, 300)], {"a-cyr"}),
+    # two component glyph
+    "adieresis": ([("bottom", 175, 0), ("top", 175, 480)], {"adieresis"}),
+    # one anchor, two component glyph
+    "amacron": ([("top", 176, 481), ("bottom", 175, 0)], {"amacron"}),
+    # three component glyph
+    "adieresismacron": ([("bottom", 175, 0), ("top", 175, 660)], {"adieresismacron"}),
+    # nested component glyph
+    "amacrondieresis": (
+        [("bottom", 175, 0), ("top", 175, 660)],
+        # 'amacron' is used as component by 'amacrondieresis' hence it is modified
+        # as well...
+        {"amacrondieresis", "amacron"},
+    ),
+    # ligature glyph
+    "a_a": (
+        [
+            ("bottom_1", 175, 0),
+            ("bottom_2", 525, 0),
+            ("top_1", 175, 300),
+            ("top_2", 525, 300),
+        ],
+        {"a_a"},
+    ),
+}
+
+
 class PropagateAnchorsFilterTest:
     def test_empty_glyph(self, font):
         philter = PropagateAnchorsFilter(include={"space"})
@@ -132,72 +161,21 @@ def test_contour_glyph(self, font):
         philter = PropagateAnchorsFilter(include={"a"})
         assert not philter(font)
 
-    def test_single_component_glyph(self, font):
-        philter = PropagateAnchorsFilter(include={"a-cyr"})
-        assert philter(font) == {"a-cyr"}
-        assert [(a.name, a.x, a.y) for a in font["a-cyr"].anchors] == [
-            ("bottom", 175, 0),
-            ("top", 175, 300),
-        ]
-
-    def test_two_component_glyph(self, font):
-        name = "adieresis"
-        philter = PropagateAnchorsFilter(include={name})
-        assert philter(font) == {name}
-        assert [(a.name, a.x, a.y) for a in font[name].anchors] == [
-            ("bottom", 175, 0),
-            ("top", 175, 480),
-        ]
-
-    def test_one_anchor_two_component_glyph(self, font):
-        name = "amacron"
-        philter = PropagateAnchorsFilter(include={name})
-        assert philter(font) == {name}
-        assert [(a.name, a.x, a.y) for a in font[name].anchors] == [
-            ("top", 176, 481),
-            ("bottom", 175, 0),
-        ]
-
-    def test_three_component_glyph(self, font):
-        name = "adieresismacron"
+    @pytest.mark.parametrize("name", list(EXPECTED))
+    def test_include_one_glyph_at_a_time(self, font, name):
         philter = PropagateAnchorsFilter(include={name})
-        assert philter(font) == {name}
-        assert [(a.name, a.x, a.y) for a in font[name].anchors] == [
-            ("bottom", 175, 0),
-            ("top", 175, 660),
-        ]
-
-    def test_nested_component_glyph(self, font):
-        name = "amacrondieresis"
-        philter = PropagateAnchorsFilter(include={name})
-        assert philter(font) == {name}
-        assert [(a.name, a.x, a.y) for a in font[name].anchors] == [
-            ("bottom", 175, 0),
-            ("top", 175, 660),
-        ]
-
-    def test_ligature_glyph(self, font):
-        name = "a_a"
-        philter = PropagateAnchorsFilter(include={name})
-        assert philter(font) == {name}
-        assert [(a.name, a.x, a.y) for a in font[name].anchors] == [
-            ("bottom_1", 175, 0),
-            ("bottom_2", 525, 0),
-            ("top_1", 175, 300),
-            ("top_2", 525, 300),
-        ]
+        modified = philter(font)
+
+        expected_anchors, expected_modified = EXPECTED[name]
+        assert modified == expected_modified
+        assert [(a.name, a.x, a.y) for a in font[name].anchors] == expected_anchors
 
     def test_whole_font(self, font):
         philter = PropagateAnchorsFilter()
         modified = philter(font)
-        assert modified == {
-            "a-cyr",
-            "amacron",
-            "adieresis",
-            "adieresismacron",
-            "amacrondieresis",
-            "a_a",
-        }
+        assert modified == set(EXPECTED)
+        for name, (expected_anchors, _) in EXPECTED.items():
+            assert [(a.name, a.x, a.y) for a in font[name].anchors] == expected_anchors
 
     def test_fail_during_anchor_propagation(self, font):
         name = "emacron"