From 97301b917de05b4c72803b92f959d3cdced4cacd Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaled@aliftype.com>
Date: Sun, 16 Jan 2022 18:57:28 +0200
Subject: [PATCH] [builder] Fix color on master layer optimization

When a master layer has color attributes, we try to avoid creating a
duplicate color layer of it. This was broken if the layer had multiple
paths with with different color attributes each. Fix this by returning
the layer only when the new layer has the same number of paths and
components, and create a new layer otherwise.
---
 Lib/glyphsLib/builder/glyph.py | 14 ++++--
 tests/builder/builder_test.py  | 81 ++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/Lib/glyphsLib/builder/glyph.py b/Lib/glyphsLib/builder/glyph.py
index 41ff47b2c..70d5deba3 100644
--- a/Lib/glyphsLib/builder/glyph.py
+++ b/Lib/glyphsLib/builder/glyph.py
@@ -13,19 +13,20 @@
 # limitations under the License.
 
 
-import copy
 import itertools
 import logging
 
 import glyphsLib.glyphdata
-from .common import to_ufo_time, from_loose_ufo_time
+
+from .. import GSLayer
+from .builders import BRACKET_GLYPH_RE, BRACKET_GLYPH_SUFFIX_RE
+from .common import from_loose_ufo_time, to_ufo_time
 from .constants import (
     GLYPHLIB_PREFIX,
     GLYPHS_COLORS,
     PUBLIC_PREFIX,
     UFO2FT_COLOR_LAYER_MAPPING_KEY,
 )
-from .builders import BRACKET_GLYPH_RE, BRACKET_GLYPH_SUFFIX_RE
 
 logger = logging.getLogger(__name__)
 
@@ -37,9 +38,14 @@
 def _clone_layer(layer, paths=None, components=None):
     paths = paths if paths is not None else []
     components = components if components is not None else []
-    new_layer = copy.copy(layer)
+    if len(paths) == len(layer.paths) and len(components) == len(layer.components):
+        return layer
+    new_layer = GSLayer()
+    new_layer.associatedMasterId = layer.associatedMasterId
+    new_layer.parent = layer.parent
     new_layer.paths = paths
     new_layer.components = components
+    new_layer.attributes = layer.attributes
     return new_layer
 
 
diff --git a/tests/builder/builder_test.py b/tests/builder/builder_test.py
index 455afc5b7..63c7c7249 100644
--- a/tests/builder/builder_test.py
+++ b/tests/builder/builder_test.py
@@ -2248,6 +2248,87 @@ def test_glyph_color_layers_group_paths_nonconsecutive(self):
 
         assert "com.github.googlei18n.ufo2ft.colorLayerMapping" not in ufo["a"].lib
 
+    def test_glyph_color_layers_master_layer(self):
+        font = generate_minimal_font(format_version=3)
+        glyph = add_glyph(font, "a")
+
+        layer = glyph.layers[0]
+        layer.attributes["color"] = 1
+
+        for i in range(2):
+            path = GSPath()
+            path.nodes = [
+                GSNode(position=(i + 0, i + 0), nodetype="line"),
+                GSNode(position=(i + 100, i + 100), nodetype="line"),
+                GSNode(position=(i + 200, i + 200), nodetype="line"),
+                GSNode(position=(i + 300, i + 300), nodetype="line"),
+            ]
+            path.attributes["gradient"] = {
+                "colors": [[[0 + i, 0, 0, 255], 0], [[185 + i, 0, 0, 255], 1]],
+                "end": [0.2 + i, 0.3 + i],
+                "start": [0.4 + i, 0.09 + i],
+            }
+            layer.paths.append(path)
+
+        ds = self.to_designspace(font, minimal=True)
+        ufo = ds.sources[0].font
+        assert ufo.lib["com.github.googlei18n.ufo2ft.colorPalettes"] == [
+            [
+                (0.0, 0.0, 0.0, 1.0),
+                (0.7254901960784313, 0.0, 0.0, 1.0),
+                (0.00392156862745098, 0.0, 0.0, 1.0),
+                (0.7294117647058823, 0.0, 0.0, 1.0),
+            ]
+        ]
+        assert ufo.lib["com.github.googlei18n.ufo2ft.colorLayers"] == {
+            "a": {
+                "Format": 1,
+                "Layers": [
+                    {
+                        "Format": 10,
+                        "Glyph": "a.color0",
+                        "Paint": {
+                            "ColorLine": {
+                                "ColorStop": [
+                                    {"Alpha": 1.0, "PaletteIndex": 0, "StopOffset": 0},
+                                    {"Alpha": 1.0, "PaletteIndex": 1, "StopOffset": 1},
+                                ],
+                                "Extend": "pad",
+                            },
+                            "Format": 4,
+                            "x0": 120.0,
+                            "x1": 60.0,
+                            "x2": 183.0,
+                            "y0": 27.0,
+                            "y1": 90.0,
+                            "y2": 87.0,
+                        },
+                    },
+                    {
+                        "Format": 10,
+                        "Glyph": "a.color1",
+                        "Paint": {
+                            "ColorLine": {
+                                "ColorStop": [
+                                    {"Alpha": 1.0, "PaletteIndex": 2, "StopOffset": 0},
+                                    {"Alpha": 1.0, "PaletteIndex": 3, "StopOffset": 1},
+                                ],
+                                "Extend": "pad",
+                            },
+                            "Format": 4,
+                            "x0": 421.0,
+                            "x1": 361.0,
+                            "x2": 484.0,
+                            "y0": 328.0,
+                            "y1": 391.0,
+                            "y2": 388.0,
+                        },
+                    },
+                ],
+            }
+        }
+        assert "com.github.googlei18n.ufo2ft.colorLayerMapping" not in ufo["a"].lib
+
     def test_master_with_light_weight_but_thin_name(self):
         font = generate_minimal_font()
         master = font.masters[0]