From 00dddb1c32d3d57c4a57c7296e891e6543bcafeb Mon Sep 17 00:00:00 2001
From: Michiharu Ariza <ariza@adobe.com>
Date: Wed, 24 Apr 2019 16:46:21 -0700
Subject: [PATCH] fixed non retain-gids with no AdvWidthMap behavior

deltas in VarData[0] are directly look up by GID so compacted
if any deltas are referenced by LsbMap / RsbMap but not used for advance widths, they are moved to the end of VarData[0]

updated expected test result expect_HVVAR.ttx accordingly
---
 Lib/fontTools/subset/__init__.py   | 14 ++++++++++----
 Lib/fontTools/varLib/varStore.py   | 10 ++++++++--
 Tests/subset/data/expect_HVVAR.ttx | 17 ++++++++---------
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/Lib/fontTools/subset/__init__.py b/Lib/fontTools/subset/__init__.py
index ebd99728f..108de4f31 100644
--- a/Lib/fontTools/subset/__init__.py
+++ b/Lib/fontTools/subset/__init__.py
@@ -1803,15 +1803,17 @@ def _remap_index_map(s, varidx_map, table_map):
 def subset_glyphs(self, s):
 	table = self.table
 
-	# TODO Update for retain_gids
-
 	used = set()
+	advIdxes_ = set()
+	retainAdvMap = False
 
 	if table.AdvWidthMap:
 		table.AdvWidthMap.mapping = _dict_subset(table.AdvWidthMap.mapping, s.glyphs)
 		used.update(table.AdvWidthMap.mapping.values())
 	else:
 		used.update(s.reverseOrigGlyphMap.values())
+		advIdxes_ = used.copy()
+		retainAdvMap = s.options.retain_gids
 
 	if table.LsbMap:
 		table.LsbMap.mapping = _dict_subset(table.LsbMap.mapping, s.glyphs)
@@ -1820,7 +1822,7 @@ def subset_glyphs(self, s):
 		table.RsbMap.mapping = _dict_subset(table.RsbMap.mapping, s.glyphs)
 		used.update(table.RsbMap.mapping.values())
 
-	varidx_map = varStore.VarStore_subset_varidxes(table.VarStore, used, retainFirstMap=table.AdvWidthMap is None)
+	varidx_map = varStore.VarStore_subset_varidxes(table.VarStore, used, retainFirstMap=retainAdvMap, advIdxes=advIdxes_)
 
 	if table.AdvWidthMap:
 		table.AdvWidthMap.mapping = _remap_index_map(s, varidx_map, table.AdvWidthMap)
@@ -1837,12 +1839,16 @@ def subset_glyphs(self, s):
 	table = self.table
 
 	used = set()
+	advIdxes_ = set()
+	retainAdvMap = False
 
 	if table.AdvHeightMap:
 		table.AdvHeightMap.mapping = _dict_subset(table.AdvHeightMap.mapping, s.glyphs)
 		used.update(table.AdvHeightMap.mapping.values())
 	else:
 		used.update(s.reverseOrigGlyphMap.values())
+		advIdxes_ = used.copy()
+		retainAdvMap = s.options.retain_gids
 
 	if table.TsbMap:
 		table.TsbMap.mapping = _dict_subset(table.TsbMap.mapping, s.glyphs)
@@ -1854,7 +1860,7 @@ def subset_glyphs(self, s):
 		table.VOrgMap.mapping = _dict_subset(table.VOrgMap.mapping, s.glyphs)
 		used.update(table.VOrgMap.mapping.values())
 
-	varidx_map = varStore.VarStore_subset_varidxes(table.VarStore, used, retainFirstMap=table.AdvHeightMap is None)
+	varidx_map = varStore.VarStore_subset_varidxes(table.VarStore, used, retainFirstMap=retainAdvMap, advIdxes=advIdxes_)
 
 	if table.AdvHeightMap:
 		table.AdvHeightMap.mapping = _remap_index_map(s, varidx_map, table.AdvHeightMap)
diff --git a/Lib/fontTools/varLib/varStore.py b/Lib/fontTools/varLib/varStore.py
index e6609a689..f8ce81996 100644
--- a/Lib/fontTools/varLib/varStore.py
+++ b/Lib/fontTools/varLib/varStore.py
@@ -187,8 +187,10 @@ class VarStoreInstancer(object):
 #
 # Optimizations
 #
+# retainFirstMap - If true, major 0 mappings are retained. Deltas for unused indices are zeroed
+# advIdxes - Set of major 0 indices for advance deltas to be listed first. Other major 0 indices follow.
 
-def VarStore_subset_varidxes(self, varIdxes, optimize=True, retainFirstMap=False):
+def VarStore_subset_varidxes(self, varIdxes, optimize=True, retainFirstMap=False, advIdxes=set()):
 
 	# Sort out used varIdxes by major/minor.
 	used = {}
@@ -222,7 +224,11 @@ def VarStore_subset_varidxes(self, varIdxes, optimize=True, retainFirstMap=False
 				newItems.append(items[minor] if minor in usedMinors else [0] * len(items[minor]))
 				varDataMap[minor] = minor
 		else:
-			for minor in sorted(usedMinors):
+			if major == 0:
+				minors = sorted(advIdxes) + sorted(usedMinors - advIdxes)
+			else:
+				minors = sorted(usedMinors)
+			for minor in minors:
 				newMinor = len(newItems)
 				newItems.append(items[minor])
 				varDataMap[(major<<16)+minor] = (newMajor<<16)+newMinor
diff --git a/Tests/subset/data/expect_HVVAR.ttx b/Tests/subset/data/expect_HVVAR.ttx
index 08d35a887..5fbc1770d 100644
--- a/Tests/subset/data/expect_HVVAR.ttx
+++ b/Tests/subset/data/expect_HVVAR.ttx
@@ -32,16 +32,15 @@
       </VarRegionList>
       <!-- VarDataCount=2 -->
       <VarData index="0">
-        <!-- ItemCount=6 -->
+        <!-- ItemCount=5 -->
         <NumShorts value="0"/>
         <!-- VarRegionCount=1 -->
         <VarRegionIndex index="0" value="1"/>
         <Item index="0" value="[1]"/>
-        <Item index="1" value="[2]"/>
-        <Item index="2" value="[3]"/>
-        <Item index="3" value="[4]"/>
-        <Item index="4" value="[5]"/>
-        <Item index="5" value="[0]"/>
+        <Item index="1" value="[3]"/>
+        <Item index="2" value="[5]"/>
+        <Item index="3" value="[2]"/>
+        <Item index="4" value="[4]"/>
       </VarData>
       <VarData index="1">
         <!-- ItemCount=3 -->
@@ -55,12 +54,12 @@
       </VarData>
     </VarStore>
     <LsbMap>
-      <Map glyph=".notdef" outer="0" inner="2"/>
+      <Map glyph=".notdef" outer="0" inner="1"/>
       <Map glyph="B" outer="1" inner="0"/>
-      <Map glyph="D" outer="0" inner="3"/>
+      <Map glyph="D" outer="0" inner="4"/>
     </LsbMap>
     <RsbMap>
-      <Map glyph=".notdef" outer="0" inner="1"/>
+      <Map glyph=".notdef" outer="0" inner="3"/>
       <Map glyph="B" outer="1" inner="2"/>
       <Map glyph="D" outer="1" inner="1"/>
     </RsbMap>