[otlLib] Build multiple glyph-based PairPos subtables when needed

Before this change, feaLib would group glyph-based pair positionings
by value formats. After this change, this logic happens in otlLib.
But clients can still do their own grouping if they wish, by calling
the buildPairPosGlyphsSubtable() method directly.

Lib/fontTools/feaLib/builder.py

@@ -888,34 +888,28 @@ class MultipleSubstBuilder(LookupBuilder):
 class SpecificPairPosBuilder(LookupBuilder):
     def __init__(self, font, location):
         LookupBuilder.__init__(self, font, location, 'GPOS', 2)
-        self.pairs = {}  # (gc1, gc2) -> (location, value1, value2)
+        self.pairs = {}  # (glyph1, glyph2) -> (value1, value2)
+        self.locations = {}  # (glyph1, glyph2) -> (filepath, line, column)
 
     def add_pair(self, location, glyph1, value1, glyph2, value2):
-        oldValue = self.pairs.get((glyph1, glyph2), None)
+        key = (glyph1, glyph2)
+        oldValue = self.pairs.get(key, None)
         if oldValue is not None:
-            otherLoc, _, _ = oldValue
+            otherLoc = self.locations[key]
             raise FeatureLibError(
                 'Already defined position for pair %s %s at %s:%d:%d'
                 % (glyph1, glyph2, otherLoc[0], otherLoc[1], otherLoc[2]),
                 location)
-        self.pairs[(glyph1, glyph2)] = (location, value1, value2)
+        val1, _ = makeOpenTypeValueRecord(value1)
+        val2, _ = makeOpenTypeValueRecord(value2)
+        self.pairs[key] = (val1, val2)
+        self.locations[key] = location
 
     def equals(self, other):
-        return (LookupBuilder.equals(self, other) and
-                self.pairs == other.pairs)
+        return (LookupBuilder.equals(self, other) and self.pairs == other.pairs)
 
     def build(self):
-        subtables = []
-        # (valueFormat1, valueFormat2) --> {(glyph1, glyph2): (val1, val2)}
-        format1 = {}
-        for (glyph1, glyph2), (location, value1, value2) in self.pairs.items():
-            val1, valFormat1 = makeOpenTypeValueRecord(value1)
-            val2, valFormat2 = makeOpenTypeValueRecord(value2)
-            p = format1.setdefault(((valFormat1, valFormat2)), {})
-            p[(glyph1, glyph2)] = (val1, val2)
-        for (vf1, vf2), pairs in sorted(format1.items()):
-            subtables.append(
-                otl.buildPairPosGlyphsSubtable(pairs, self.glyphMap, vf1, vf2))
+        subtables = otl.buildPairPosGlyphs(self.pairs, self.glyphMap)
         return self.buildLookup_(subtables)
 
 

Lib/fontTools/otlLib/builder.py

@@ -310,6 +310,18 @@ def buildMark2Record(anchors):
     return self
 
 
+def buildPairPosGlyphs(pairs, glyphMap):
+    p = {}  # (formatA, formatB) --> {(glyphA, glyphB): (valA, valB)}
+    for (glyphA, glyphB), (valA, valB) in pairs.items():
+        formatA = valA.getFormat() if valA is not None else 0
+        formatB = valB.getFormat() if valB is not None else 0
+        pos = p.setdefault((formatA, formatB), {})
+        pos[(glyphA, glyphB)] = (valA, valB)
+    return [
+        buildPairPosGlyphsSubtable(pos, glyphMap, formatA, formatB)
+        for ((formatA, formatB), pos) in sorted(p.items())]
+
+
 def buildPairPosGlyphsSubtable(pairs, glyphMap,
                                valueFormat1=None, valueFormat2=None):
     self = ot.PairPos()

Lib/fontTools/otlLib/builder_test.py

@@ -768,6 +768,47 @@ class BuilderTest(unittest.TestCase):
                          '  </Mark2Anchor>'
                          '</Mark2Record>')
 
+    def test_buildPairPosGlyphs(self):
+        d50 = builder.buildValue({"XPlacement": -50})
+        d8020 = builder.buildValue({"XPlacement": -80, "YPlacement": -20})
+        subtables = builder.buildPairPosGlyphs({
+            ("A", "zero"): (None, d50),
+            ("A", "one"):  (d8020, d50),
+        }, self.GLYPHMAP)
+        self.maxDiff = None
+        self.assertEqual(''.join([getXML(t.toXML) for t in subtables]),
+                         '<PairPos Format="1">'
+                         '  <Coverage>'
+                         '    <Glyph value="A"/>'
+                         '  </Coverage>'
+                         '  <ValueFormat1 value="0"/>'
+                         '  <ValueFormat2 value="1"/>'
+                         '  <!-- PairSetCount=1 -->'
+                         '  <PairSet index="0">'
+                         '    <!-- PairValueCount=1 -->'
+                         '    <PairValueRecord index="0">'
+                         '      <SecondGlyph value="zero"/>'
+                         '      <Value2 XPlacement="-50"/>'
+                         '    </PairValueRecord>'
+                         '  </PairSet>'
+                         '</PairPos>'
+                         '<PairPos Format="1">'
+                         '  <Coverage>'
+                         '    <Glyph value="A"/>'
+                         '  </Coverage>'
+                         '  <ValueFormat1 value="3"/>'
+                         '  <ValueFormat2 value="1"/>'
+                         '  <!-- PairSetCount=1 -->'
+                         '  <PairSet index="0">'
+                         '    <!-- PairValueCount=1 -->'
+                         '    <PairValueRecord index="0">'
+                         '      <SecondGlyph value="one"/>'
+                         '      <Value1 XPlacement="-80" YPlacement="-20"/>'
+                         '      <Value2 XPlacement="-50"/>'
+                         '    </PairValueRecord>'
+                         '  </PairSet>'
+                         '</PairPos>')
+
     def test_buildPairPosGlyphsSubtable(self):
         d20 = builder.buildValue({"XPlacement": -20})
         d50 = builder.buildValue({"XPlacement": -50})