Merge pull request #560 from googlefonts/merk

Push building a lookup up one level
googlefonts · Nov 10, 2023 · 34e2e2f · 34e2e2f
2 parents 51cfa0f + 08472e4
commit 34e2e2f
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 27 deletions.
diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs
@@ -358,27 +358,28 @@ impl<'a> FeatureWriter<'a> {
         for (group_name, group) in mark_groups.iter() {
             // if we have bases *and* marks produce mark to base
             if !group.bases.is_empty() && !group.marks.is_empty() {
+                let mut mark_base = MarkToBaseBuilder::default();
+
+                for (mark_name, mark_anchor) in group.marks.iter() {
+                    let mark_gid = self.glyph_id(mark_name)?;
+                    let mark_anchor_table = self.create_anchor_table(mark_anchor, builder)?;
+                    mark_base
+                        .insert_mark(mark_gid, group_name.0.into(), mark_anchor_table.clone())
+                        .map_err(Error::PreviouslyAssignedClass)?;
+                }
+
                 for (base_name, base_anchor) in group.bases.iter() {
                     let base_gid = self.glyph_id(base_name)?;
-                    let mut mark_base = MarkToBaseBuilder::default();
-                    for (mark_name, mark_anchor) in group.marks.iter() {
-                        let mark_gid = self.glyph_id(mark_name)?;
-                        let mark_anchor_table = self.create_anchor_table(mark_anchor, builder)?;
-                        mark_base
-                            .insert_mark(mark_gid, group_name.0.into(), mark_anchor_table.clone())
-                            .map_err(Error::PreviouslyAssignedClass)?;
-                    }
-
                     let base_anchor_table = self.create_anchor_table(base_anchor, builder)?;
                     mark_base.insert_base(base_gid, &group_name.0.into(), base_anchor_table);
-
-                    // each in it's own lookup, whch differs from fontmake
-                    mark_base_lookups.push(builder.add_lookup(
-                        LookupFlag::default(),
-                        None,
-                        vec![mark_base],
-                    ));
                 }
+
+                // each mark to base it's own lookup, whch differs from fontmake
+                mark_base_lookups.push(builder.add_lookup(
+                    LookupFlag::default(),
+                    None,
+                    vec![mark_base],
+                ));
             }
 
             // If a mark has anchors that are themselves marks what we got here is a mark to mark

diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs
@@ -362,12 +362,13 @@ mod tests {
 
     use read_fonts::{
         tables::{
-            gpos::{AnchorTable, PositionLookup},
+            gpos::{AnchorTable, Gpos, MarkBasePosFormat1Marker, PositionLookup},
             name::Name,
             os2::SelectionFlags,
             variations::{DeltaSetIndexMap, ItemVariationData},
         },
         types::NameId,
+        TableRef,
     };
     use skrifa::{
         charmap::Charmap,
@@ -2310,6 +2311,19 @@ mod tests {
         }
     }
 
+    fn mark_base_lookups<'a>(gpos: &'a Gpos) -> Vec<TableRef<'a, MarkBasePosFormat1Marker>> {
+        // If only we had more indirections
+        gpos.lookup_list()
+            .iter()
+            .flat_map(|l| l.lookups().iter().map(|l| l.unwrap()))
+            .filter_map(|l| match l {
+                PositionLookup::MarkToBase(mark_base) => Some(mark_base),
+                _ => None,
+            })
+            .flat_map(|mb| mb.subtables().iter().map(|s| s.unwrap()))
+            .collect()
+    }
+
     #[test]
     fn compile_basic_gpos_mark_base() {
         let temp_dir = tempdir().unwrap();
@@ -2328,16 +2342,7 @@ mod tests {
         let brevecomb_gid = GlyphId::new(result.get_glyph_index("brevecomb").unwrap() as u16);
 
         // If only we had more indirections
-        let mark_base_lookups: Vec<_> = gpos
-            .lookup_list()
-            .iter()
-            .flat_map(|l| l.lookups().iter().map(|l| l.unwrap()))
-            .filter_map(|l| match l {
-                PositionLookup::MarkToBase(mark_base) => Some(mark_base),
-                _ => None,
-            })
-            .flat_map(|mb| mb.subtables().iter().map(|s| s.unwrap()))
-            .collect();
+        let mark_base_lookups = mark_base_lookups(&gpos);
 
         let bases = mark_base_lookups
             .iter()
@@ -2455,4 +2460,19 @@ mod tests {
     fn default_fs_type_designspace() {
         assert_fs_type("designspace_from_glyphs/WghtVar.designspace", 1 << 2);
     }
+
+    #[test]
+    fn one_lookup_per_group() {
+        let temp_dir = tempdir().unwrap();
+        let build_dir = temp_dir.path();
+        compile(Args::for_test(build_dir, "glyphs3/Oswald-AE-comb.glyphs"));
+
+        let font_file = build_dir.join("font.ttf");
+        let buf = fs::read(font_file).unwrap();
+        let font = FontRef::new(&buf).unwrap();
+        let gpos = font.gpos().unwrap();
+
+        // We had a bug where it was 2
+        assert_eq!(1, mark_base_lookups(&gpos).len());
+    }
 }