Merge pull request #791 from googlefonts/move-pending-lookup

[fea] Move PendingLookup into fea-rs

fea-rs/src/compile.rs

@@ -13,7 +13,7 @@ use self::{
 use self::error::UfoGlyphOrderError;
 
 pub use compiler::Compiler;
-pub use feature_writer::{FeatureBuilder, FeatureProvider, NopFeatureProvider};
+pub use feature_writer::{FeatureBuilder, FeatureProvider, NopFeatureProvider, PendingLookup};
 pub use language_system::LanguageSystem;
 pub use lookups::{
     Builder, FeatureKey, LookupId, MarkToBaseBuilder, MarkToMarkBuilder, PairPosBuilder,

fea-rs/src/compile/feature_writer.rs

@@ -46,6 +46,40 @@ pub trait GposSubtableBuilder: Sized {
     ) -> ExternalGposLookup;
 }
 
+/// A lookup generated outside of user FEA
+///
+/// This will be merged into any user-provided features during compilation.
+#[derive(Debug, Default, Clone, PartialEq)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+pub struct PendingLookup<T> {
+    subtables: Vec<T>,
+    flags: LookupFlag,
+    mark_filter_set: Option<GlyphSet>,
+}
+
+impl<T> PendingLookup<T> {
+    /// Create a new lookup.
+    ///
+    /// This can later be added to the feature builder via [`FeatureBuilder::add_lookup`]
+    pub fn new(subtables: Vec<T>, flags: LookupFlag, mark_filter_set: Option<GlyphSet>) -> Self {
+        Self {
+            subtables,
+            flags,
+            mark_filter_set,
+        }
+    }
+
+    /// Return a reference to the subtables in this lookup.
+    pub fn subtables(&self) -> &[T] {
+        &self.subtables
+    }
+
+    /// Return the `LookupFlag` for this lookup.
+    pub fn flags(&self) -> LookupFlag {
+        self.flags
+    }
+}
+
 /// An externally created GPOS lookup.
 ///
 /// This only exists so that we can avoid making our internal types `pub`.
@@ -77,16 +111,17 @@ impl<'a> FeatureBuilder<'a> {
         self.tables.gdef.as_ref()
     }
 
-    /// Create a new lookup.
+    /// Add a lookup to the lookup list.
     ///
-    /// The `LookupId` that is returned can then be included in features
-    pub fn add_lookup<T: GposSubtableBuilder>(
-        &mut self,
-        flags: LookupFlag,
-        filter_set: Option<GlyphSet>,
-        subtables: Vec<T>,
-    ) -> LookupId {
-        let filter_set_id = filter_set.map(|cls| self.get_filter_set_id(cls));
+    /// The `LookupId` that is returned can then be included in features (i.e,
+    /// passed to [`add_feature`](Self::add_feature).)
+    pub fn add_lookup<T: GposSubtableBuilder>(&mut self, lookup: PendingLookup<T>) -> LookupId {
+        let PendingLookup {
+            subtables,
+            flags,
+            mark_filter_set,
+        } = lookup;
+        let filter_set_id = mark_filter_set.map(|cls| self.get_filter_set_id(cls));
         let lookup = T::to_pos_lookup(flags, filter_set_id, subtables);
         let next_id = LookupId::External(self.lookups.len());
         self.lookups.push((next_id, lookup.0));
@@ -113,7 +148,6 @@ impl<'a> FeatureBuilder<'a> {
 
     fn get_filter_set_id(&mut self, cls: GlyphSet) -> FilterSetId {
         let next_id = self.filter_set_id_start + self.mark_filter_sets.len();
-        //.expect("too many filter sets");
         *self.mark_filter_sets.entry(cls).or_insert_with(|| {
             next_id
                 .try_into()

fontbe/src/features.rs

@@ -15,7 +15,10 @@ use log::{debug, error, trace, warn};
 use ordered_float::OrderedFloat;
 
 use fea_rs::{
-    compile::{error::CompilerError, Compilation, FeatureBuilder, FeatureProvider, VariationInfo},
+    compile::{
+        error::CompilerError, Compilation, FeatureBuilder, FeatureProvider, PendingLookup,
+        VariationInfo,
+    },
     parse::{FileSystemResolver, SourceLoadError, SourceResolver},
     DiagnosticSet, GlyphMap, Opts, ParseTree,
 };
@@ -40,13 +43,11 @@ use crate::{
     orchestration::{AnyWorkId, BeWork, Context, FeaAst, FeaRsKerns, FeaRsMarks, WorkId},
 };
 
-mod common;
 mod kern;
 mod marks;
 mod ot_tags;
 mod properties;
 
-pub(crate) use common::PendingLookup;
 pub use kern::{create_gather_ir_kerning_work, create_kern_segment_work, create_kerns_work};
 pub use marks::create_mark_work;
 
@@ -221,13 +222,7 @@ impl<'a> FeatureWriter<'a> {
             .kerning
             .lookups
             .iter()
-            .map(|lookup| {
-                builder.add_lookup(
-                    lookup.flags,
-                    lookup.mark_filter_set.clone(),
-                    lookup.subtables.clone(),
-                )
-            })
+            .map(|lookup| builder.add_lookup(lookup.clone()))
             .collect::<Vec<_>>();
 
         for (feature, ids) in &self.kerning.features {
@@ -271,20 +266,20 @@ impl<'a> FeatureWriter<'a> {
 
         for mark_base in marks.mark_base.iter() {
             // each mark to base it's own lookup, whch differs from fontmake
-            mark_base_lookups.push(builder.add_lookup(
-                LookupFlag::default(),
-                None,
+            mark_base_lookups.push(builder.add_lookup(PendingLookup::new(
                 vec![mark_base.to_owned()],
-            ));
+                LookupFlag::empty(),
+                None,
+            )));
         }
 
         // If a mark has anchors that are themselves marks what we got here is a mark to mark
         for mark_mark in marks.mark_mark.iter() {
-            mark_mark_lookups.push(builder.add_lookup(
+            mark_mark_lookups.push(builder.add_lookup(PendingLookup::new(
+                vec![mark_mark.to_owned()],
                 LookupFlag::default(),
                 None,
-                vec![mark_mark.to_owned()],
-            ));
+            )));
         }
 
         if !mark_base_lookups.is_empty() {

fontbe/src/features/kern.rs

@@ -583,7 +583,7 @@ fn debug_ordered_lookups(
     lookups: &[PendingLookup<PairPosBuilder>],
 ) {
     for (i, lookup) in lookups.iter().enumerate() {
-        let total_rules = lookup.subtables.iter().map(|x| x.len()).sum::<usize>();
+        let total_rules = lookup.subtables().iter().map(|x| x.len()).sum::<usize>();
         log::trace!("lookup {i}, {total_rules} rules");
     }
 
@@ -1143,7 +1143,7 @@ mod tests {
         assert_eq!(
             cyr_rules
                 .iter()
-                .flat_map(|x| x.subtables.iter().map(|sub| sub.len()))
+                .flat_map(|x| x.subtables().iter().map(|sub| sub.len()))
                 .sum::<usize>(),
             1
         );
@@ -1152,16 +1152,20 @@ mod tests {
         assert_eq!(
             latn_rules
                 .iter()
-                .flat_map(|x| x.subtables.iter().map(|sub| sub.len()))
+                .flat_map(|x| x.subtables().iter().map(|sub| sub.len()))
                 .sum::<usize>(),
             2
         );
     }
 
     fn flags_and_rule_count(lookup: &PendingLookup<PairPosBuilder>) -> (LookupFlag, usize) {
         (
-            lookup.flags,
-            lookup.subtables.iter().map(|sub| sub.len()).sum::<usize>(),
+            lookup.flags(),
+            lookup
+                .subtables()
+                .iter()
+                .map(|sub| sub.len())
+                .sum::<usize>(),
         )
     }
 

fontbe/src/orchestration.rs

@@ -11,7 +11,7 @@ use std::{
 
 use fea_rs::{
     compile::{
-        FeatureKey, MarkToBaseBuilder, MarkToMarkBuilder, PairPosBuilder,
+        FeatureKey, MarkToBaseBuilder, MarkToMarkBuilder, PairPosBuilder, PendingLookup,
         ValueRecord as ValueRecordBuilder,
     },
     GlyphMap, GlyphSet, ParseTree,
@@ -63,7 +63,7 @@ use write_fonts::{
     FontWrite,
 };
 
-use crate::{error::Error, features::PendingLookup, paths::Paths};
+use crate::{error::Error, paths::Paths};
 
 type KernBlock = usize;