From 6364d3663322d9b679e8ac044a4ce0c9f5d9cdbb Mon Sep 17 00:00:00 2001 From: Simon Cozens Date: Thu, 2 Jul 2020 18:40:20 +0100 Subject: [PATCH] [otlLib] Refactor chained contextual builders (#2007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Introduce a new subclass for chained contextual (sub and pos) * Rename .substitutions to .rules in subst builders to allow for code reuse * Make format of subtable break marker tuple common between sub/pos Note that prior to this patch, add_subtable_break in a Subst builder adds: (self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_) while add_subtable_break in a Pos builder adds: (self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, [self.SUBTABLE_BREAK_]) This is messy. If we read the first element from the tuple instead of the last one to test if a rule is a subtable break, we can make the marker tuple the same. * And now the subtable break code can be hoisted into superclass * These helper methods will make the build routine common * Hoist common build method to superclass The diff doesn’t show it very clearly because it’s being too clever, but all I’ve done is moved one method. Everything works apart from the error message, which comes next. * Fix the error message --- Lib/fontTools/feaLib/builder.py | 12 ++-- Lib/fontTools/otlLib/builder.py | 114 +++++++++++++++----------------- 2 files changed, 58 insertions(+), 68 deletions(-) diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py index f42da09b4..3b8cfd85d 100644 --- a/Lib/fontTools/feaLib/builder.py +++ b/Lib/fontTools/feaLib/builder.py @@ -868,7 +868,7 @@ class Builder(object): def add_chain_context_subst(self, location, prefix, glyphs, suffix, lookups): lookup = self.get_lookup_(location, ChainContextSubstBuilder) - lookup.substitutions.append((prefix, glyphs, suffix, + lookup.rules.append((prefix, glyphs, suffix, self.find_lookup_builders_(lookups))) def add_alternate_subst(self, location, @@ -880,7 +880,7 @@ class Builder(object): if prefix or suffix: chain = self.get_lookup_(location, ChainContextSubstBuilder) lookup = self.get_chained_lookup_(location, AlternateSubstBuilder) - chain.substitutions.append((prefix, [{glyph}], suffix, [lookup])) + chain.rules.append((prefix, [{glyph}], suffix, [lookup])) else: lookup = self.get_lookup_(location, AlternateSubstBuilder) if glyph in lookup.alternates: @@ -935,7 +935,7 @@ class Builder(object): if prefix or suffix or forceChain: chain = self.get_lookup_(location, ChainContextSubstBuilder) lookup = self.get_chained_lookup_(location, LigatureSubstBuilder) - chain.substitutions.append((prefix, glyphs, suffix, [lookup])) + chain.rules.append((prefix, glyphs, suffix, [lookup])) else: lookup = self.get_lookup_(location, LigatureSubstBuilder) @@ -953,7 +953,7 @@ class Builder(object): chain = self.get_lookup_(location, ChainContextSubstBuilder) sub = self.get_chained_lookup_(location, MultipleSubstBuilder) sub.mapping[glyph] = replacements - chain.substitutions.append((prefix, [{glyph}], suffix, [sub])) + chain.rules.append((prefix, [{glyph}], suffix, [sub])) return lookup = self.get_lookup_(location, MultipleSubstBuilder) if glyph in lookup.mapping: @@ -973,7 +973,7 @@ class Builder(object): def add_reverse_chain_single_subst(self, location, old_prefix, old_suffix, mapping): lookup = self.get_lookup_(location, ReverseChainSingleSubstBuilder) - lookup.substitutions.append((old_prefix, old_suffix, mapping)) + lookup.rules.append((old_prefix, old_suffix, mapping)) def add_single_subst(self, location, prefix, suffix, mapping, forceChain): if self.cur_feature_name_ == "aalt": @@ -1007,7 +1007,7 @@ class Builder(object): if sub is None: sub = self.get_chained_lookup_(location, SingleSubstBuilder) sub.mapping.update(mapping) - chain.substitutions.append((prefix, [mapping.keys()], suffix, [sub])) + chain.rules.append((prefix, [mapping.keys()], suffix, [sub])) def add_cursive_pos(self, location, glyphclass, entryAnchor, exitAnchor): lookup = self.get_lookup_(location, CursivePosBuilder) diff --git a/Lib/fontTools/otlLib/builder.py b/Lib/fontTools/otlLib/builder.py index c43abf61a..2ba8a00d4 100644 --- a/Lib/fontTools/otlLib/builder.py +++ b/Lib/fontTools/otlLib/builder.py @@ -156,11 +156,7 @@ class AlternateSubstBuilder(LookupBuilder): self.alternates[(self.SUBTABLE_BREAK_, location)] = self.SUBTABLE_BREAK_ -class ChainContextPosBuilder(LookupBuilder): - def __init__(self, font, location): - LookupBuilder.__init__(self, font, location, 'GPOS', 8) - self.rules = [] # (prefix, input, suffix, lookups) - +class ChainContextualBuilder(LookupBuilder): def equals(self, other): return (LookupBuilder.equals(self, other) and self.rules == other.rules) @@ -170,32 +166,56 @@ class ChainContextPosBuilder(LookupBuilder): for (prefix, glyphs, suffix, lookups) in self.rules: if prefix == self.SUBTABLE_BREAK_: continue - st = ot.ChainContextPos() + st = self.newSubtable_() subtables.append(st) st.Format = 3 self.setBacktrackCoverage_(prefix, st) self.setLookAheadCoverage_(suffix, st) self.setInputCoverage_(glyphs, st) - st.PosCount = 0 - st.PosLookupRecord = [] for sequenceIndex, lookupList in enumerate(lookups): if lookupList is not None: if not isinstance(lookupList, list): # Can happen with synthesised lookups lookupList = [ lookupList ] for l in lookupList: - st.PosCount += 1 if l.lookup_index is None: + if isinstance(self, ChainContextPosBuilder): + other = "substitution" + else: + other = "positioning" raise OpenTypeLibError('Missing index of the specified ' - 'lookup, might be a substitution lookup', + f'lookup, might be a {other} lookup', self.location) - rec = ot.PosLookupRecord() + rec = self.newLookupRecord_() rec.SequenceIndex = sequenceIndex rec.LookupListIndex = l.lookup_index - st.PosLookupRecord.append(rec) + self.addLookupRecordToSubtable_(st, rec) return self.buildLookup_(subtables) + def add_subtable_break(self, location): + self.rules.append((self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, + self.SUBTABLE_BREAK_, [self.SUBTABLE_BREAK_])) + + +class ChainContextPosBuilder(ChainContextualBuilder): + def __init__(self, font, location): + LookupBuilder.__init__(self, font, location, 'GPOS', 8) + self.rules = [] # (prefix, input, suffix, lookups) + + def newSubtable_(self): + st = ot.ChainContextPos() + st.PosCount = 0 + st.PosLookupRecord = [] + return st + + def newLookupRecord_(self): + return ot.PosLookupRecord() + + def addLookupRecordToSubtable_(self, st, rec): + st.PosCount += 1 + st.PosLookupRecord.append(rec) + def find_chainable_single_pos(self, lookups, glyphs, value): """Helper for add_single_pos_chained_()""" res = None @@ -207,55 +227,29 @@ class ChainContextPosBuilder(LookupBuilder): res = lookup return res - def add_subtable_break(self, location): - self.rules.append((self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, - self.SUBTABLE_BREAK_, [self.SUBTABLE_BREAK_])) - -class ChainContextSubstBuilder(LookupBuilder): +class ChainContextSubstBuilder(ChainContextualBuilder): def __init__(self, font, location): LookupBuilder.__init__(self, font, location, 'GSUB', 6) - self.substitutions = [] # (prefix, input, suffix, lookups) + self.rules = [] # (prefix, input, suffix, lookups) - def equals(self, other): - return (LookupBuilder.equals(self, other) and - self.substitutions == other.substitutions) + def newSubtable_(self): + st = ot.ChainContextSubst() + st.SubstCount = 0 + st.SubstLookupRecord = [] + return st - def build(self): - subtables = [] - for (prefix, input, suffix, lookups) in self.substitutions: - if prefix == self.SUBTABLE_BREAK_: - continue - st = ot.ChainContextSubst() - subtables.append(st) - st.Format = 3 - self.setBacktrackCoverage_(prefix, st) - self.setLookAheadCoverage_(suffix, st) - self.setInputCoverage_(input, st) + def newLookupRecord_(self): + return ot.SubstLookupRecord() - st.SubstCount = 0 - st.SubstLookupRecord = [] - for sequenceIndex, lookupList in enumerate(lookups): - if lookupList is not None: - if not isinstance(lookupList, list): - # Can happen with synthesised lookups - lookupList = [ lookupList ] - for l in lookupList: - st.SubstCount += 1 - if l.lookup_index is None: - raise OpenTypeLibError('Missing index of the specified ' - 'lookup, might be a positioning lookup', - self.location) - rec = ot.SubstLookupRecord() - rec.SequenceIndex = sequenceIndex - rec.LookupListIndex = l.lookup_index - st.SubstLookupRecord.append(rec) - return self.buildLookup_(subtables) + def addLookupRecordToSubtable_(self, st, rec): + st.SubstCount += 1 + st.SubstLookupRecord.append(rec) def getAlternateGlyphs(self): result = {} - for (_, _, _, lookuplist) in self.substitutions: - if lookuplist == self.SUBTABLE_BREAK_: + for (prefix, _, _, lookuplist) in self.rules: + if prefix == self.SUBTABLE_BREAK_: continue for lookups in lookuplist: if not isinstance(lookups, list): @@ -270,19 +264,15 @@ class ChainContextSubstBuilder(LookupBuilder): def find_chainable_single_subst(self, glyphs): """Helper for add_single_subst_chained_()""" res = None - for _, _, _, substitutions in self.substitutions[::-1]: - if substitutions == self.SUBTABLE_BREAK_: + for prefix, _, _, rules in self.rules[::-1]: + if prefix == self.SUBTABLE_BREAK_: return res - for sub in substitutions: + for sub in rules: if (isinstance(sub, SingleSubstBuilder) and not any(g in glyphs for g in sub.mapping.keys())): res = sub return res - def add_subtable_break(self, location): - self.substitutions.append((self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_, - self.SUBTABLE_BREAK_, self.SUBTABLE_BREAK_)) - class LigatureSubstBuilder(LookupBuilder): def __init__(self, font, location): @@ -435,15 +425,15 @@ class MarkMarkPosBuilder(LookupBuilder): class ReverseChainSingleSubstBuilder(LookupBuilder): def __init__(self, font, location): LookupBuilder.__init__(self, font, location, 'GSUB', 8) - self.substitutions = [] # (prefix, suffix, mapping) + self.rules = [] # (prefix, suffix, mapping) def equals(self, other): return (LookupBuilder.equals(self, other) and - self.substitutions == other.substitutions) + self.rules == other.rules) def build(self): subtables = [] - for prefix, suffix, mapping in self.substitutions: + for prefix, suffix, mapping in self.rules: st = ot.ReverseChainSingleSubst() st.Format = 1 self.setBacktrackCoverage_(prefix, st)