From 2c0761934407a66d7ee3088b23bc4b285dc7fafe Mon Sep 17 00:00:00 2001
From: Cosimo Lupo <clupo@google.com>
Date: Thu, 2 Nov 2023 16:32:15 +0000
Subject: [PATCH] [otlLib] make ClassDefBuilder class order match
 varLib.mergers

see https://github.com/fonttools/fonttools/blob/c3d876/Lib/fontTools/misc/classifyTools.py#L77

i.e. we want (large classes first, then lexicographic order on the glyphs); previously otlLib was sorting by the _reverse_ of (small classes first, then glyphs lexicographic order) -- effectively comparing the reverse of the glyph sets of classes of the same size.

Fixes https://github.com/fonttools/fonttools/issues/3321

note the ttx dump of previously built fonts may change but there won't be any functional changes.
---
 Lib/fontTools/otlLib/builder.py        |  2 +-
 Tests/feaLib/data/PairPosSubtable.ttx  | 16 ++++++++--------
 Tests/feaLib/data/bug633.ttx           | 12 ++++++------
 Tests/feaLib/data/spec5f_ii_3.ttx      | 22 +++++++++++-----------
 Tests/feaLib/data/variable_bug2772.ttx | 10 +++++-----
 Tests/otlLib/builder_test.py           |  2 +-
 6 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/Lib/fontTools/otlLib/builder.py b/Lib/fontTools/otlLib/builder.py
index 94628bff1..3508a7e28 100644
--- a/Lib/fontTools/otlLib/builder.py
+++ b/Lib/fontTools/otlLib/builder.py
@@ -2674,7 +2674,7 @@ class ClassDefBuilder(object):
         # class form a contiguous range, the encoding is actually quite
         # compact, whereas a non-contiguous set might need a lot of bytes
         # in the output file. We don't get this right with the key below.
-        result = sorted(self.classes_, key=lambda s: (len(s), s), reverse=True)
+        result = sorted(self.classes_, key=lambda s: (-len(s), s))
         if not self.useClass0_:
             result.insert(0, frozenset())
         return result
diff --git a/Tests/feaLib/data/PairPosSubtable.ttx b/Tests/feaLib/data/PairPosSubtable.ttx
index 2d78f64fa..d671537e6 100644
--- a/Tests/feaLib/data/PairPosSubtable.ttx
+++ b/Tests/feaLib/data/PairPosSubtable.ttx
@@ -93,14 +93,14 @@
           <ValueFormat1 value="4"/>
           <ValueFormat2 value="0"/>
           <ClassDef1>
-            <ClassDef glyph="b" class="1"/>
-            <ClassDef glyph="o" class="1"/>
-          </ClassDef1>
-          <ClassDef2>
-            <ClassDef glyph="c" class="2"/>
-            <ClassDef glyph="d" class="2"/>
             <ClassDef glyph="v" class="1"/>
             <ClassDef glyph="w" class="1"/>
+          </ClassDef1>
+          <ClassDef2>
+            <ClassDef glyph="c" class="1"/>
+            <ClassDef glyph="d" class="1"/>
+            <ClassDef glyph="v" class="2"/>
+            <ClassDef glyph="w" class="2"/>
           </ClassDef2>
           <!-- Class1Count=2 -->
           <!-- Class2Count=3 -->
@@ -112,7 +112,7 @@
               <Value1 XAdvance="0"/>
             </Class2Record>
             <Class2Record index="2">
-              <Value1 XAdvance="-20"/>
+              <Value1 XAdvance="-10"/>
             </Class2Record>
           </Class1Record>
           <Class1Record index="1">
@@ -120,7 +120,7 @@
               <Value1 XAdvance="0"/>
             </Class2Record>
             <Class2Record index="1">
-              <Value1 XAdvance="-10"/>
+              <Value1 XAdvance="-20"/>
             </Class2Record>
             <Class2Record index="2">
               <Value1 XAdvance="0"/>
diff --git a/Tests/feaLib/data/bug633.ttx b/Tests/feaLib/data/bug633.ttx
index 075c17779..8be745cd5 100644
--- a/Tests/feaLib/data/bug633.ttx
+++ b/Tests/feaLib/data/bug633.ttx
@@ -43,10 +43,10 @@
           <ClassDef1>
           </ClassDef1>
           <ClassDef2>
-            <ClassDef glyph="C" class="2"/>
-            <ClassDef glyph="O" class="2"/>
-            <ClassDef glyph="V" class="1"/>
-            <ClassDef glyph="W" class="1"/>
+            <ClassDef glyph="C" class="1"/>
+            <ClassDef glyph="O" class="1"/>
+            <ClassDef glyph="V" class="2"/>
+            <ClassDef glyph="W" class="2"/>
           </ClassDef2>
           <!-- Class1Count=1 -->
           <!-- Class2Count=3 -->
@@ -55,10 +55,10 @@
               <Value1 XAdvance="0"/>
             </Class2Record>
             <Class2Record index="1">
-              <Value1 XAdvance="0"/>
+              <Value1 XAdvance="-20"/>
             </Class2Record>
             <Class2Record index="2">
-              <Value1 XAdvance="-20"/>
+              <Value1 XAdvance="0"/>
             </Class2Record>
           </Class1Record>
         </PairPos>
diff --git a/Tests/feaLib/data/spec5f_ii_3.ttx b/Tests/feaLib/data/spec5f_ii_3.ttx
index a94efceab..c03a81fb7 100644
--- a/Tests/feaLib/data/spec5f_ii_3.ttx
+++ b/Tests/feaLib/data/spec5f_ii_3.ttx
@@ -66,9 +66,9 @@
             <ClassDef glyph="z" class="1"/>
           </BacktrackClassDef>
           <InputClassDef>
-            <ClassDef glyph="a" class="3"/>
+            <ClassDef glyph="a" class="1"/>
             <ClassDef glyph="d" class="2"/>
-            <ClassDef glyph="n" class="1"/>
+            <ClassDef glyph="n" class="3"/>
           </InputClassDef>
           <LookAheadClassDef>
             <ClassDef glyph="a" class="1"/>
@@ -103,18 +103,12 @@
             <!-- ChainSubClassRuleCount=0 -->
           </ChainSubClassSet>
           <ChainSubClassSet index="1">
-            <!-- ChainSubClassRuleCount=0 -->
-          </ChainSubClassSet>
-          <ChainSubClassSet index="2">
-            <!-- ChainSubClassRuleCount=0 -->
-          </ChainSubClassSet>
-          <ChainSubClassSet index="3">
             <!-- ChainSubClassRuleCount=3 -->
             <ChainSubClassRule index="0">
               <!-- BacktrackGlyphCount=1 -->
               <Backtrack index="0" value="1"/>
               <!-- InputGlyphCount=3 -->
-              <Input index="0" value="1"/>
+              <Input index="0" value="3"/>
               <Input index="1" value="2"/>
               <!-- LookAheadGlyphCount=0 -->
               <!-- SubstCount=0 -->
@@ -122,7 +116,7 @@
             <ChainSubClassRule index="1">
               <!-- BacktrackGlyphCount=0 -->
               <!-- InputGlyphCount=3 -->
-              <Input index="0" value="1"/>
+              <Input index="0" value="3"/>
               <Input index="1" value="2"/>
               <!-- LookAheadGlyphCount=1 -->
               <LookAhead index="0" value="1"/>
@@ -131,7 +125,7 @@
             <ChainSubClassRule index="2">
               <!-- BacktrackGlyphCount=0 -->
               <!-- InputGlyphCount=3 -->
-              <Input index="0" value="1"/>
+              <Input index="0" value="3"/>
               <Input index="1" value="2"/>
               <!-- LookAheadGlyphCount=0 -->
               <!-- SubstCount=1 -->
@@ -141,6 +135,12 @@
               </SubstLookupRecord>
             </ChainSubClassRule>
           </ChainSubClassSet>
+          <ChainSubClassSet index="2">
+            <!-- ChainSubClassRuleCount=0 -->
+          </ChainSubClassSet>
+          <ChainSubClassSet index="3">
+            <!-- ChainSubClassRuleCount=0 -->
+          </ChainSubClassSet>
         </ChainContextSubst>
       </Lookup>
       <Lookup index="1">
diff --git a/Tests/feaLib/data/variable_bug2772.ttx b/Tests/feaLib/data/variable_bug2772.ttx
index 465462099..12fda609b 100644
--- a/Tests/feaLib/data/variable_bug2772.ttx
+++ b/Tests/feaLib/data/variable_bug2772.ttx
@@ -73,8 +73,8 @@
           <ClassDef1>
           </ClassDef1>
           <ClassDef2>
-            <ClassDef glyph="g" class="2"/>
-            <ClassDef glyph="y" class="1"/>
+            <ClassDef glyph="g" class="1"/>
+            <ClassDef glyph="y" class="2"/>
           </ClassDef2>
           <!-- Class1Count=1 -->
           <!-- Class2Count=3 -->
@@ -83,6 +83,9 @@
               <Value1 XAdvance="0"/>
             </Class2Record>
             <Class2Record index="1">
+              <Value1 XAdvance="-5"/>
+            </Class2Record>
+            <Class2Record index="2">
               <Value1 XAdvance="0">
                 <XAdvDevice>
                   <StartSize value="0"/>
@@ -91,9 +94,6 @@
                 </XAdvDevice>
               </Value1>
             </Class2Record>
-            <Class2Record index="2">
-              <Value1 XAdvance="-5"/>
-            </Class2Record>
           </Class1Record>
         </PairPos>
       </Lookup>
diff --git a/Tests/otlLib/builder_test.py b/Tests/otlLib/builder_test.py
index 548a31e9f..b7a6caa2f 100644
--- a/Tests/otlLib/builder_test.py
+++ b/Tests/otlLib/builder_test.py
@@ -1080,7 +1080,7 @@ class ClassDefBuilderTest(object):
         b.add({"e", "f", "g", "h"})
         cdef = b.build()
         assert isinstance(cdef, otTables.ClassDef)
-        assert cdef.classDefs == {"a": 2, "b": 2, "c": 3, "aa": 1, "bb": 1}
+        assert cdef.classDefs == {"a": 1, "b": 1, "c": 3, "aa": 2, "bb": 2}
 
     def test_build_notUsingClass0(self):
         b = builder.ClassDefBuilder(useClass0=False)