Generate GSUB5/GPOS7 lookups (See #1856) #2016
Conversation
Currently unused but will be super helpful for optimizing the formats of contextual lookups. * Splits the rules of a class contextual lookup based on explicit subtable breaks * Returns various properties on the ruleset to help determine appropriate layout format. * (More properties, such as "touched glyphs", planned - will be added when needed.)
…t 3 lookups right now.
It's a bit gross, but I'm blaming OpenType for that. I'm doing this now because otherwise it would be even more repetitive once we start adding format 1 Rule and Ruleset subtables.
|
I now have another couple of commits on top of this which add Format 1 subtables where possible, but will save that for another PR as and when this one gets merged. |
There was a problem hiding this comment.
thanks Simon. I just had a quick look, I only left some stylistic comments below.
I see you adjusted/renamed some tests since they now use the non-chaining contextual types (they were indeed non-chaining types disguised as chaining). But maybe we should add similar tests to ensure the code keeps working with the chaining ones as well.
Lib/fontTools/otlLib/builder.py
Outdated
| # Do we have any prefixes/suffixes? If this is False for all | ||
| # rulesets, we can express the whole lookup as GPOS5/GSUB7. | ||
| for (prefix, glyphs, suffix, lookups) in self.rules: | ||
| if len(prefix) > 0 or len(suffix) > 0: return True |
There was a problem hiding this comment.
if statement split over two lines, please
Lib/fontTools/otlLib/builder.py
Outdated
| # we can express this subtable as a Format 1. | ||
| for (prefix, glyphs, suffix, lookups) in self.rules: | ||
| for coverage in (prefix, glyphs, suffix): | ||
| if any([len(x) > 1 for x in coverage]): |
There was a problem hiding this comment.
| if any([len(x) > 1 for x in coverage]): | |
| if any(len(x) > 1 for x in coverage): |
Lib/fontTools/otlLib/builder.py
Outdated
| return True | ||
| return False | ||
|
|
||
| def format2Classdefs(self): |
There was a problem hiding this comment.
| def format2Classdefs(self): | |
| def format2ClassDefs(self): |
Lib/fontTools/otlLib/builder.py
Outdated
|
|
||
| def format2Classdefs(self): | ||
| PREFIX, GLYPHS, SUFFIX = 0,1,2 | ||
| classdefbuilders = [] |
There was a problem hiding this comment.
| classdefbuilders = [] | |
| classDefBuilders = [] |
I think we should stick to one style for the entire module. If the rest uses camelCase, then new code should follow.
Lib/fontTools/otlLib/builder.py
Outdated
| def rulesets(self): | ||
| # Return a list of ChainContextRuleset objects, taking explicit | ||
| # subtable breaks into account | ||
| ruleset = [ ChainContextualRuleset() ] |
There was a problem hiding this comment.
there's no reason to add spaces to square brackets, it's just distracting. I know the rest of the codebase is not consistent, but for at least new code let's stick to black formatting so we don't waste time over style.
| ruleset = [ ChainContextualRuleset() ] | |
| ruleset = [ChainContextualRuleset()] |
Lib/fontTools/otlLib/builder.py
Outdated
| st.Format = 3 | ||
| chaining = False | ||
| rulesets = self.rulesets() | ||
| chaining = any([ruleset.hasPrefixOrSuffix for ruleset in rulesets]) |
There was a problem hiding this comment.
| chaining = any([ruleset.hasPrefixOrSuffix for ruleset in rulesets]) | |
| chaining = any(ruleset.hasPrefixOrSuffix for ruleset in rulesets) |
Lib/fontTools/otlLib/builder.py
Outdated
| if lookupList is not None: | ||
| if not isinstance(lookupList, list): | ||
| # Can happen with synthesised lookups | ||
| lookupList = [ lookupList ] |
There was a problem hiding this comment.
| lookupList = [ lookupList ] | |
| lookupList = [lookupList] |
Tests/otlLib/builder_test.py
Outdated
| font = ttLib.TTFont() | ||
| font.setGlyphOrder(["a","b","c","d","A","B","C","D","E"]) | ||
| sb = builder.ChainContextSubstBuilder(font, None) | ||
| prefix = [ ["a"], ["b"] ] |
There was a problem hiding this comment.
| prefix = [ ["a"], ["b"] ] | |
| prefix = [["a"], ["b"]] |
and all the following lines as well
Tests/otlLib/builder_test.py
Outdated
| prefix = [ ["A"] ] | ||
| input_ = [ ["E"] ] | ||
| sb.rules.append((prefix, input_, suffix, lookups)) | ||
| input_ = [ ["C","D"] ] |
There was a problem hiding this comment.
| input_ = [ ["C","D"] ] | |
| input_ = [["C", "D"]] |
| suffix = [] | ||
| sb.rules.append((prefix, input_, suffix, lookups)) | ||
| input_ = [ ["C","D"] ] | ||
| sb.rules.append((prefix, input_, suffix, lookups)) |
There was a problem hiding this comment.
it's hard to track the state of the prefix, input_, suffix local variables as you mutate them inside the test. maybe spell out the content of each every time you append a new rule (even if it means repeating the same string literals) so it's clear what rules contain what.
Done in fairly small steps. Note that the ClassContextualRuleset object which this introduces is also going to be used when choosing the appropriate format representation of contextual lookups, so this is also the start of the contextual optimizer.