From 5241cc12f6a09e1b4373d850d25076cd6f5ac4f0 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 10:48:36 +0200 Subject: [PATCH 1/7] Add test case that exposes redundant layer creation --- test-py/test_backend_designspace.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test-py/test_backend_designspace.py b/test-py/test_backend_designspace.py index 05ee0539d..290d64585 100644 --- a/test-py/test_backend_designspace.py +++ b/test-py/test_backend_designspace.py @@ -249,6 +249,27 @@ async def test_newFileSystemBackend(tmpdir, testFont): assert glyph == referenceGlyph +async def test_writeCorrectLayers(tmpdir, testFont): + tmpdir = pathlib.Path(tmpdir) + dsPath = tmpdir / "Test.designspace" + font = newFileSystemBackend(dsPath) + + axes = await testFont.getGlobalAxes() + await font.putGlobalAxes(axes) + glyphMap = await testFont.getGlyphMap() + glyph = await testFont.getGlyph("A") + await font.putGlyph("A", glyph, glyphMap["A"]) + await font.putGlyph("A.alt", glyph, []) + + assert [ + "fontinfo.plist", + "glyphs", + "glyphs.M_utatorS_ansL_ightC_ondensed_support", + "layercontents.plist", + "metainfo.plist", + ] == fileNamesFromDir(tmpdir / "Test_Regular.ufo") + + def fileNamesFromDir(path): return sorted(p.name for p in path.iterdir()) From 8326f208939ab7490a2b62adfcfe6b82f846bead Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 10:49:17 +0200 Subject: [PATCH 2/7] Pass glyphName down to _newUFOLayer --- src/fontra/backends/designspace.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/fontra/backends/designspace.py b/src/fontra/backends/designspace.py index 2e1906912..a39908b83 100644 --- a/src/fontra/backends/designspace.py +++ b/src/fontra/backends/designspace.py @@ -321,7 +321,7 @@ async def putGlyph(self, glyphName, glyph, unicodes): localSources = [] for source in glyph.sources: sourceInfo = self._prepareUFOSourceLayer( - source, localDefaultLocation, revLayerNameMapping + glyphName, source, localDefaultLocation, revLayerNameMapping ) if sourceInfo.sourceName != source.name: sourceNameMapping[sourceInfo.sourceName] = source.name @@ -349,7 +349,9 @@ async def putGlyph(self, glyphName, glyph, unicodes): if ufoLayer is None: # This layer is not used by any source and we haven't seen it # before. Let's create a new layer in the default UFO. - ufoLayer = self._newUFOLayer(self.defaultUFOLayer.path, layerName) + ufoLayer = self._newUFOLayer( + glyphName, self.defaultUFOLayer.path, layerName + ) if ufoLayer.fontraLayerName != layerName: layerNameMapping[ufoLayer.fontraLayerName] = layerName layerName = ufoLayer.fontraLayerName @@ -397,7 +399,9 @@ async def putGlyph(self, glyphName, glyph, unicodes): self.savedGlyphModificationTimes[glyphName] = modTimes - def _prepareUFOSourceLayer(self, source, localDefaultLocation, revLayerNameMapping): + def _prepareUFOSourceLayer( + self, glyphName, source, localDefaultLocation, revLayerNameMapping + ): sparseLocalLocation = { name: source.location[name] for name, value in localDefaultLocation.items() @@ -412,7 +416,7 @@ def _prepareUFOSourceLayer(self, source, localDefaultLocation, revLayerNameMappi locationTuple=tuplifyLocation(globalLocation) ) if dsSource is None: - dsSource = self._createDSSource(source, globalLocation) + dsSource = self._createDSSource(glyphName, source, globalLocation) if sparseLocalLocation: ufoLayer = self.ufoLayers.findItem( @@ -423,7 +427,7 @@ def _prepareUFOSourceLayer(self, source, localDefaultLocation, revLayerNameMappi if ufoLayer is None: ufoPath = dsSource.layer.path - ufoLayer = self._newUFOLayer(ufoPath, source.layerName) + ufoLayer = self._newUFOLayer(glyphName, ufoPath, source.layerName) ufoLayerName = ufoLayer.name else: ufoLayerName = ufoLayer.name @@ -446,7 +450,7 @@ def _prepareUFOSourceLayer(self, source, localDefaultLocation, revLayerNameMappi localSourceDict=localSourceDict, ) - def _createDSSource(self, source, globalLocation): + def _createDSSource(self, glyphName, source, globalLocation): manager = self.ufoManager atPole, notAtPole = splitLocationByPolePosition( globalLocation, self.axisPolePositions @@ -487,7 +491,9 @@ def _createDSSource(self, source, globalLocation): poleDSSource = self.defaultDSSource assert poleDSSource is not None ufoPath = poleDSSource.layer.path - ufoLayer = self._newUFOLayer(poleDSSource.layer.path, source.layerName) + ufoLayer = self._newUFOLayer( + glyphName, poleDSSource.layer.path, source.layerName + ) ufoLayerName = ufoLayer.name self.dsDoc.addSourceDescriptor( @@ -507,7 +513,7 @@ def _createDSSource(self, source, globalLocation): return dsSource - def _newUFOLayer(self, ufoPath, suggestedLayerName): + def _newUFOLayer(self, glyphName, ufoPath, suggestedLayerName): reader = self.ufoManager.getReader(ufoPath) makeUniqueName = uniqueNameMaker(reader.getLayerNames()) ufoLayerName = makeUniqueName(suggestedLayerName) From 713931f3276ceb9caf7a70b42454c40d41a2cc51 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 11:04:48 +0200 Subject: [PATCH 3/7] Only add a number to the layer name if the glyph actually exists in the layer --- src/fontra/backends/designspace.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/fontra/backends/designspace.py b/src/fontra/backends/designspace.py index a39908b83..f8f339bb8 100644 --- a/src/fontra/backends/designspace.py +++ b/src/fontra/backends/designspace.py @@ -515,10 +515,14 @@ def _createDSSource(self, glyphName, source, globalLocation): def _newUFOLayer(self, glyphName, ufoPath, suggestedLayerName): reader = self.ufoManager.getReader(ufoPath) - makeUniqueName = uniqueNameMaker(reader.getLayerNames()) - ufoLayerName = makeUniqueName(suggestedLayerName) - # Create the new UFO layer now - _ = self.ufoManager.getGlyphSet(ufoPath, ufoLayerName) + ufoLayerName = suggestedLayerName + count = 0 + while glyphName in self.ufoManager.getGlyphSet(ufoPath, ufoLayerName): + # The glyph already exists in the layer, which means there is + # a conflict. Let's make up a layer name in which the glyph + # does not exist. + count += 1 + ufoLayerName = f"{suggestedLayerName}#{count}" reader.writeLayerContents() ufoLayer = UFOLayer( From e60e620ed08e2582da2ec90e532ea11ec1ffc983 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 11:07:14 +0200 Subject: [PATCH 4/7] Only write the layer contents if the layer didn't already exist --- src/fontra/backends/designspace.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/fontra/backends/designspace.py b/src/fontra/backends/designspace.py index f8f339bb8..e8073dde9 100644 --- a/src/fontra/backends/designspace.py +++ b/src/fontra/backends/designspace.py @@ -515,6 +515,7 @@ def _createDSSource(self, glyphName, source, globalLocation): def _newUFOLayer(self, glyphName, ufoPath, suggestedLayerName): reader = self.ufoManager.getReader(ufoPath) + existingLayerNames = set(reader.getLayerNames()) ufoLayerName = suggestedLayerName count = 0 while glyphName in self.ufoManager.getGlyphSet(ufoPath, ufoLayerName): @@ -523,7 +524,8 @@ def _newUFOLayer(self, glyphName, ufoPath, suggestedLayerName): # does not exist. count += 1 ufoLayerName = f"{suggestedLayerName}#{count}" - reader.writeLayerContents() + if ufoLayerName not in existingLayerNames: + reader.writeLayerContents() ufoLayer = UFOLayer( manager=self.ufoManager, From f264939c952f13348d30c75c325a48c09442ed5e Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 11:07:42 +0200 Subject: [PATCH 5/7] whitespace --- src/fontra/backends/designspace.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fontra/backends/designspace.py b/src/fontra/backends/designspace.py index e8073dde9..d154d78b8 100644 --- a/src/fontra/backends/designspace.py +++ b/src/fontra/backends/designspace.py @@ -524,6 +524,7 @@ def _newUFOLayer(self, glyphName, ufoPath, suggestedLayerName): # does not exist. count += 1 ufoLayerName = f"{suggestedLayerName}#{count}" + if ufoLayerName not in existingLayerNames: reader.writeLayerContents() From ec49023c4e496d378bf77f33e2d622dbf21fe50d Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 11:12:22 +0200 Subject: [PATCH 6/7] Add comment --- test-py/test_backend_designspace.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test-py/test_backend_designspace.py b/test-py/test_backend_designspace.py index 290d64585..8c7560ab1 100644 --- a/test-py/test_backend_designspace.py +++ b/test-py/test_backend_designspace.py @@ -250,6 +250,7 @@ async def test_newFileSystemBackend(tmpdir, testFont): async def test_writeCorrectLayers(tmpdir, testFont): + # Check that no redundant layers are written tmpdir = pathlib.Path(tmpdir) dsPath = tmpdir / "Test.designspace" font = newFileSystemBackend(dsPath) From d6e622af2f1d29e5851f71d1e8f0062a67c308ab Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 5 Sep 2023 11:15:26 +0200 Subject: [PATCH 7/7] Add comment --- src/fontra/backends/designspace.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fontra/backends/designspace.py b/src/fontra/backends/designspace.py index d154d78b8..0a9d570d2 100644 --- a/src/fontra/backends/designspace.py +++ b/src/fontra/backends/designspace.py @@ -518,6 +518,7 @@ def _newUFOLayer(self, glyphName, ufoPath, suggestedLayerName): existingLayerNames = set(reader.getLayerNames()) ufoLayerName = suggestedLayerName count = 0 + # getGlyphSet() will create the layer if it doesn't already exist while glyphName in self.ufoManager.getGlyphSet(ufoPath, ufoLayerName): # The glyph already exists in the layer, which means there is # a conflict. Let's make up a layer name in which the glyph