Update COLR build fns per review feedback

Lib/fontTools/colorLib/builder.py

@@ -53,16 +53,17 @@ _ColorGlyphsV0Dict = Dict[str, Sequence[Tuple[str, int]]]
 
 MAX_PAINT_COLR_LAYER_COUNT = 255
 _DEFAULT_ALPHA = VariableFloat(1.0)
+_MAX_REUSE_LEN = 32
 
 
 def _beforeBuildPaintRadialGradient(paint, source):
     # normalize input types (which may or may not specify a varIdx)
     x0 = convertTupleClass(VariableFloat, source["x0"])
-    y0 = convertTupleClass(VariableFloat, source.get("y0", 0.0))
-    r0 = convertTupleClass(VariableFloat, source.get("r0", 0.0))
-    x1 = convertTupleClass(VariableFloat, source.get("x1", 0.0))
-    y1 = convertTupleClass(VariableFloat, source.get("y1", 0.0))
-    r1 = convertTupleClass(VariableFloat, source.get("r1", 0.0))
+    y0 = convertTupleClass(VariableFloat, source["y0"])
+    r0 = convertTupleClass(VariableFloat, source["r0"])
+    x1 = convertTupleClass(VariableFloat, source["x1"])
+    y1 = convertTupleClass(VariableFloat, source["y1"])
+    r1 = convertTupleClass(VariableFloat, source["r1"])
 
     # TODO apparently no builder_test confirms this works (?)
 
@@ -373,11 +374,12 @@ def _split_color_glyphs_by_version(
 def _reuse_ranges(num_layers: int) -> Generator[Tuple[int, int], None, None]:
     # TODO feels like something itertools might have already
     for lbound in range(num_layers):
-        # TODO may want a max length to limit scope of search
         # Reuse of very large #s of layers is relatively unlikely
         # +2: we want sequences of at least 2
         # otData handles single-record duplication
-        for ubound in range(lbound + 2, num_layers + 1):
+        for ubound in range(
+            lbound + 2, min(num_layers + 1, lbound + 2 + _MAX_REUSE_LEN)
+        ):
             yield (lbound, ubound)
 
 
@@ -448,6 +450,10 @@ class LayerV1ListBuilder:
         # Convert maps seqs or whatever into typed objects
         layers = [self.buildPaint(l) for l in layers]
 
+        # No reason to have a colr layers with just one entry
+        if len(layers) == 1:
+            return layers[0], {}
+
         # Look for reuse, with preference to longer sequences
         # This may make the layer list smaller
         found_reuse = True
@@ -474,6 +480,7 @@ class LayerV1ListBuilder:
                 break
 
         # The layer list is now final; if it's too big we need to tree it
+        is_tree = len(layers) > MAX_PAINT_COLR_LAYER_COUNT
         layers = _build_n_ary_tree(layers, n=MAX_PAINT_COLR_LAYER_COUNT)
 
         # We now have a tree of sequences with Paint leaves.
@@ -494,12 +501,13 @@ class LayerV1ListBuilder:
         paint.FirstLayerIndex = len(self.layers)
         self.layers.extend(layers)
 
-        # Register our parts for reuse
-        # TODO what if we made ourselves a lovely little tree
-        for lbound, ubound in _reuse_ranges(len(layers)):
-            self.reusePool[self._as_tuple(layers[lbound:ubound])] = (
-                lbound + paint.FirstLayerIndex
-            )
+        # Register our parts for reuse provided we aren't a tree
+        # If we are a tree the leaves registered for reuse and that will suffice
+        if not is_tree:
+            for lbound, ubound in _reuse_ranges(len(layers)):
+                self.reusePool[self._as_tuple(layers[lbound:ubound])] = (
+                    lbound + paint.FirstLayerIndex
+                )
 
         # we've fully built dest; empty source prevents generalized build from kicking in
         return paint, {}

Lib/fontTools/colorLib/table_builder.py

@@ -12,7 +12,6 @@ from fontTools.ttLib.tables.otBase import (
 )
 from fontTools.ttLib.tables.otConverters import (
     ComputedInt,
-    GlyphID,
     SimpleValue,
     Struct,
     Short,
@@ -23,18 +22,6 @@ from fontTools.ttLib.tables.otConverters import (
 )
 
 
-def _to_glyph_id(value):
-    assert isinstance(value, str), "Expected a glyph name"
-    return value
-
-
-_CONVERTER_OVERRIDES = {
-    Short: int,
-    UShort: int,
-    GlyphID: _to_glyph_id,
-}
-
-
 class BuildCallback(enum.Enum):
     """Keyed on (BEFORE_BUILD, class[, Format if available]).
     Receives (dest, source).
@@ -107,10 +94,10 @@ class TableBuilder:
         self._callbackTable = callbackTable
 
     def _convert(self, dest, field, converter, value):
-        converter = _CONVERTER_OVERRIDES.get(type(converter), converter)
-
-        enumClass = getattr(converter, "enumClass", None)
         tupleClass = getattr(converter, "tupleClass", None)
+        enumClass = getattr(converter, "enumClass", None)
+        simpleValueClass = getattr(converter, "valueClass", None)
+
         if tupleClass:
             value = convertTupleClass(tupleClass, value)
 
@@ -125,6 +112,9 @@ class TableBuilder:
             else:
                 value = enumClass(value)
 
+        elif simpleValueClass:
+            value = simpleValueClass(value)
+
         elif isinstance(converter, Struct):
             if converter.repeat:
                 if _isNonStrSequence(value):

Lib/fontTools/ttLib/tables/otConverters.py

@@ -216,6 +216,8 @@ class SimpleValue(BaseConverter):
 		return self.fromString(attrs["value"])
 
 class IntValue(SimpleValue):
+	valueClass = int
+
 	@staticmethod
 	def fromString(value):
 		return int(value, 0)
@@ -295,6 +297,7 @@ class Tag(SimpleValue):
 		writer.writeTag(value)
 
 class GlyphID(SimpleValue):
+	valueClass = str
 	staticSize = 2
 	typecode = "H"
 	def readArray(self, reader, font, tableDict, count):
@@ -334,6 +337,7 @@ class NameID(UShort):
 
 
 class FloatValue(SimpleValue):
+	valueClass = float
 	@staticmethod
 	def fromString(value):
 		return float(value)

Tests/colorLib/builder_test.py

@@ -269,7 +269,9 @@ def test_buildPaintSolid_Alpha():
 
 
 def test_buildPaintSolid_Variable():
-    p = _buildPaint((ot.PaintFormat.PaintSolid, (3, builder.VariableFloat(0.5, varIdx=2))))
+    p = _buildPaint(
+        (ot.PaintFormat.PaintSolid, (3, builder.VariableFloat(0.5, varIdx=2)))
+    )
     assert p.Format == ot.PaintFormat.PaintSolid
     assert p.Color.PaletteIndex == 3
     assert p.Color.Alpha.value == 0.5
@@ -1374,6 +1376,29 @@ class BuildCOLRTest(object):
         assert isinstance(colr.table, ot.COLR)
         assert colr.table.VarStore is None
 
+    def test_paint_one_colr_layers(self):
+        # A set of one layers should flip to just that layer
+        colr = builder.buildCOLR(
+            {
+                "a": (
+                    ot.PaintFormat.PaintColrLayers,
+                    [
+                        (
+                            ot.PaintFormat.PaintGlyph,
+                            (ot.PaintFormat.PaintSolid, 0),
+                            "b",
+                        ),
+                    ],
+                )
+            },
+        )
+
+        assert len(colr.table.LayerV1List.Paint) == 0, "PaintColrLayers should be gone"
+        assert colr.table.BaseGlyphV1List.BaseGlyphCount == 1
+        paint = colr.table.BaseGlyphV1List.BaseGlyphV1Record[0].Paint
+        assert paint.Format == ot.PaintFormat.PaintGlyph
+        assert paint.Paint.Format == ot.PaintFormat.PaintSolid
+
 
 class TrickyRadialGradientTest:
     @staticmethod