Repository: googlefonts/ufo2ft

Issue:
Respect overrides due to openTypeNameRecords for the predefined name IDs
In Glyphs, it is possible to add custom naming-table entries via `Name Table Entry`. If the custom entry touches any of the predefined name IDs, such as 1 and 2, Glyphs then takes the custom value upon export, thereby allowing the designer to have, for instance, different predefined names for different platforms.

Recently, glyphsLib has been updated to propagate `Name Table Entry`'s to the resulting UFO upon conversion. However, upon compilation, ufo2ft resolves the situation with overrides differently:

https://github.com/googlefonts/ufo2ft/blob/main/Lib/ufo2ft/outlineCompiler.py#L385-L453

It can be seen that it first sets the custom entries (which can reference the predefined IDs) and then goes into processing the predefined IDs. Moreover, it sometimes skips setting some of the predefined IDs when it detects that some other predefined IDs are sufficient—all of that without taking into account what has been set due to the custom entries. It leads to some puzzling end results where things oscillate between styles within the same family.

The proposal is to simply swap the order. The core logic with its optimization will remain the same, but if the end user so wishes, things will be added on top without any checks.

Discussion:

Patch:
diff --git a/Lib/ufo2ft/featureWriters/baseFeatureWriter.py b/Lib/ufo2ft/featureWriters/baseFeatureWriter.py
index ba79d38..6d66dc2 100644
--- a/Lib/ufo2ft/featureWriters/baseFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/baseFeatureWriter.py
@@ -234,21 +234,23 @@ class BaseFeatureWriter:
             statements.insert(index, feature)
             indices.append(index)
 
-        minindex = min(indices)
-        if lookups:
-            feaFile.statements = statements = (
-                statements[:minindex] + lookups + statements[minindex:]
-            )
-
-        # Write classDefs, anchorsDefs, markClassDefs, lookups at
-        # the very top of the feature file.
+        # Write classDefs, anchorsDefs, markClassDefs, lookups at earliest
+        # opportunity.
         others = []
+        minindex = min(indices)
         for defs in [classDefs, anchorDefs, markClassDefs]:
             if defs:
                 others.extend(defs)
                 others.append(ast.Comment(""))
-
-        feaFile.statements = statements = others + statements
+        # Insert lookups
+        if lookups:
+            if minindex > 0 and not others:
+                others.append(ast.Comment(""))
+            others.extend(lookups)
+        if others:
+            feaFile.statements = statements = (
+                statements[:minindex] + others + statements[minindex:]
+            )
 
     @staticmethod
     def collectInsertMarkers(feaFile, insertFeatureMarker, featureTags):
diff --git a/Lib/ufo2ft/outlineCompiler.py b/Lib/ufo2ft/outlineCompiler.py
index 8466b00..4c923ac 100644
--- a/Lib/ufo2ft/outlineCompiler.py
+++ b/Lib/ufo2ft/outlineCompiler.py
@@ -382,20 +382,6 @@ class BaseOutlineCompiler:
         self.otf["name"] = name = newTable("name")
         name.names = []
 
-        # Set name records from font.info.openTypeNameRecords
-        for nameRecord in getAttrWithFallback(font.info, "openTypeNameRecords"):
-            nameId = nameRecord["nameID"]
-            platformId = nameRecord["platformID"]
-            platEncId = nameRecord["encodingID"]
-            langId = nameRecord["languageID"]
-            # on Python 2, plistLib (used by ufoLib) returns unicode strings
-            # only when plist data contain non-ascii characters, and returns
-            # ascii-encoded bytes when it can. On the other hand, fontTools's
-            # name table `setName` method wants unicode strings, so we must
-            # decode them first
-            nameVal = nameRecord["string"]
-            name.setName(nameVal, nameId, platformId, platEncId, langId)
-
         # Build name records
         familyName = getAttrWithFallback(font.info, "styleMapFamilyName")
         styleName = getAttrWithFallback(font.info, "styleMapStyleName").title()
@@ -452,6 +438,15 @@ class BaseOutlineCompiler:
                 continue
             name.setName(nameVal, nameId, platformId, platEncId, langId)
 
+        # Set name records from font.info.openTypeNameRecords
+        for nameRecord in getAttrWithFallback(font.info, "openTypeNameRecords"):
+            nameId = nameRecord["nameID"]
+            platformId = nameRecord["platformID"]
+            platEncId = nameRecord["encodingID"]
+            langId = nameRecord["languageID"]
+            nameVal = nameRecord["string"]
+            name.setName(nameVal, nameId, platformId, platEncId, langId)
+
     def setupTable_maxp(self):
         """
         Make the maxp table.


Test patch:
diff --git a/tests/featureCompiler_test.py b/tests/featureCompiler_test.py
index 8ed92a4..6d6bead 100644
--- a/tests/featureCompiler_test.py
+++ b/tests/featureCompiler_test.py
@@ -341,6 +341,7 @@ class FeatureCompilerTest:
                 sub f f by f_f;
             } liga;
 
+
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos a v -40;
diff --git a/tests/featureWriters/__init__.py b/tests/featureWriters/__init__.py
index 8783a2c..881bb1d 100644
--- a/tests/featureWriters/__init__.py
+++ b/tests/featureWriters/__init__.py
@@ -13,12 +13,8 @@ class FeatureWriterTest:
         """
         writer = cls.FeatureWriter(**kwargs)
         feaFile = parseLayoutFeatures(ufo)
-        old_statements = [st.asFea() for st in feaFile.statements]
-
+        n = len(feaFile.statements)
         if writer.write(ufo, feaFile):
             new = ast.FeatureFile()
-
-            for statement in feaFile.statements:
-                if statement.asFea() not in old_statements:
-                    new.statements.append(statement)
+            new.statements = feaFile.statements[n:]
             return new
diff --git a/tests/featureWriters/kernFeatureWriter2_test.py b/tests/featureWriters/kernFeatureWriter2_test.py
index 6230126..284745e 100644
--- a/tests/featureWriters/kernFeatureWriter2_test.py
+++ b/tests/featureWriters/kernFeatureWriter2_test.py
@@ -147,7 +147,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos B C -30;
@@ -166,7 +166,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         feaFile = self.writeFeatures(font, ignoreMarks=False)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 pos A acutecomb -55;
                 pos B C -30;
@@ -198,7 +198,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_ltr_marks {
                 pos A acutecomb -55;
             } kern_ltr_marks;
@@ -236,6 +236,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         expected = existing + dedent(
             """
+
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -300,7 +301,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -383,6 +384,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
                 #
             } kern;
 
+
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -399,7 +401,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -442,7 +444,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -474,7 +476,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_rtl {
                 lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
@@ -904,7 +906,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos A V -40;
@@ -931,7 +933,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos A V -40;
@@ -1023,7 +1025,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_rtl {
                 lookupflag IgnoreMarks;
                 pos u10A1E u10A06 <117 0 117 0>;
@@ -1151,7 +1153,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_dflt {
                 lookupflag IgnoreMarks;
                 pos seven four -25;
diff --git a/tests/featureWriters/kernFeatureWriter_test.py b/tests/featureWriters/kernFeatureWriter_test.py
index d99258b..d98a0a1 100644
--- a/tests/featureWriters/kernFeatureWriter_test.py
+++ b/tests/featureWriters/kernFeatureWriter_test.py
@@ -155,7 +155,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_Latn {
                 lookupflag IgnoreMarks;
                 pos B C -30;
@@ -176,7 +176,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         feaFile = self.writeFeatures(font, ignoreMarks=False)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_Latn {
                 pos A acutecomb -55;
                 pos B C -30;
@@ -210,7 +210,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_Default_marks {
                 pos A acutecomb -55;
             } kern_Default_marks;
@@ -250,6 +250,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         expected = existing + dedent(
             """
+
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -318,7 +319,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -405,6 +406,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
                 #
             } kern;
 
+
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -423,7 +425,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -468,7 +470,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -545,7 +547,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert dedent(str(generated)) == dedent(
-            """\
+            """
             lookup kern_Arab {
                 lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
@@ -572,7 +574,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert dedent(str(generated)) == dedent(
-            """\
+            """
             lookup kern_Thaa {
                 lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
@@ -1089,7 +1091,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Khar {
                 lookupflag IgnoreMarks;
                 pos u10A1E u10A06 <117 0 117 0>;
@@ -1209,7 +1211,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert dedent(str(generated)) == dedent(
-            """\
+            """
             lookup kern_Hebr {
                 lookupflag IgnoreMarks;
                 pos yod-hb bet-hb <-100 0 -100 0>;
@@ -2113,7 +2115,7 @@ def test_dflt_language(FontClass):
     newFeatures = KernFeatureWriterTest.writeFeatures(ufo)
 
     assert dedent(str(newFeatures)) == dedent(
-        """\
+        """
         lookup kern_Latn {
             lookupflag IgnoreMarks;
             pos a a 1;
diff --git a/tests/featureWriters/markFeatureWriter_test.py b/tests/featureWriters/markFeatureWriter_test.py
index 1dc9044..b698779 100644
--- a/tests/featureWriters/markFeatureWriter_test.py
+++ b/tests/featureWriters/markFeatureWriter_test.py
@@ -282,9 +282,9 @@ class MarkFeatureWriterTest(FeatureWriterTest):
 
         assert str(feaFile) == dedent(
             """\
+            markClass acutecomb <anchor 100 200> @MC_top;
             markClass tildecomb <anchor 100 200> @MC_top;
 
-            markClass acutecomb <anchor 100 200> @MC_top;
             feature mark {
                 lookup mark2base {
                     pos base a
@@ -377,8 +377,6 @@ class MarkFeatureWriterTest(FeatureWriterTest):
 
         assert str(feaFile) == dedent(
             """\
-            markClass tildecomb <anchor 100 200> @MC_top;
-
             markClass acutecomb <anchor 100 200> @MC_top;
             feature mark {
                 lookup mark1 {
@@ -390,6 +388,8 @@ class MarkFeatureWriterTest(FeatureWriterTest):
                 #
             } mark;
 
+            markClass tildecomb <anchor 100 200> @MC_top;
+
             feature mark {
                 lookup mark2base {
                     pos base a
diff --git a/tests/outlineCompiler_test.py b/tests/outlineCompiler_test.py
index b31319c..50f3f4c 100644
--- a/tests/outlineCompiler_test.py
+++ b/tests/outlineCompiler_test.py
@@ -228,6 +228,26 @@ class OutlineTTFCompilerTest:
         assert meta.data["PRIA"] == b"Some private ascii string"
         assert meta.data["PRIU"] == "Some private unicode string…".encode("utf-8")
 
+    def test_setupTable_name(self, testufo):
+        compiler = OutlineTTFCompiler(testufo)
+        compiler.compile()
+        actual = compiler.otf["name"].getName(1, 3, 1, 1033).string
+        assert actual == "Some Font Regular (Style Map Family Name)"
+
+        testufo.info.openTypeNameRecords.append(
+            {
+                "nameID": 1,
+                "platformID": 3,
+                "encodingID": 1,
+                "languageID": 1033,
+                "string": "Custom Name for Windows",
+            }
+        )
+        compiler = OutlineTTFCompiler(testufo)
+        compiler.compile()
+        actual = compiler.otf["name"].getName(1, 3, 1, 1033).string
+        assert actual == "Custom Name for Windows"
+
 
 class OutlineOTFCompilerTest:
     def test_setupTable_CFF_all_blues_defined(self, testufo):