From f96b2128a1b5468464b40c485f5752aa693114e8 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 24 Jan 2024 11:28:13 +0000 Subject: [PATCH] [feaLib] keep declaration order of ligatures within ligature set Fixes #3428 --- Lib/fontTools/otlLib/builder.py | 15 +-------- Lib/fontTools/ttLib/tables/otTables.py | 35 ++++++++++++++++++--- Tests/feaLib/data/spec4h1.ttx | 2 +- Tests/feaLib/data/spec5d1.ttx | 8 ++--- Tests/mtiLib/data/mti/gsubligature.ttx.GSUB | 16 +++++----- Tests/otlLib/builder_test.py | 6 ++-- 6 files changed, 48 insertions(+), 34 deletions(-) diff --git a/Lib/fontTools/otlLib/builder.py b/Lib/fontTools/otlLib/builder.py index 4b457f4d9..20cf9adb3 100644 --- a/Lib/fontTools/otlLib/builder.py +++ b/Lib/fontTools/otlLib/builder.py @@ -1567,19 +1567,6 @@ def buildAlternateSubstSubtable(mapping): return self -def _getLigatureKey(components): - # Computes a key for ordering ligatures in a GSUB Type-4 lookup. - - # When building the OpenType lookup, we need to make sure that - # the longest sequence of components is listed first, so we - # use the negative length as the primary key for sorting. - # To make buildLigatureSubstSubtable() deterministic, we use the - # component sequence as the secondary key. - - # For example, this will sort (f,f,f) < (f,f,i) < (f,f) < (f,i) < (f,l). - return (-len(components), components) - - def buildLigatureSubstSubtable(mapping): """Builds a ligature substitution (GSUB4) subtable. @@ -1613,7 +1600,7 @@ def buildLigatureSubstSubtable(mapping): # with fontTools >= 3.1: # self.ligatures = dict(mapping) self.ligatures = {} - for components in sorted(mapping.keys(), key=_getLigatureKey): + for components in sorted(mapping.keys(), key=self._getLigatureSortKey): ligature = ot.Ligature() ligature.Component = components[1:] ligature.CompCount = len(ligature.Component) + 1 diff --git a/Lib/fontTools/ttLib/tables/otTables.py b/Lib/fontTools/ttLib/tables/otTables.py index 262f8d418..3505f4233 100644 --- a/Lib/fontTools/ttLib/tables/otTables.py +++ b/Lib/fontTools/ttLib/tables/otTables.py @@ -1123,6 +1123,35 @@ class LigatureSubst(FormatSwitchingBaseTable): self.ligatures = ligatures del self.Format # Don't need this anymore + @staticmethod + def _getLigatureSortKey(components): + # Computes a key for ordering ligatures in a GSUB Type-4 lookup. + + # When building the OpenType lookup, we need to make sure that + # the longest sequence of components is listed first, so we + # use the negative length as the key for sorting. + # Note, we no longer need to worry about deterministic order because the + # ligature mapping `dict` remembers the insertion order, and this in + # turn depends on the order in which the ligatures are written in the FEA. + # Since python sort algorithm is stable, the ligatures of equal length + # will keep the relative order in which they appear in the feature file. + # For example, given the following ligatures (all starting with 'f' and + # thus belonging to the same LigatureSet): + # + # feature liga { + # sub f i by f_i; + # sub f f f by f_f_f; + # sub f f by f_f; + # sub f f i by f_f_i; + # } liga; + # + # this should sort to: f_f_f, f_f_i, f_i, f_f + # This is also what fea-rs does, see: + # https://github.com/adobe-type-tools/afdko/issues/1727 + # https://github.com/fonttools/fonttools/issues/3428 + # https://github.com/googlefonts/fontc/pull/680 + return -len(components) + def preWrite(self, font): self.Format = 1 ligatures = getattr(self, "ligatures", None) @@ -1135,13 +1164,11 @@ class LigatureSubst(FormatSwitchingBaseTable): # ligatures is map from components-sequence to lig-glyph newLigatures = dict() - for comps, lig in sorted( - ligatures.items(), key=lambda item: (-len(item[0]), item[0]) - ): + for comps in sorted(ligatures.keys(), key=self._getLigatureSortKey): ligature = Ligature() ligature.Component = comps[1:] ligature.CompCount = len(comps) - ligature.LigGlyph = lig + ligature.LigGlyph = ligatures[comps] newLigatures.setdefault(comps[0], []).append(ligature) ligatures = newLigatures diff --git a/Tests/feaLib/data/spec4h1.ttx b/Tests/feaLib/data/spec4h1.ttx index 0e42fc56e..a399ab2b4 100644 --- a/Tests/feaLib/data/spec4h1.ttx +++ b/Tests/feaLib/data/spec4h1.ttx @@ -147,8 +147,8 @@ - + diff --git a/Tests/feaLib/data/spec5d1.ttx b/Tests/feaLib/data/spec5d1.ttx index 77dfc93b8..79c0e8675 100644 --- a/Tests/feaLib/data/spec5d1.ttx +++ b/Tests/feaLib/data/spec5d1.ttx @@ -62,16 +62,16 @@ - - + + - - + + diff --git a/Tests/mtiLib/data/mti/gsubligature.ttx.GSUB b/Tests/mtiLib/data/mti/gsubligature.ttx.GSUB index 5ad201845..d7575117f 100644 --- a/Tests/mtiLib/data/mti/gsubligature.ttx.GSUB +++ b/Tests/mtiLib/data/mti/gsubligature.ttx.GSUB @@ -16,20 +16,20 @@ - - - - - - + + + - - + + + + + diff --git a/Tests/otlLib/builder_test.py b/Tests/otlLib/builder_test.py index b7a6caa2f..e2743808b 100644 --- a/Tests/otlLib/builder_test.py +++ b/Tests/otlLib/builder_test.py @@ -1051,11 +1051,11 @@ class BuilderTest(object): func = lambda writer, font: value.toXML(writer, font, valueName="Val") assert getXML(func) == [''] - def test_getLigatureKey(self): + def test_getLigatureSortKey(self): components = lambda s: [tuple(word) for word in s.split()] c = components("fi fl ff ffi fff") - c.sort(key=builder._getLigatureKey) - assert c == components("fff ffi ff fi fl") + c.sort(key=otTables.LigatureSubst._getLigatureSortKey) + assert c == components("ffi fff fi fl ff") def test_getSinglePosValueKey(self): device = builder.buildDevice({10: 1, 11: 3})