diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py index b29c41b5c..35c2c8b64 100644 --- a/Lib/fontTools/feaLib/builder.py +++ b/Lib/fontTools/feaLib/builder.py @@ -933,16 +933,31 @@ class Builder(object): for glyph in glyphs: lookup.add_pos(location, glyph, value) + def find_chainable_SinglePos_(self, lookups, glyphs, value): + """Helper for add_single_pos_chained_()""" + for look in lookups: + if all(look.can_add(glyph, value) for glyph in glyphs): + return look + return None + def add_single_pos_chained_(self, location, prefix, suffix, pos): + # https://github.com/fonttools/fonttools/issues/514 chain = self.get_lookup_(location, ChainContextPosBuilder) - sub = self.get_chained_lookup_(location, SinglePosBuilder) + targets = [] + for _, _, _, lookups in chain.rules: + for lookup in lookups: + if isinstance(lookup, SinglePosBuilder): + targets.append(lookup) subs = [] for glyphs, value in pos: if value is None: subs.append(None) continue - if not set(glyphs).isdisjoint(sub.mapping.keys()): + otValue, _ = makeOpenTypeValueRecord(value, pairPosContext=False) + sub = self.find_chainable_SinglePos_(targets, glyphs, otValue) + if sub is None: sub = self.get_chained_lookup_(location, SinglePosBuilder) + targets.append(sub) for glyph in glyphs: sub.add_pos(location, glyph, value) subs.append(sub) @@ -1471,8 +1486,7 @@ class SinglePosBuilder(LookupBuilder): def add_pos(self, location, glyph, valueRecord): otValueRecord, _ = makeOpenTypeValueRecord( valueRecord, pairPosContext=False) - curValue = self.mapping.get(glyph) - if curValue is not None and curValue != otValueRecord: + if not self.can_add(glyph, otValueRecord): otherLoc = self.locations[glyph] raise FeatureLibError( 'Already defined different position for glyph "%s" at %s:%d:%d' @@ -1482,6 +1496,11 @@ class SinglePosBuilder(LookupBuilder): self.mapping[glyph] = otValueRecord self.locations[glyph] = location + def can_add(self, glyph, value): + assert isinstance(value, otl.ValueRecord) + curValue = self.mapping.get(glyph) + return curValue is None or curValue == value + def equals(self, other): return (LookupBuilder.equals(self, other) and self.mapping == other.mapping) diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py index b0f71d168..bb4015b10 100644 --- a/Tests/feaLib/builder_test.py +++ b/Tests/feaLib/builder_test.py @@ -63,7 +63,7 @@ class BuilderTest(unittest.TestCase): spec9a spec9b spec9c1 spec9c2 spec9c3 spec9d spec9e spec9f spec9g spec10 bug453 bug457 bug463 bug501 bug502 bug504 bug505 bug506 bug509 - bug512 bug568 bug633 + bug512 bug514 bug568 bug633 name size size2 multiple_feature_blocks omitted_GlyphClassDef ZeroValue_SinglePos ZeroValue_PairPos_horizontal ZeroValue_PairPos_vertical diff --git a/Tests/feaLib/data/bug514.fea b/Tests/feaLib/data/bug514.fea new file mode 100644 index 000000000..26da86557 --- /dev/null +++ b/Tests/feaLib/data/bug514.fea @@ -0,0 +1,11 @@ +# The many chain targets for this feature should get combined into +# two separate SinglePos lookups: {A:-40, B:-40, C:-40} and {A:-111}. +# https://github.com/fonttools/fonttools/issues/514 +# +# makeotf produces {A:-40, B:-40, C:-40} and {A:-111, B:-40} which +# is redundant. https://github.com/adobe-type-tools/afdko/issues/169 +feature test { + pos X [A-B]' -40 B' -40 A' -40 Y; + pos X A' -111 Y; + pos X B' -40 A' -111 [A-C]' -40 Y; +} test; diff --git a/Tests/feaLib/data/bug514.ttx b/Tests/feaLib/data/bug514.ttx new file mode 100644 index 000000000..f6b14f68d --- /dev/null +++ b/Tests/feaLib/data/bug514.ttx @@ -0,0 +1,154 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +