Merge pull request #725 from googlefonts/kern-group-type

Add KernGroup type

fontbe/src/error.rs

@@ -3,7 +3,8 @@ use std::{fmt::Display, io, path::PathBuf};
 use fea_rs::compile::error::CompilerError;
 use fontdrasil::{coords::NormalizedLocation, types::GlyphName};
 use fontir::{
-    error::VariationModelError, orchestration::WorkId as FeWorkId, variations::DeltaError,
+    error::VariationModelError, ir::KernGroup, orchestration::WorkId as FeWorkId,
+    variations::DeltaError,
 };
 use smol_str::SmolStr;
 use thiserror::Error;
@@ -84,6 +85,8 @@ pub enum Error {
     DeltaError(DeltaError),
     #[error("No glyph id for '{0}'")]
     MissingGlyphId(GlyphName),
+    #[error("Missing kern group {0:?}")]
+    MissingKernGroup(KernGroup),
 }
 
 #[derive(Debug)]

fontbe/src/kern.rs

@@ -191,11 +191,11 @@ impl Work<Context, AnyWorkId, Error> for KerningFragmentWork {
                 (KernParticipant::Group(left), KernParticipant::Group(right)) => {
                     let left = glyph_classes
                         .get(left)
-                        .ok_or_else(|| Error::MissingGlyphId(left.clone()))?
+                        .ok_or_else(|| Error::MissingKernGroup(left.clone()))?
                         .clone();
                     let right = glyph_classes
                         .get(right)
-                        .ok_or_else(|| Error::MissingGlyphId(right.clone()))?
+                        .ok_or_else(|| Error::MissingKernGroup(right.clone()))?
                         .clone();
                     kerns.push(PairPosEntry::Class(
                         left,
@@ -211,7 +211,7 @@ impl Work<Context, AnyWorkId, Error> for KerningFragmentWork {
                         .ok_or_else(|| Error::MissingGlyphId(left.clone()))?;
                     let right = glyph_classes
                         .get(right)
-                        .ok_or_else(|| Error::MissingGlyphId(right.clone()))?;
+                        .ok_or_else(|| Error::MissingKernGroup(right.clone()))?;
                     for gid1 in right.iter() {
                         kerns.push(PairPosEntry::Pair(
                             gid0,
@@ -224,7 +224,7 @@ impl Work<Context, AnyWorkId, Error> for KerningFragmentWork {
                 (KernParticipant::Group(left), KernParticipant::Glyph(right)) => {
                     let left = glyph_classes
                         .get(left)
-                        .ok_or_else(|| Error::MissingGlyphId(left.clone()))?;
+                        .ok_or_else(|| Error::MissingKernGroup(left.clone()))?;
                     let gid1 = gid(right)?;
                     for gid0 in left.iter() {
                         kerns.push(PairPosEntry::Pair(

fontbe/src/orchestration.rs

@@ -20,7 +20,7 @@ use fontdrasil::{
     types::GlyphName,
 };
 use fontir::{
-    ir::{Anchor, KernPair},
+    ir::{Anchor, KernGroup, KernPair},
     orchestration::{
         Context as FeContext, ContextItem, ContextMap, Flags, IdAware, Persistable,
         PersistentStorage, WorkId as FeWorkIdentifier,
@@ -423,7 +423,7 @@ pub type KernAdjustments = BTreeMap<NormalizedLocation, OrderedFloat<f32>>;
 /// Every kerning pair we have, taking from IR.
 #[derive(Default, Clone, Serialize, Deserialize, PartialEq)]
 pub struct AllKerningPairs {
-    pub glyph_classes: BTreeMap<GlyphName, GlyphSet>,
+    pub glyph_classes: BTreeMap<KernGroup, GlyphSet>,
     pub adjustments: Vec<(KernPair, KernAdjustments)>,
 }
 

fontc/src/lib.rs

@@ -492,7 +492,7 @@ mod tests {
     };
     use fontdrasil::{coords::NormalizedCoord, paths::safe_filename, types::GlyphName};
     use fontir::{
-        ir::{self, GlyphOrder, KernPair, KernParticipant},
+        ir::{self, GlyphOrder, KernGroup, KernPair, KernParticipant},
         orchestration::{Context as FeContext, Persistable, WorkId as FeWorkIdentifier},
     };
     use indexmap::IndexSet;
@@ -1866,7 +1866,7 @@ mod tests {
             .map(|(name, entries)| {
                 let mut entries: Vec<_> = entries.iter().map(|e| e.as_str()).collect();
                 entries.sort();
-                (name.as_str(), entries)
+                (name.to_owned(), entries)
             })
             .collect();
         groups.sort();
@@ -1906,10 +1906,22 @@ mod tests {
             (groups, kerns),
             (
                 vec![
-                    ("public.kern1.bracketleft_R", vec!["bracketleft"],),
-                    ("public.kern1.bracketright_R", vec!["bracketright"],),
-                    ("public.kern2.bracketleft_L", vec!["bracketleft"],),
-                    ("public.kern2.bracketright_L", vec!["bracketright"],),
+                    (
+                        KernGroup::Side1("bracketleft_R".into()),
+                        vec!["bracketleft"],
+                    ),
+                    (
+                        KernGroup::Side1("bracketright_R".into()),
+                        vec!["bracketright"],
+                    ),
+                    (
+                        KernGroup::Side2("bracketleft_L".into()),
+                        vec!["bracketleft"],
+                    ),
+                    (
+                        KernGroup::Side2("bracketright_L".into()),
+                        vec!["bracketright"],
+                    ),
                 ],
                 vec![
                     (
@@ -1935,7 +1947,7 @@ mod tests {
                     ),
                     (
                         KernParticipant::Glyph("exclam".into()),
-                        KernParticipant::Group("public.kern2.bracketright_L".into()),
+                        KernParticipant::Group(KernGroup::Side2("bracketright_L".into())),
                         vec![("wght 0".to_string(), -160.0),],
                     ),
                     (
@@ -1947,12 +1959,12 @@ mod tests {
                         ],
                     ),
                     (
-                        KernParticipant::Group("public.kern1.bracketleft_R".into()),
+                        KernParticipant::Group(KernGroup::Side1("bracketleft_R".into())),
                         KernParticipant::Glyph("exclam".into()),
                         vec![("wght 0".to_string(), -165.0),],
                     ),
                 ],
-            ),
+            )
         );
     }
 

fontdrasil/src/types.rs

@@ -73,7 +73,6 @@ impl std::borrow::Borrow<str> for GlyphName {
     }
 }
 
-pub type GroupName = GlyphName;
 pub type AnchorName = GlyphName;
 
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]

fontir/Cargo.toml

@@ -31,6 +31,7 @@ parking_lot.workspace = true
 write-fonts.workspace = true  # for pens
 
 chrono.workspace = true
+smol_str.workspace = true
 
 # unique to me!
 blake3 = "1.3.3"

fontir/src/ir.rs

@@ -1,7 +1,7 @@
 //! Font IR types.
 use std::{
     collections::{hash_map::RandomState, BTreeMap, BTreeSet, HashMap, HashSet},
-    fmt::Debug,
+    fmt::{Debug, Display},
     io::Read,
     path::PathBuf,
 };
@@ -11,7 +11,8 @@ use indexmap::IndexSet;
 use kurbo::{Affine, BezPath, PathEl, Point};
 use log::{log_enabled, trace, warn};
 use ordered_float::OrderedFloat;
-use serde::{Deserialize, Serialize};
+use serde::{de::Error, Deserialize, Serialize};
+use smol_str::SmolStr;
 use write_fonts::{
     tables::os2::SelectionFlags,
     types::{GlyphId, NameId, Tag},
@@ -20,7 +21,7 @@ use write_fonts::{
 
 use fontdrasil::{
     coords::{NormalizedCoord, NormalizedLocation, UserLocation},
-    types::{AnchorName, Axis, GlyphName, GroupName},
+    types::{AnchorName, Axis, GlyphName},
 };
 
 use crate::{
@@ -204,7 +205,7 @@ pub type KernPair = (KernParticipant, KernParticipant);
 /// plus the set of locations that can have kerning.
 #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq, Eq)]
 pub struct KerningGroups {
-    pub groups: BTreeMap<GroupName, BTreeSet<GlyphName>>,
+    pub groups: BTreeMap<KernGroup, BTreeSet<GlyphName>>,
     /// The locations that have kerning defined.
     ///
     /// Must be a subset of the master locations.
@@ -213,7 +214,7 @@ pub struct KerningGroups {
     /// Optional group renaming map, meant for [KerningInstance] to consume
     ///
     /// The rhs should be the name used in the groups map.
-    pub old_to_new_group_names: BTreeMap<GroupName, GroupName>,
+    pub old_to_new_group_names: BTreeMap<KernGroup, KernGroup>,
 }
 
 /// IR representation of kerning for a location.
@@ -231,6 +232,16 @@ pub struct KerningInstance {
     pub kerns: BTreeMap<KernPair, OrderedFloat<f32>>,
 }
 
+/// A named set of glyphs with common kerning behaviour
+///
+/// Identical sets can have different behaviour depending on whether or not they
+/// are in the first or second logical position.
+#[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord, Hash)]
+pub enum KernGroup {
+    Side1(SmolStr),
+    Side2(SmolStr),
+}
+
 /// A participant in kerning, one of the entries in a kerning pair.
 ///
 /// Concretely, a glyph or a group of glyphs.
@@ -239,7 +250,42 @@ pub struct KerningInstance {
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum KernParticipant {
     Glyph(GlyphName),
-    Group(GroupName),
+    Group(KernGroup),
+}
+
+impl Display for KernGroup {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            KernGroup::Side1(name) => write!(f, "side1.{name}"),
+            KernGroup::Side2(name) => write!(f, "side2.{name}"),
+        }
+    }
+}
+
+// we need custom impls here because yaml does not support nested enums
+impl Serialize for KernGroup {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        match self {
+            KernGroup::Side1(name) => serializer.serialize_str(&format!("side1.{name}")),
+            KernGroup::Side2(name) => serializer.serialize_str(&format!("side2.{name}")),
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for KernGroup {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        s.strip_prefix("side1.")
+            .map(|s| KernGroup::Side1(s.into()))
+            .or_else(|| s.strip_prefix("side2.").map(|s| KernGroup::Side2(s.into())))
+            .ok_or_else(|| D::Error::custom(format!("missing side1/side2 prefix: {s}")))
+    }
 }
 
 impl KernParticipant {

glyphs2fontir/src/source.rs

@@ -12,12 +12,12 @@ use log::{debug, trace, warn};
 use fontdrasil::{
     coords::{NormalizedCoord, NormalizedLocation},
     orchestration::{Access, AccessBuilder, Work},
-    types::{GlyphName, GroupName},
+    types::GlyphName,
 };
 use fontir::{
     error::{Error, WorkError},
     ir::{
-        self, AnchorBuilder, GlobalMetric, GlobalMetrics, GlyphInstance, GlyphOrder,
+        self, AnchorBuilder, GlobalMetric, GlobalMetrics, GlyphInstance, GlyphOrder, KernGroup,
         KernParticipant, KerningGroups, KerningInstance, NameBuilder, NameKey, NamedInstance,
         StaticMetadata, DEFAULT_VENDOR_ID,
     },
@@ -608,31 +608,17 @@ impl Work<Context, WorkId, WorkError> for FeatureWork {
     }
 }
 
-/// What side of the kern is this, in logical order
-enum KernSide {
-    Side1,
-    Side2,
-}
-
-impl KernSide {
-    fn class_prefix(&self) -> &'static str {
-        match self {
-            KernSide::Side1 => "@MMK_L_",
-            KernSide::Side2 => "@MMK_R_",
-        }
-    }
-
-    fn group_prefix(&self) -> &'static str {
-        match self {
-            KernSide::Side1 => "public.kern1.",
-            KernSide::Side2 => "public.kern2.",
-        }
-    }
+fn parse_kern_group(name: &str) -> Option<KernGroup> {
+    name.strip_prefix(SIDE1_PREFIX)
+        .map(|name| KernGroup::Side1(name.into()))
+        .or_else(|| {
+            name.strip_prefix(SIDE2_PREFIX)
+                .map(|name| KernGroup::Side2(name.into()))
+        })
 }
 
-fn is_kerning_class(name: &str) -> bool {
-    name.starts_with("@MMK_")
-}
+const SIDE1_PREFIX: &str = "@MMK_L_";
+const SIDE2_PREFIX: &str = "@MMK_R_";
 
 #[derive(Debug)]
 struct KerningGroupWork {
@@ -653,29 +639,19 @@ struct AnchorWork {
 /// See <https://github.com/googlefonts/glyphsLib/blob/42bc1db912fd4b66f130fb3bdc63a0c1e774eb38/Lib/glyphsLib/builder/kerning.py#L53-L72>
 fn kern_participant(
     glyph_order: &GlyphOrder,
-    groups: &BTreeMap<GlyphName, BTreeSet<GlyphName>>,
-    side: KernSide,
+    groups: &BTreeMap<KernGroup, BTreeSet<GlyphName>>,
+    expect_prefix: &str,
     raw_side: &str,
 ) -> Option<KernParticipant> {
-    if is_kerning_class(raw_side) {
-        if raw_side.starts_with(side.class_prefix()) {
-            let group_name = format!(
-                "{}{}",
-                side.group_prefix(),
-                raw_side.strip_prefix(side.class_prefix()).unwrap()
-            );
-            let group = GroupName::from(group_name.as_str());
-            if groups.contains_key(&group) {
-                Some(KernParticipant::Group(group))
-            } else {
-                warn!("Invalid kern side: {raw_side}, no group {group_name}");
-                None
-            }
+    if let Some(group) = parse_kern_group(raw_side) {
+        if !raw_side.starts_with(expect_prefix) {
+            warn!("Invalid kern side: {raw_side}, should have prefix {expect_prefix}",);
+            return None;
+        }
+        if groups.contains_key(&group) {
+            Some(KernParticipant::Group(group))
         } else {
-            warn!(
-                "Invalid kern side: {raw_side}, should have prefix {}",
-                side.class_prefix()
-            );
+            warn!("Invalid kern side: {raw_side}, no group {group:?}");
             None
         }
     } else {
@@ -714,14 +690,14 @@ impl Work<Context, WorkId, WorkError> for KerningGroupWork {
                 glyph
                     .right_kern
                     .iter()
-                    .map(|group| (KernSide::Side1, group))
-                    .chain(glyph.left_kern.iter().map(|group| (KernSide::Side2, group)))
-                    .map(|(side, group_name)| {
-                        (
-                            GroupName::from(format!("{}{}", side.group_prefix(), group_name)),
-                            GlyphName::from(glyph_name.as_str()),
-                        )
-                    })
+                    .map(|group| KernGroup::Side1(group.into()))
+                    .chain(
+                        glyph
+                            .left_kern
+                            .iter()
+                            .map(|group| KernGroup::Side2(group.into())),
+                    )
+                    .map(|group| (group, GlyphName::from(glyph_name.as_str())))
             })
             .for_each(|(group_name, glyph_name)| {
                 groups
@@ -792,8 +768,8 @@ impl Work<Context, WorkId, WorkError> for KerningInstanceWork {
                 })
             })
             .filter_map(|((side1, side2), pos_adjust)| {
-                let side1 = kern_participant(glyph_order, groups, KernSide::Side1, side1);
-                let side2 = kern_participant(glyph_order, groups, KernSide::Side2, side2);
+                let side1 = kern_participant(glyph_order, groups, SIDE1_PREFIX, side1);
+                let side2 = kern_participant(glyph_order, groups, SIDE2_PREFIX, side2);
                 let (Some(side1), Some(side2)) = (side1, side2) else {
                     return None;
                 };