From fb0d8e151a482c527b3097658c67bae6023d107e Mon Sep 17 00:00:00 2001
From: Colin Rofls <colin@cmyr.net>
Date: Wed, 21 Aug 2024 13:26:43 -0400
Subject: [PATCH] Set GDEF Glyph Categories if not explicit in FEA

This checks whether or not the FEA contained explicit glyph categories,
and if it did not it replaces the categories computed by fea-rs (which
include only glyphs that were used in mark positioning rules) by the
categories either provided in public.openTypeCatgories (in UFO, if set)
or assigned to individual glyphs in glyphsapp.
---
 fontbe/src/features.rs      | 28 ++++++++++++++++++++++++++--
 glyphs2fontir/src/source.rs | 14 ++++++--------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs
index 16d1f606..fdc829f2 100644
--- a/fontbe/src/features.rs
+++ b/fontbe/src/features.rs
@@ -35,7 +35,11 @@ use fontdrasil::{
     orchestration::{Access, AccessBuilder, Work},
     types::Axis,
 };
-use write_fonts::{tables::variations::VariationRegion, types::Tag, OtRound};
+use write_fonts::{
+    tables::{layout::ClassDef, variations::VariationRegion},
+    types::Tag,
+    OtRound,
+};
 
 use crate::{
     error::Error,
@@ -526,6 +530,7 @@ impl Work<Context, AnyWorkId, Error> for FeatureCompilationWork {
 
     fn read_access(&self) -> Access<AnyWorkId> {
         AccessBuilder::new()
+            .variant(FeWorkId::GlyphOrder)
             .variant(WorkId::FeaturesAst)
             .variant(WorkId::GatherBeKerning)
             .variant(WorkId::Marks)
@@ -543,10 +548,29 @@ impl Work<Context, AnyWorkId, Error> for FeatureCompilationWork {
     fn exec(&self, context: &Context) -> Result<(), Error> {
         let static_metadata = context.ir.static_metadata.get();
         let ast = context.fea_ast.get();
+        let glyph_order = context.ir.glyph_order.get();
         let kerns = context.fea_rs_kerns.get();
         let marks = context.fea_rs_marks.get();
 
-        let result = self.compile(&static_metadata, &ast, kerns.as_ref(), marks.as_ref())?;
+        let mut result = self.compile(&static_metadata, &ast, kerns.as_ref(), marks.as_ref())?;
+        if result.gdef_classes.is_none() {
+            // there were no explicit gdef categories, so let's use the ones
+            // that were in public.openTypeCatgories or which we computed from
+            // GlyphData.xml
+
+            if let Some(gdef) = result.gdef.as_mut() {
+                let class_def: ClassDef = static_metadata
+                    .gdef_categories
+                    .categories
+                    .iter()
+                    .filter_map(|(name, cls)| {
+                        glyph_order.glyph_id(name).map(|id| (id, *cls as u16))
+                    })
+                    .collect();
+
+                gdef.glyph_class_def.set(class_def);
+            }
+        }
 
         debug!(
             "Built features, gpos? {} gsub? {} gdef? {}",
diff --git a/glyphs2fontir/src/source.rs b/glyphs2fontir/src/source.rs
index 6f0b8c59..68ca9228 100644
--- a/glyphs2fontir/src/source.rs
+++ b/glyphs2fontir/src/source.rs
@@ -17,9 +17,9 @@ use fontdrasil::{
 use fontir::{
     error::{BadGlyph, BadGlyphKind, BadSource, Error},
     ir::{
-        self, AnchorBuilder, AnchorKind, GdefCategories, GlobalMetric, GlobalMetrics,
-        GlyphInstance, GlyphOrder, KernGroup, KernSide, KerningGroups, KerningInstance,
-        NameBuilder, NameKey, NamedInstance, StaticMetadata, DEFAULT_VENDOR_ID,
+        self, AnchorBuilder, GdefCategories, GlobalMetric, GlobalMetrics, GlyphInstance,
+        GlyphOrder, KernGroup, KernSide, KerningGroups, KerningInstance, NameBuilder, NameKey,
+        NamedInstance, StaticMetadata, DEFAULT_VENDOR_ID,
     },
     orchestration::{Context, IrWork, WorkId},
     source::{Input, Source},
@@ -475,11 +475,9 @@ fn category_for_glyph(glyph: &glyphs_reader::Glyph) -> Option<GlyphClassDef> {
         .layers
         .iter()
         .flat_map(|layer| layer.anchors.iter())
-        .any(|anchor| {
-            AnchorKind::new(&anchor.name)
-                .map(|a| a.is_attaching())
-                .unwrap_or(false)
-        });
+        // glyphsLib considers any anchor that does not start with '_' as an
+        // 'attaching anchor'; see https://github.com/googlefonts/glyphsLib/issues/1024
+        .any(|anchor| !anchor.name.starts_with('_'));
     match (glyph.category, glyph.sub_category) {
         (_, Subcategory::Ligature) if has_attaching_anchor => Some(GlyphClassDef::Ligature),
         (Some(Category::Mark), Subcategory::Nonspacing | Subcategory::SpacingCombining) => {