Merge pull request #928 from googlefonts/fix-brace-layer-name-v3

generate intermediate layer name from coordinates when converting to UFO

Lib/glyphsLib/builder/layers.py

@@ -36,6 +36,15 @@ def to_ufo_layer(self, glyph, layer):
     # Give color layers better names
     if layer._is_color_palette_layer():
         layer_name = f"color.{layer._color_palette_index()}"
+    elif "coordinates" in layer.attributes:
+        # For Glyphs 3's intermediate (formerly 'brace') layers we must generate the
+        # name from the attributes (as it's shown in Glyphs.app UI) and discard
+        # the layer's actual 'name' as found in the source file, which is usually just
+        # the unique date-time when a layer was first created.
+        # Using the generated name ensures that all the intermediate glyph instances
+        # at a given location end up in the same UFO source layer, see:
+        # https://github.com/googlefonts/glyphsLib/issues/851
+        layer_name = f"{{{', '.join(str(v) for v in layer.attributes['coordinates'])}}}"
 
     if layer.associatedMasterId == layer.layerId:
         ufo_layer = ufo_font.layers.defaultLayer

tests/builder/designspace_gen_test.py

@@ -15,6 +15,7 @@
 
 
 import os
+import fnmatch
 from fontTools.designspaceLib import DesignSpaceDocument
 from xmldiff import main, formatting
 from fontTools.varLib import FEAVAR_FEATURETAG_LIB_KEY
@@ -168,17 +169,29 @@ def test_designspace_generation_brace_layers(datadir, filename, ufo_module):
         ("Weight", 100, 100, 700, [(100, 100.0), (700, 1000.0)]),
     ]
 
-    source_order = [(s.filename, s.layerName, s.name) for s in designspace.sources]
-    assert source_order == [
-        ("NewFont-Light.ufo", None, "New Font Light"),
-        ("NewFont-Light.ufo", "{75}", "New Font Light {75}"),
-        ("NewFont-Bold.ufo", None, "New Font Bold"),
-        ("NewFont-Bold.ufo", "{75}", "New Font Bold {75}"),
-        ("NewFont-Bold.ufo", "Test2 {90.5, 500}", "New Font Bold Test2 {90.5, 500}"),
-        ("NewFont-Bold.ufo", "Test1 {90.5, 600}", "New Font Bold Test1 {90.5, 600}"),
-        ("NewFont-CondensedLight.ufo", None, "New Font Condensed Light"),
-        ("NewFont-CondensedBold.ufo", None, "New Font Condensed Bold"),
-    ]
+    for (fname, layerName, name), (exp_fname, exp_layerName, exp_name) in zip(
+        [(s.filename, s.layerName, s.name) for s in designspace.sources],
+        [
+            ("NewFont-Light.ufo", None, "New Font Light"),
+            ("NewFont-Light.ufo", "{75}", "New Font Light {75}"),
+            ("NewFont-Bold.ufo", None, "New Font Bold"),
+            ("NewFont-Bold.ufo", "{75}", "New Font Bold {75}"),
+            ("NewFont-Bold.ufo", "*{90.5, 500}", "New Font Bold *{90.5, 500}"),
+            ("NewFont-Bold.ufo", "*{90.5, 600}", "New Font Bold *{90.5, 600}"),
+            ("NewFont-CondensedLight.ufo", None, "New Font Condensed Light"),
+            ("NewFont-CondensedBold.ufo", None, "New Font Condensed Bold"),
+        ],
+    ):
+        assert fname == exp_fname
+        # the brace layer name for Glyphs3 is automatically generated from its
+        # coordinates and unlike in Glyphs2 won't contain any extra prefix, hence
+        # we only 'fnmatch' it.
+        # https://github.com/googlefonts/glyphsLib/issues/851
+        if exp_layerName is not None:
+            assert fnmatch.fnmatch(layerName, exp_layerName)
+        else:
+            assert layerName is None
+        assert fnmatch.fnmatch(name, exp_name)
 
     # Check that all sources have a font object attached and sources with the same
     # filename have the same font object attached.

tests/data/BraceTestFontV3.glyphs

@@ -341,8 +341,7 @@ width = 300;
 associatedMasterId = "1034EC4A-9832-4D17-A75A-2B17BF7C4AA6";
 attr = {
 coordinates = (
-75,
-0
+75
 );
 };
 layerId = "1ABBBA7E-D0DC-49B1-AC13-AB42C8F97BA5";
@@ -451,8 +450,7 @@ width = 300;
 associatedMasterId = "ED9A0AB1-C69E-43FC-AC45-FCA9085EE0B1";
 attr = {
 coordinates = (
-75,
-0
+75
 );
 };
 layerId = "FD486ED6-D76D-490C-B562-C4A027492D0B";