Implement Cosimo feedback

Lib/fontTools/varLib/instancer.py

@@ -1024,10 +1024,12 @@ def instantiateSTAT(varfont, axisLimits):
         return  # STAT table empty, nothing to do
 
     log.info("Instantiating STAT table")
-    _instantiateSTAT(stat, axisLimits)
+    newAxisValueTables = axisValuesFromAxisLimits(stat, axisLimits)
+    stat.AxisValueArray.AxisValue = newAxisValueTables
+    stat.AxisValueCount = len(stat.AxisValueArray.AxisValue)
 
 
-def _instantiateSTAT(stat, axisLimits):
+def axisValuesFromAxisLimits(stat, axisLimits):
     location, axisRanges = splitAxisLocationAndRanges(axisLimits, rangeType=AxisRange)
 
     def isAxisValueOutsideLimits(axisTag, axisValue):
@@ -1068,9 +1070,7 @@ def _instantiateSTAT(stat, axisLimits):
         else:
             log.warn("Unknown AxisValue table format (%s); ignored", axisValueFormat)
         newAxisValueTables.append(axisValueTable)
-
+    return newAxisValueTables
-    stat.AxisValueArray.AxisValue = newAxisValueTables
-    stat.AxisValueCount = len(stat.AxisValueArray.AxisValue)
 
 
 def getVariationNameIDs(varfont):
@@ -1121,8 +1121,97 @@ def updateNameTable(varfont, axisLimits):
     """Update an instatiated variable font's name table using the Axis
     Values from the STAT table.
 
-    The updated nametable will conform to the R/I/B/BI naming model.
+    The updated name table will conform to the R/I/B/BI naming model.
     """
+    # This task can be split into two parts:
+
+    # Task 1: Collecting and sorting the relevant AxisValues:
+    # 1. First check the variable font has a STAT table and it contains
+    #    AxisValues.
+    # 2. Create a dictionary which contains the pinned axes from the
+    #    axisLimits dict and for the unpinned axes, we'll use the fvar
+    #    default coordinates e.g
+    #    axisLimits = {"wght": 500, "wdth": AxisRange(75, 100), our dict will
+    #    be {"wght": 500, "wdth": 100} if the width axis has a default of 100.
+    # 3. Create a new list of AxisValues whose Values match the dict we just
+    #    created.
+    # 4. Remove any AxisValues from the list which have the
+    #    Elidable_AXIS_VALUE_NAME flag set.
+    # 5. Remove and sort AxisValues in the list so format 4 AxisValues take
+    #    precedence. According to the MS Spec "if a format 1, format 2 or
+    #    format 3 table has a (nominal) value used in a format 4 table that
+    #    also has values for other axes, the format 4 table, being the more
+    #    specific match, is used",
+    #    https://docs.microsoft.com/en-us/typography/opentype/spec/stat#axis-value-table-format-4
+
+    # Task 2: Updating a name table's style and family names from a list of
+    # AxisValues:
+    # 1. Sort AxisValues into two groups. For the first group, the names must be
+    #    any of the following ["Regular", "Italic", "Bold", "Bold Italic"].
+    #    This group of names is often referred to as "RIBBI" names. For the
+    #    other group, names must be non-RIBBI e.g "Medium Italic", "Condensed"
+    #    etc.
+    # 2. Repeat the next steps for each name table record platform:
+    #    a. Create new subFamily name and Typographic subFamily name from the
+    #       above groups.
+    #    b. Update nameIDs 1, 2, 3, 4, 6, 16, 17 using the new name created
+    #       in the last step.
+    #
+    # Step by step example:
+    # A variable font which has a width and weight axes.
+    # AxisValues in font (represented as simplified dicts):
+    # axisValues = [
+    #     {"name": "Light", "axis": "wght", "value": 300},
+    #     {"name": "Regular", "axis": "wght", "value": 400},
+    #     {"name": "Medium", "axis": "wght", "value": 500},
+    #     {"name": "Bold", "axis": "wght", "value": 600},
+    #     {"name": "Condensed", "axis": "wdth", "value": 75},
+    #     {"name": "Normal", "axis": "wdth", "value": 100, "flags": 0x2},
+    # ]
+    # # Let's instantiate a partial font which has a pinned wght axis and an
+    #   unpinned width axis.
+    # >>> axisLimits = {"wght": 500, "width": AxisRange(75, 100)}
+    # >>> updateNameTable(varfont, axisLimits)
+    #
+    # AxisValues remaining after task 1.3:
+    # axisValues = [
+    #     {"name": "Medium", "axis": "wght", "value": 500},
+    #     {"name": "Normal", "axis": "wdth", "value": 100, "flags": 0x2}
+    # ]
+    #
+    # AxisValues remaining after completing all 1.x tasks:
+    # axisValues = [{"name": "Medium", "axis": "wght", "value": 500}]
+    # The Normal AxisValue is removed because it has the
+    # Elidable_AXIS_VALUE_NAME flag set.
+    #
+    # # AxisValues after separating into two groups in task 2.1:
+    # ribbiAxisValues = []
+    # nonRibbiAxisValues = [{"name": "Medium", "axis": "wght", "value": 500}]
+    #
+    # # Names created from AxisValues in task 2.2a for Win US English platform:
+    # subFamilyName = ""
+    # typoSubFamilyName = "Medium"
+    #
+    # NameRecords updated in task 2.2b for Win US English platform:
+    # NameID 1 familyName: "Open Sans" --> "Open Sans Medium"
+    # NameID 2 subFamilyName: "Regular" --> "Regular"
+    # NameID 3 Unique font identifier: "3.000;GOOG;OpenSans-Regular" --> \
+    #     "3.000;GOOG;OpenSans-Medium"
+    # NameID 4 Full font name: "Open Sans Regular" --> "Open Sans Medium"
+    # NameID 6 PostScript name: "OpenSans-Regular" --> "OpenSans-Medium"
+    # NameID 16 Typographic Family name: None --> "Open Sans"
+    # NameID 17 Typographic Subfamily name: None --> "Medium"
+    #
+    # Notes on name table record updates:
+    # - Typographic names have been added since Medium is a non-Ribbi name.
+    # - Neither the before or after name records include the Width AxisValue
+    #   names because the "Normal" AxisValue has the
+    #   Elidable_AXIS_VALUE_NAME flag set.
+    #   If we instantiate the same font but pin the wdth axis to 75,
+    #   the "Condensed" AxisValue will be included.
+    # - For info regarding how RIBBI and non-RIBBI can be constructed see:
+    # https://docs.microsoft.com/en-us/typography/opentype/spec/name#name-ids
+
     if "STAT" not in varfont:
         raise ValueError("Cannot update name table since there is no STAT table.")
     stat = varfont["STAT"].table
@@ -1134,42 +1223,44 @@ def updateNameTable(varfont, axisLimits):
     # If we're instantiating a partial font, we will populate the unpinned
     # axes with their default axis values.
     fvarDefaults = {a.axisTag: a.defaultValue for a in fvar.axes}
-    axisCoords = deepcopy(axisLimits)
+    defaultAxisCoords = deepcopy(axisLimits)
     for axisTag, val in fvarDefaults.items():
-        if axisTag not in axisCoords or isinstance(axisCoords[axisTag], tuple):
+        if axisTag not in defaultAxisCoords or isinstance(
-            axisCoords[axisTag] = val
+            defaultAxisCoords[axisTag], AxisRange
+        ):
+            defaultAxisCoords[axisTag] = val
 
-    # To get the required Axis Values for the zero origin, we can simply
+    axisValueTables = axisValuesFromAxisLimits(stat, defaultAxisCoords)
-    # duplicate the STAT table and instantiate it using the axis coords we
+    checkAxisValuesExist(stat, axisValueTables, defaultAxisCoords)
-    # created in the previous step.
-    stat_new = deepcopy(stat)
-    _instantiateSTAT(stat_new, axisCoords)
-    checkMissingAxisValues(stat_new, axisCoords)
 
-    axisValueTables = stat_new.AxisValueArray.AxisValue
+    # Remove axis Values which have ELIDABLE_AXIS_VALUE_NAME flag set.
-    # Remove axis Values which have Elidable_AXIS_VALUE_NAME flag set.
     # Axis Values which have this flag enabled won't be visible in
     # application font menus.
     axisValueTables = [
-        v for v in axisValueTables if v.Flags & ELIDABLE_AXIS_VALUE_NAME != 2
+        v for v in axisValueTables if not v.Flags & ELIDABLE_AXIS_VALUE_NAME
     ]
-    stat_new.AxisValueArray.AxisValue = axisValueTables
+    axisValueTables = _sortAxisValues(axisValueTables)
-    axisValueTables = _sortedAxisValues(stat_new, axisCoords)
     _updateNameRecords(varfont, axisValueTables)
 
 
-def checkMissingAxisValues(stat, axisCoords):
+def checkAxisValuesExist(stat, axisValues, axisCoords):
     seen = set()
-    axisValues = stat.AxisValueArray.AxisValue
     designAxes = stat.DesignAxisRecord.Axis
-    for val in axisValues:
+    for axisValueTable in axisValues:
-        if val.Format == 4:
+        axisValueFormat = axisValueTable.Format
-            for rec in val.AxisValueRecord:
+        if axisValueTable.Format in (1, 2, 3):
-                axisTag = designAxes[rec.AxisIndex].AxisTag
+            axisTag = designAxes[axisValueTable.AxisIndex].AxisTag
+            if axisValueFormat == 2:
+                axisValue = axisValueTable.NominalValue
+            else:
+                axisValue = axisValueTable.Value
+            if axisTag in axisCoords and axisValue == axisCoords[axisTag]:
                 seen.add(axisTag)
-        else:
+        elif axisValueTable.Format == 4:
-            axisTag = designAxes[val.AxisIndex].AxisTag
+            for rec in axisValueTable.AxisValueRecord:
-            seen.add(axisTag)
+                axisTag = designAxes[rec.AxisIndex].AxisTag
+                if axisTag in axisCoords and rec.Value == axisCoords[axisTag]:
+                    seen.add(axisTag)
 
     missingAxes = set(axisCoords) - seen
     if missingAxes:
@@ -1177,11 +1268,9 @@ def checkMissingAxisValues(stat, axisCoords):
         raise ValueError(f"Cannot find Axis Values [{missing}]")
 
 
-def _sortedAxisValues(stat, axisCoords):
+def _sortAxisValues(axisValues):
     # Sort and remove duplicates ensuring that format 4 Axis Values
     # are dominant
-    axisValues = stat.AxisValueArray.AxisValue
-    designAxes = stat.DesignAxisRecord.Axis
     results = []
     seenAxes = set()
     # Sort format 4 axes so the tables with the most AxisValueRecords
@@ -1320,11 +1409,11 @@ def _updateNameTableStyleRecords(
         ):
             nametable.removeNames(nameID=nameID)
 
-    newFamilyName = nameIDs.get(NameID.TYPOGRAPHIC_FAMILY_NAME) or nameIDs.get(
+    newFamilyName = (
-        NameID.FAMILY_NAME
+        nameIDs.get(NameID.TYPOGRAPHIC_FAMILY_NAME) or nameIDs[NameID.FAMILY_NAME]
     )
-    newStyleName = nameIDs.get(NameID.TYPOGRAPHIC_SUBFAMILY_NAME) or nameIDs.get(
+    newStyleName = (
-        NameID.SUBFAMILY_NAME
+        nameIDs.get(NameID.TYPOGRAPHIC_SUBFAMILY_NAME) or nameIDs[NameID.SUBFAMILY_NAME]
     )
 
     nameIDs[NameID.FULL_FONT_NAME] = f"{newFamilyName} {newStyleName}"
@@ -1373,23 +1462,20 @@ def _updateUniqueIdNameRecord(varfont, nameIDs, platform):
     if not currentRecord:
         return None
 
-    def isSubString(string1, string2):
-        if string2 in string1:
-            return True
-        return False
-
     # Check if full name and postscript name are a substring of currentRecord
-    for nameID in (4, 6):
+    for nameID in (NameID.FULL_FONT_NAME, NameID.POSTSCRIPT_NAME):
         nameRecord = nametable.getName(nameID, *platform)
         if not nameRecord:
             continue
-        if isSubString(currentRecord.toUnicode(), nameRecord.toUnicode()):
+        if currentRecord.toUnicode() in nameRecord.toUnicode():
             return currentRecord.toUnicode().replace(
                 nameRecord.toUnicode(), nameIDs[nameRecord.nameID]
             )
     # Create a new string since we couldn't find any substrings.
     fontVersion = _fontVersion(varfont, platform)
-    vendor = varfont["OS/2"].achVendID.strip()
+    achVendID = varfont["OS/2"].achVendID
+    # Remove non-ASCII characers and trailing spaces
+    vendor = re.sub(r"[^\x00-\x7F]", "", achVendID).strip()
     psName = nameIDs[NameID.POSTSCRIPT_NAME]
     return f"{fontVersion};{vendor};{psName}"
 
@@ -1546,7 +1632,7 @@ def instantiateVariableFont(
         varfont = deepcopy(varfont)
 
     if updateFontNames:
-        log.info("Updating nametable")
+        log.info("Updating name table")
         updateNameTable(varfont, axisLimits)
 
     if "gvar" in varfont:
@@ -1693,9 +1779,9 @@ def parseArgs(args):
         "when generating a full instance). Requires skia-pathops",
     )
     parser.add_argument(
-        "--update-nametable",
+        "--update-name-table",
         action="store_true",
-        help="Update the instantiated font's nametable. Input font must have "
+        help="Update the instantiated font's `name` table. Input font must have "
         "a STAT table with Axis Value Tables",
     )
     loggingGroup = parser.add_mutually_exclusive_group(required=False)
@@ -1749,7 +1835,7 @@ def main(args=None):
         inplace=True,
         optimize=options.optimize,
         overlap=options.overlap,
-        updateFontNames=options.update_nametable,
+        updateFontNames=options.update_name_table,
     )
 
     outfile = (

Tests/varLib/instancer_test.py

@@ -2000,6 +2000,7 @@ def test_updatetNameTable_axis_order(varfont):
 
 def test_updateNameTable_with_multilingual_names(varfont):
     name = varfont["name"]
+    # langID 0x405 is the Czech Windows langID
     name.setName("Test Variable Font", 1, 3, 1, 0x405)
     name.setName("Normal", 2, 3, 1, 0x405)
     name.setName("Normal", 261, 3, 1, 0x405) # nameID 261=Regular STAT entry
@@ -2035,17 +2036,16 @@ def test_updateNameTable_with_multilingual_names(varfont):
 
 
 def test_updateNametable_partial(varfont):
-    instancer.updateNameTable(varfont, {"wdth": 79, "wght": (400, 900)})
+    instancer.updateNameTable(varfont, {"wdth": 79, "wght": instancer.AxisRange(400, 900)})
     names = _get_name_records(varfont)
     assert names[(1, 3, 1, 0x409)] == "Test Variable Font Condensed"
     assert names[(2, 3, 1, 0x409)] == "Regular"
-    assert (3, 3, 1, 0x405) not in names
     assert names[(16, 3, 1, 0x409)] == "Test Variable Font"
     assert names[(17, 3, 1, 0x409)] == "Condensed"
 
 
 def test_updateNameTable_missing_axisValues(varfont):
-    with pytest.raises(ValueError, match="Cannot find Axis Value Tables \['wght=200'\]"):
+    with pytest.raises(ValueError, match="Cannot find Axis Values \['wght=200'\]"):
         instancer.updateNameTable(varfont, {"wght": 200})