From aa6912fbd47aa65da4b30ae9222a9c6a7b9d4f2c Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 12 Jul 2023 17:46:54 +0100 Subject: [PATCH 1/2] designspace_gen_test: confirm source ufo contains the named intermediate layer! Trying to debug https://github.com/googlefonts/glyphsLib/issues/925 Follow-up from #928 and #851.. apparently I made it worse. --- tests/builder/designspace_gen_test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/builder/designspace_gen_test.py b/tests/builder/designspace_gen_test.py index 565db6d83..7f2f65f09 100644 --- a/tests/builder/designspace_gen_test.py +++ b/tests/builder/designspace_gen_test.py @@ -169,8 +169,8 @@ def test_designspace_generation_brace_layers(datadir, filename, ufo_module): ("Weight", 100, 100, 700, [(100, 100.0), (700, 1000.0)]), ] - for (fname, layerName, name), (exp_fname, exp_layerName, exp_name) in zip( - [(s.filename, s.layerName, s.name) for s in designspace.sources], + for (fname, layerName, name, ufo), (exp_fname, exp_layerName, exp_name) in zip( + [(s.filename, s.layerName, s.name, s.font) for s in designspace.sources], [ ("NewFont-Light.ufo", None, "New Font Light"), ("NewFont-Light.ufo", "{75}", "New Font Light {75}"), @@ -189,6 +189,7 @@ def test_designspace_generation_brace_layers(datadir, filename, ufo_module): # https://github.com/googlefonts/glyphsLib/issues/851 if exp_layerName is not None: assert fnmatch.fnmatch(layerName, exp_layerName) + assert layerName in ufo.layers else: assert layerName is None assert fnmatch.fnmatch(name, exp_name) From 4f2de546a9609083774e953020ab9907543ea952 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 12 Jul 2023 18:05:12 +0100 Subject: [PATCH 2/2] consistently name UFO and DS source layer for the same brace glyphs Fixes #925 --- Lib/glyphsLib/builder/layers.py | 11 ++--------- Lib/glyphsLib/builder/sources.py | 2 +- Lib/glyphsLib/classes.py | 15 +++++++++++++++ 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/Lib/glyphsLib/builder/layers.py b/Lib/glyphsLib/builder/layers.py index 824c24cbc..ab8e5a2a0 100644 --- a/Lib/glyphsLib/builder/layers.py +++ b/Lib/glyphsLib/builder/layers.py @@ -36,15 +36,8 @@ 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'])}}}" + elif layer._is_brace_layer(): + layer_name = layer._brace_layer_name() if layer.associatedMasterId == layer.layerId: ufo_layer = ufo_font.layers.defaultLayer diff --git a/Lib/glyphsLib/builder/sources.py b/Lib/glyphsLib/builder/sources.py index 9b2042f13..dd770eeb4 100644 --- a/Lib/glyphsLib/builder/sources.py +++ b/Lib/glyphsLib/builder/sources.py @@ -156,7 +156,7 @@ def _to_designspace_source_layer(self): for glyph in self.font.glyphs: for layer in glyph.layers: if layer._is_brace_layer(): - key = (layer.name, tuple(layer._brace_coordinates())) + key = (layer._brace_layer_name(), tuple(layer._brace_coordinates())) layer_to_master_ids[key].add(layer.associatedMasterId) layer_to_glyph_names[key].append(glyph.name) diff --git a/Lib/glyphsLib/classes.py b/Lib/glyphsLib/classes.py index cff44bdf7..643b2fedf 100755 --- a/Lib/glyphsLib/classes.py +++ b/Lib/glyphsLib/classes.py @@ -3993,6 +3993,21 @@ def _brace_coordinates(self): coordinates = name[name.index("{") + 1 : name.index("}")] return [float(c) for c in coordinates.split(",")] + def _brace_layer_name(self): + # 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 + # TODO: Figure out a better API for layer.name vs layer.nameUI() mess... + if "coordinates" in self.attributes: + # Glyphs 3 + return f"{{{', '.join(str(v) for v in self.attributes['coordinates'])}}}" + # Glyphs 2 + return self.name + COLOR_PALETTE_LAYER_RE = re.compile(r"^Color (?P\*|\d+)$") def _is_color_palette_layer(self):