Merge pull request #723 from googlefonts/fea-rs-gdef-class

[fea-rs] Remove ClassId, add gdef classes to output

fea-rs/src/compile/compile_ctx.rs

@@ -13,7 +13,7 @@ use smol_str::SmolStr;
 use write_fonts::{
     tables::{
         self,
-        gdef::CaretValue,
+        gdef::{CaretValue, GlyphClassDef},
         gpos::ValueFormat,
         layout::{ConditionFormat1, ConditionSet, FeatureVariations, LookupFlag},
         variations::ivs_builder::{RemapVariationIndices, VariationStoreBuilder},
@@ -45,7 +45,7 @@ use super::{
     },
     metrics::{Anchor, DeviceOrDeltas, Metric, ValueRecord},
     output::Compilation,
-    tables::{ClassId, ScriptRecord, Tables},
+    tables::{GlyphClassDefExt, ScriptRecord, Tables},
     tags, VariationInfo,
 };
 
@@ -240,6 +240,10 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
             }
         }
 
+        let gdef_classes = self.tables.gdef.as_ref().and_then(|gdef| {
+            (!gdef.glyph_classes_were_inferred).then(|| gdef.glyph_classes.clone())
+        });
+
         Ok((
             Compilation {
                 head: self.tables.head.as_ref().map(|raw| raw.build(None)),
@@ -253,6 +257,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
                 gsub,
                 gpos,
                 opts: self.opts.clone(),
+                gdef_classes,
             },
             self.errors.clone(),
         ))
@@ -294,6 +299,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
         let mut gdef = self.tables.gdef.take().unwrap_or_default();
         // infer glyph classes, if they were not declared explicitly
         if gdef.glyph_classes.is_empty() {
+            gdef.glyph_classes_were_inferred = true;
             self.lookups.infer_glyph_classes(|glyph, class_id| {
                 gdef.glyph_classes.insert(glyph, class_id);
             });
@@ -303,7 +309,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
                 .flat_map(|class| class.members.iter().map(|(cls, _)| cls.iter()))
                 .flatten()
             {
-                gdef.glyph_classes.insert(glyph, ClassId::Mark);
+                gdef.glyph_classes.insert(glyph, GlyphClassDef::Mark);
             }
         }
 
@@ -1691,19 +1697,20 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
 
                 typed::GdefTableItem::ClassDef(rule) => {
                     for (class, id) in [
-                        (rule.base_glyphs(), ClassId::Base),
-                        (rule.ligature_glyphs(), ClassId::Ligature),
-                        (rule.mark_glyphs(), ClassId::Mark),
-                        (rule.component_glyphs(), ClassId::Component),
+                        (rule.base_glyphs(), GlyphClassDef::Base),
+                        (rule.ligature_glyphs(), GlyphClassDef::Ligature),
+                        (rule.mark_glyphs(), GlyphClassDef::Mark),
+                        (rule.component_glyphs(), GlyphClassDef::Component),
                     ] {
                         let Some(class) = class else {
                             continue;
                         };
                         if let Err((bad_glyph, old_class)) =
                             gdef.add_glyph_class(self.resolve_glyph_class(&class), id)
                         {
-                            let bad_glyph_name = self.reverse_glyph_map.get(&bad_glyph).unwrap();
-                            self.error(class.range(), format!("class includes glyph '{bad_glyph_name}', already in class {old_class}"));
+                            let bad_name = self.reverse_glyph_map.get(&bad_glyph).unwrap();
+                            let class_name = old_class.display();
+                            self.error(class.range(), format!("class includes glyph '{bad_name}', already in class {class_name}"));
                         }
                     }
                 }

fea-rs/src/compile/lookups.rs

@@ -14,6 +14,7 @@ use smol_str::SmolStr;
 
 use write_fonts::{
     tables::{
+        gdef::GlyphClassDef,
         gpos::{self as write_gpos},
         gsub as write_gsub,
         layout::{
@@ -33,7 +34,7 @@ use crate::{
     Kind, Opts,
 };
 
-use super::{features::AllFeatures, metrics::Anchor, tables::ClassId, tags};
+use super::{features::AllFeatures, metrics::Anchor, tags};
 
 use contextual::{
     ContextualLookupBuilder, PosChainContextBuilder, PosContextBuilder, ReverseChainBuilder,
@@ -506,27 +507,35 @@ impl AllLookups {
         )));
     }
 
-    pub(crate) fn infer_glyph_classes(&self, mut f: impl FnMut(GlyphId, ClassId)) {
+    pub(crate) fn infer_glyph_classes(&self, mut f: impl FnMut(GlyphId, GlyphClassDef)) {
         for lookup in &self.gpos {
             match lookup {
                 PositionLookup::MarkToBase(lookup) => {
                     for subtable in &lookup.subtables {
-                        subtable.base_glyphs().for_each(|k| f(k, ClassId::Base));
-                        subtable.mark_glyphs().for_each(|k| f(k, ClassId::Mark));
+                        subtable
+                            .base_glyphs()
+                            .for_each(|k| f(k, GlyphClassDef::Base));
+                        subtable
+                            .mark_glyphs()
+                            .for_each(|k| f(k, GlyphClassDef::Mark));
                     }
                 }
                 PositionLookup::MarkToLig(lookup) => {
                     for subtable in &lookup.subtables {
-                        subtable.lig_glyphs().for_each(|k| f(k, ClassId::Ligature));
-                        subtable.mark_glyphs().for_each(|k| f(k, ClassId::Mark));
+                        subtable
+                            .lig_glyphs()
+                            .for_each(|k| f(k, GlyphClassDef::Ligature));
+                        subtable
+                            .mark_glyphs()
+                            .for_each(|k| f(k, GlyphClassDef::Mark));
                     }
                 }
                 PositionLookup::MarkToMark(lookup) => {
                     for subtable in &lookup.subtables {
                         subtable
                             .mark1_glyphs()
                             .chain(subtable.mark2_glyphs())
-                            .for_each(|k| f(k, ClassId::Mark));
+                            .for_each(|k| f(k, GlyphClassDef::Mark));
                     }
                 }
                 _ => (),

fea-rs/src/compile/output.rs

@@ -1,7 +1,10 @@
 //! The result of a compilation
 
+use std::collections::HashMap;
+
 use write_fonts::{
-    tables::{self as wtables, maxp::Maxp},
+    tables::{self as wtables, gdef::GlyphClassDef, maxp::Maxp},
+    types::GlyphId,
     BuilderError, FontBuilder,
 };
 
@@ -42,6 +45,11 @@ pub struct Compilation {
     pub gsub: Option<wtables::gsub::Gsub>,
     /// The `GPOS` table, if one was generated
     pub gpos: Option<wtables::gpos::Gpos>,
+    /// Any *explicit* gdef classes declared in the FEA.
+    ///
+    /// This is provided so that the user can reference them if they are going
+    /// to manually generate kerning or markpos lookups.
+    pub gdef_classes: Option<HashMap<GlyphId, GlyphClassDef>>,
 }
 
 impl Compilation {

fea-rs/src/compile/tables.rs

@@ -27,7 +27,7 @@ mod os2;
 mod stat;
 
 pub(crate) use base::{BaseBuilder, ScriptRecord};
-pub(crate) use gdef::{ClassId, GdefBuilder};
+pub(crate) use gdef::{GdefBuilder, GlyphClassDefExt};
 pub(crate) use name::{NameBuilder, NameSpec};
 pub(crate) use os2::{CodePageRange, Os2Builder};
 pub(crate) use stat::{AxisLocation, AxisRecord, AxisValue, StatBuilder, StatFallbackName};

fea-rs/src/compile/tables/gdef.rs

@@ -7,6 +7,7 @@
 //! [gdef-spec]: http://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#9b-gdef-table
 
 use std::collections::{BTreeMap, BTreeSet, HashMap};
+use std::fmt::Display;
 
 use write_fonts::types::GlyphId;
 
@@ -24,34 +25,19 @@ use crate::common::{GlyphClass, GlyphSet};
 /// Data collected from a GDEF block.
 #[derive(Clone, Debug, Default)]
 pub struct GdefBuilder {
-    pub glyph_classes: HashMap<GlyphId, ClassId>,
+    pub glyph_classes: HashMap<GlyphId, GlyphClassDef>,
+    /// if `true`, then glyph classes were not declared explicitly.
+    ///
+    /// we track this because it is an important distinction when using the
+    /// glyph classes for manually generated kern/markpos lookups
+    pub glyph_classes_were_inferred: bool,
     pub attach: BTreeMap<GlyphId, BTreeSet<u16>>,
     pub ligature_pos: BTreeMap<GlyphId, Vec<CaretValue>>,
     pub mark_attach_class: BTreeMap<GlyphId, u16>,
     pub mark_glyph_sets: Vec<GlyphSet>,
     pub var_store: Option<VariationStoreBuilder>,
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-#[repr(u16)]
-pub enum ClassId {
-    Base = 1,
-    Ligature = 2,
-    Mark = 3,
-    Component = 4,
-}
-
-impl From<ClassId> for GlyphClassDef {
-    fn from(src: ClassId) -> GlyphClassDef {
-        match src {
-            ClassId::Base => GlyphClassDef::Base,
-            ClassId::Ligature => GlyphClassDef::Ligature,
-            ClassId::Mark => GlyphClassDef::Mark,
-            ClassId::Component => GlyphClassDef::Component,
-        }
-    }
-}
-
 impl GdefBuilder {
     pub fn build(&self) -> (tables::gdef::Gdef, Option<VariationIndexRemapping>) {
         let mut table = tables::gdef::Gdef::new(
@@ -128,8 +114,8 @@ impl GdefBuilder {
         // technically should be a GlyphSet, but we're storing it as individual
         // glyphs and this is private API, so :shrug:
         glyphs: GlyphClass,
-        class: ClassId,
-    ) -> Result<(), (GlyphId, ClassId)> {
+        class: GlyphClassDef,
+    ) -> Result<(), (GlyphId, GlyphClassDef)> {
         for glyph in glyphs.iter() {
             if let Some(prev_class) = self.glyph_classes.insert(glyph, class) {
                 if prev_class != class {
@@ -150,13 +136,18 @@ impl GdefBuilder {
     }
 }
 
-impl std::fmt::Display for ClassId {
-    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+pub(crate) trait GlyphClassDefExt {
+    fn display(&self) -> impl Display;
+}
+
+impl GlyphClassDefExt for GlyphClassDef {
+    fn display(&self) -> impl Display {
         match self {
-            ClassId::Base => write!(f, "Base"),
-            ClassId::Ligature => write!(f, "Ligature"),
-            ClassId::Mark => write!(f, "Mark"),
-            ClassId::Component => write!(f, "Component"),
+            GlyphClassDef::Base => "Base",
+            GlyphClassDef::Ligature => "Ligature",
+            GlyphClassDef::Mark => "Mark",
+            GlyphClassDef::Component => "Component",
+            _ => "Invalid",
         }
     }
 }