From f96b2128a1b5468464b40c485f5752aa693114e8 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 24 Jan 2024 11:28:13 +0000 Subject: [PATCH 1/3] [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}) From 957b5fb45a795b06f9a3dfd168dc2c9cc5bae3a5 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 24 Jan 2024 12:34:48 +0000 Subject: [PATCH 2/3] don't sort product of liga components to keep declaration order technically we are tweaking the original example from the spec but it keeps the spirit, so that the product of glyph classes produces the same representation in the font as if the sequences were manually enumerated (while keeping the declaration order) --- Lib/fontTools/feaLib/builder.py | 2 +- Tests/feaLib/data/spec5d1.fea | 12 ++++++------ Tests/feaLib/data/spec5d1.ttx | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py index 36eed9514..02151620b 100644 --- a/Lib/fontTools/feaLib/builder.py +++ b/Lib/fontTools/feaLib/builder.py @@ -1338,7 +1338,7 @@ class Builder(object): # substitutions to be specified on target sequences that contain # glyph classes, the implementation software will enumerate # all specific glyph sequences if glyph classes are detected" - for g in sorted(itertools.product(*glyphs)): + for g in itertools.product(*glyphs): lookup.ligatures[g] = replacement # GSUB 5/6 diff --git a/Tests/feaLib/data/spec5d1.fea b/Tests/feaLib/data/spec5d1.fea index cf7d76f70..a9a23f815 100644 --- a/Tests/feaLib/data/spec5d1.fea +++ b/Tests/feaLib/data/spec5d1.fea @@ -2,7 +2,7 @@ # http://www.adobe.com/devnet/opentype/afdko/topic_feature_file_syntax.html feature F1 { - sub [one one.oldstyle] [slash fraction] [two two.oldstyle] by onehalf; + sub [one one.oldstyle] [fraction slash] [two two.oldstyle] by onehalf; } F1; # Since the OpenType specification does not allow ligature substitutions @@ -12,12 +12,12 @@ feature F1 { # example produces an identical representation in the font as if all # the sequences were manually enumerated by the font editor: feature F2 { - sub one slash two by onehalf; - sub one.oldstyle slash two by onehalf; sub one fraction two by onehalf; - sub one.oldstyle fraction two by onehalf; - sub one slash two.oldstyle by onehalf; - sub one.oldstyle slash two.oldstyle by onehalf; sub one fraction two.oldstyle by onehalf; + sub one slash two by onehalf; + sub one slash two.oldstyle by onehalf; + sub one.oldstyle fraction two by onehalf; sub one.oldstyle fraction two.oldstyle by onehalf; + sub one.oldstyle slash two by onehalf; + sub one.oldstyle slash two.oldstyle by onehalf; } F2; diff --git a/Tests/feaLib/data/spec5d1.ttx b/Tests/feaLib/data/spec5d1.ttx index 79c0e8675..77dfc93b8 100644 --- a/Tests/feaLib/data/spec5d1.ttx +++ b/Tests/feaLib/data/spec5d1.ttx @@ -62,16 +62,16 @@ - - + + - - + + From 796f677225ae7d9a0d0b661fa04ad6a31a883ac6 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 24 Jan 2024 16:41:34 +0000 Subject: [PATCH 3/3] restore original spec5d1.fea example and modify expected ttx instead --- Tests/feaLib/data/spec5d1.fea | 22 ++++++++++++++++------ Tests/feaLib/data/spec5d1.ttx | 16 ++++++++-------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/Tests/feaLib/data/spec5d1.fea b/Tests/feaLib/data/spec5d1.fea index a9a23f815..0b5acdd30 100644 --- a/Tests/feaLib/data/spec5d1.fea +++ b/Tests/feaLib/data/spec5d1.fea @@ -2,7 +2,7 @@ # http://www.adobe.com/devnet/opentype/afdko/topic_feature_file_syntax.html feature F1 { - sub [one one.oldstyle] [fraction slash] [two two.oldstyle] by onehalf; + sub [one one.oldstyle] [slash fraction] [two two.oldstyle] by onehalf; } F1; # Since the OpenType specification does not allow ligature substitutions @@ -11,13 +11,23 @@ feature F1 { # if glyph classes are detected in . Thus, the above # example produces an identical representation in the font as if all # the sequences were manually enumerated by the font editor: +# +# NOTE(anthrotype): The previous sentence is no longer entirely true, since we +# now preserve the order in which the ligatures (with same length and first glyph) +# were specified in the feature file and do not sort them alphabetically +# by the ligature component names. Therefore, the way this particular example from +# the FEA spec is written will produce two slightly different representations +# in the font in which the ligatures are enumerated differently, however the two +# lookups are functionally equivalent. +# See: https://github.com/fonttools/fonttools/issues/3428 +# https://github.com/adobe-type-tools/afdko/issues/1727 feature F2 { - sub one fraction two by onehalf; - sub one fraction two.oldstyle by onehalf; sub one slash two by onehalf; - sub one slash two.oldstyle by onehalf; - sub one.oldstyle fraction two by onehalf; - sub one.oldstyle fraction two.oldstyle by onehalf; sub one.oldstyle slash two by onehalf; + sub one fraction two by onehalf; + sub one.oldstyle fraction two by onehalf; + sub one slash two.oldstyle by onehalf; sub one.oldstyle slash two.oldstyle by onehalf; + sub one fraction two.oldstyle by onehalf; + sub one.oldstyle fraction two.oldstyle by onehalf; } F2; diff --git a/Tests/feaLib/data/spec5d1.ttx b/Tests/feaLib/data/spec5d1.ttx index 77dfc93b8..8763c931e 100644 --- a/Tests/feaLib/data/spec5d1.ttx +++ b/Tests/feaLib/data/spec5d1.ttx @@ -43,16 +43,16 @@ - - + + - - + + @@ -62,16 +62,16 @@ - - + + - - + +