Merge pull request #780 from googlefonts/better-ufo-layers

Don't create redundant UFO layers
googlefonts · Sep 5, 2023 · ee033ed · ee033ed
2 parents 5d97f90 + d6e622a
commit ee033ed
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 13 deletions.
diff --git 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,13 +513,21 @@ 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)
-        # Create the new UFO layer now
-        _ = self.ufoManager.getGlyphSet(ufoPath, ufoLayerName)
-        reader.writeLayerContents()
+        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
+            # does not exist.
+            count += 1
+            ufoLayerName = f"{suggestedLayerName}#{count}"
+
+        if ufoLayerName not in existingLayerNames:
+            reader.writeLayerContents()
 
         ufoLayer = UFOLayer(
             manager=self.ufoManager,

diff --git a/test-py/test_backend_designspace.py b/test-py/test_backend_designspace.py
@@ -249,6 +249,28 @@ async def test_newFileSystemBackend(tmpdir, testFont):
     assert glyph == referenceGlyph
 
 
+async def test_writeCorrectLayers(tmpdir, testFont):
+    # Check that no redundant layers are written
+    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())