Merge pull request #1131 from googlefonts/remove-set-unconditionally

Remove `set_unconditionally` & `BeValue`

fontbe/src/avar.rs

@@ -5,9 +5,10 @@ use fontdrasil::{
     orchestration::{Access, Work},
     types::Axis,
 };
-use fontir::orchestration::WorkId as FeWorkId;
+use fontir::orchestration::{Persistable, WorkId as FeWorkId};
 use log::debug;
 use write_fonts::{
+    read::FontRead,
     tables::avar::{Avar, AxisValueMap, SegmentMaps},
     types::F2Dot14,
 };
@@ -17,6 +18,45 @@ use crate::{
     orchestration::{AnyWorkId, BeWork, Context, WorkId},
 };
 
+/// Avar is a special case where sometimes we want to explicitly have an empty one.
+///
+/// We can't just store `Option` because we can't impl Persistable for it
+/// (doing so conflicts with the generic impl for write-fonts types)
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub enum PossiblyEmptyAvar {
+    NonEmpty(Avar),
+    Empty,
+}
+
+impl Persistable for PossiblyEmptyAvar {
+    fn read(from: &mut dyn std::io::Read) -> Self {
+        let mut bytes = Vec::new();
+        from.read_to_end(&mut bytes).unwrap();
+        if bytes.is_empty() {
+            Self::Empty
+        } else {
+            let table =
+                FontRead::read(bytes.as_slice().into()).expect("we wrote it, we can write it");
+            Self::NonEmpty(table)
+        }
+    }
+
+    fn write(&self, to: &mut dyn std::io::Write) {
+        if let Self::NonEmpty(table) = self {
+            table.write(to);
+        }
+    }
+}
+
+impl PossiblyEmptyAvar {
+    pub fn as_ref(&self) -> Option<&Avar> {
+        match self {
+            PossiblyEmptyAvar::NonEmpty(avar) => Some(avar),
+            PossiblyEmptyAvar::Empty => None,
+        }
+    }
+}
+
 #[derive(Debug)]
 struct AvarWork {}
 
@@ -103,11 +143,12 @@ impl Work<Context, AnyWorkId, Error> for AvarWork {
         }
         let axis_segment_maps: Vec<_> = static_metadata.axes.iter().map(to_segment_map).collect();
         // only when all the segment maps are uninteresting, we can omit avar
-        let avar = axis_segment_maps
-            .iter()
-            .any(|segmap| !segmap.is_identity())
-            .then(|| Avar::new(axis_segment_maps));
-        context.avar.set_unconditionally(avar.into());
+        let avar = if axis_segment_maps.iter().any(|segmap| !segmap.is_identity()) {
+            PossiblyEmptyAvar::NonEmpty(Avar::new(axis_segment_maps))
+        } else {
+            PossiblyEmptyAvar::Empty
+        };
+        context.avar.set(avar);
         Ok(())
     }
 }

fontbe/src/cmap.rs

@@ -52,7 +52,7 @@ impl Work<Context, AnyWorkId, Error> for CmapWork {
             });
 
         let cmap = Cmap::from_mappings(mappings)?;
-        context.cmap.set_unconditionally(cmap.into());
+        context.cmap.set(cmap);
         Ok(())
     }
 }

fontbe/src/features.rs

@@ -584,13 +584,13 @@ impl Work<Context, AnyWorkId, Error> for FeatureCompilationWork {
             result.gdef.is_some(),
         );
         if let Some(gpos) = result.gpos {
-            context.gpos.set_unconditionally(gpos.into());
+            context.gpos.set(gpos);
         }
         if let Some(gsub) = result.gsub {
-            context.gsub.set_unconditionally(gsub.into());
+            context.gsub.set(gsub);
         }
         if let Some(gdef) = result.gdef {
-            context.gdef.set_unconditionally(gdef.into());
+            context.gdef.set(gdef);
         }
 
         // Enables the assumption that if the file exists features were compiled

fontbe/src/font.rs

@@ -81,7 +81,7 @@ fn has(context: &Context, id: WorkId) -> bool {
 fn bytes_for(context: &Context, id: WorkId) -> Result<Option<Vec<u8>>, Error> {
     // TODO: to_vec copies :(
     let bytes = match id {
-        WorkId::Avar => to_bytes(context.avar.get().as_ref()),
+        WorkId::Avar => context.avar.get().as_ref().as_ref().and_then(to_bytes),
         WorkId::Cmap => to_bytes(context.cmap.get().as_ref()),
         WorkId::Fvar => to_bytes(context.fvar.get().as_ref()),
         WorkId::Head => to_bytes(context.head.get().as_ref()),
@@ -163,7 +163,7 @@ impl Work<Context, AnyWorkId, Error> for FontWork {
         debug!("Building font");
         let font = builder.build();
         debug!("Assembled {} byte font", font.len());
-        context.font.set_unconditionally(font.into());
+        context.font.set(font.into());
         Ok(())
     }
 }

fontbe/src/fvar.rs

@@ -93,7 +93,7 @@ impl Work<Context, AnyWorkId, Error> for FvarWork {
     /// Generate [fvar](https://learn.microsoft.com/en-us/typography/opentype/spec/fvar)
     fn exec(&self, context: &Context) -> Result<(), Error> {
         if let Some(fvar) = generate_fvar(&context.ir.static_metadata.get()) {
-            context.fvar.set_unconditionally(fvar.into());
+            context.fvar.set(fvar);
         }
         Ok(())
     }

fontbe/src/gvar.rs

@@ -76,7 +76,7 @@ impl Work<Context, AnyWorkId, Error> for GvarWork {
                 context: "gvar".into(),
             })?
             .into();
-        context.gvar.set_unconditionally(raw_gvar);
+        context.gvar.set(raw_gvar);
 
         Ok(())
     }

fontbe/src/head.rs

@@ -132,7 +132,7 @@ impl Work<Context, AnyWorkId, Error> for HeadWork {
         );
         apply_created_modified(&mut head, static_metadata.misc.created);
         apply_macstyle(&mut head, static_metadata.misc.selection_flags);
-        context.head.set_unconditionally(head.into());
+        context.head.set(head);
 
         // Defer x/y Min/Max to metrics and limits job
 

fontbe/src/hvar.rs

@@ -228,7 +228,7 @@ impl Work<Context, AnyWorkId, Error> for HvarWork {
         }
 
         let hvar = Hvar::new(MajorMinor::VERSION_1_0, varstore, varidx_map, None, None);
-        context.hvar.set_unconditionally(hvar.into());
+        context.hvar.set(hvar);
 
         Ok(())
     }

fontbe/src/metrics_and_limits.rs

@@ -5,6 +5,7 @@
 use std::{
     cmp::{max, min},
     collections::HashMap,
+    sync::Arc,
 };
 
 use fontdrasil::orchestration::{Access, AccessBuilder, Work};
@@ -309,7 +310,7 @@ impl Work<Context, AnyWorkId, Error> for MetricAndLimitWork {
                 }
             })?,
         };
-        context.hhea.set_unconditionally(hhea.into());
+        context.hhea.set(hhea);
 
         // Send hmtx out into the world
         let hmtx = Hmtx::new(long_metrics, lsbs);
@@ -319,7 +320,7 @@ impl Work<Context, AnyWorkId, Error> for MetricAndLimitWork {
                 context: "hmtx".into(),
             })?
             .into();
-        context.hmtx.set_unconditionally(raw_hmtx);
+        context.hmtx.set(raw_hmtx);
 
         // Might as well do maxp while we're here
         let composite_limits = glyph_limits.update_composite_limits();
@@ -341,16 +342,16 @@ impl Work<Context, AnyWorkId, Error> for MetricAndLimitWork {
             max_component_elements: Some(glyph_limits.max_component_elements),
             max_component_depth: Some(composite_limits.max_depth),
         };
-        context.maxp.set_unconditionally(maxp.into());
+        context.maxp.set(maxp);
 
         // Set x/y min/max in head
-        let mut head = context.head.get().0.clone().unwrap();
+        let mut head = Arc::unwrap_or_clone(context.head.get());
         let bbox = glyph_limits.bbox.unwrap_or_default();
         head.x_min = bbox.x_min;
         head.y_min = bbox.y_min;
         head.x_max = bbox.x_max;
         head.y_max = bbox.y_max;
-        context.head.set_unconditionally(head.into());
+        context.head.set(head);
 
         Ok(())
     }

fontbe/src/mvar.rs

@@ -152,9 +152,9 @@ impl Work<Context, AnyWorkId, Error> for MvarWork {
                 mvar_builder.add_sources(mvar_tag, values)?;
             }
         }
-        let mvar = mvar_builder.build();
-
-        context.mvar.set_unconditionally(mvar.into());
+        if let Some(mvar) = mvar_builder.build() {
+            context.mvar.set(mvar);
+        }
 
         Ok(())
     }

fontbe/src/name.rs

@@ -46,7 +46,7 @@ impl Work<Context, AnyWorkId, Error> for NameWork {
 
         context
             .name
-            .set_unconditionally(Name::new(name_records.into_iter().collect()).into());
+            .set(Name::new(name_records.into_iter().collect()));
         Ok(())
     }
 }

fontbe/src/orchestration.rs

@@ -36,9 +36,8 @@ use serde::{Deserialize, Serialize};
 
 use write_fonts::{
     dump_table,
-    read::{FontData, FontRead},
+    read::FontRead,
     tables::{
-        avar::Avar,
         cmap::Cmap,
         fvar::Fvar,
         gdef::Gdef,
@@ -63,7 +62,7 @@ use write_fonts::{
     FontWrite,
 };
 
-use crate::{error::Error, paths::Paths};
+use crate::{avar::PossiblyEmptyAvar, error::Error, paths::Paths};
 
 type KernBlock = usize;
 
@@ -235,7 +234,7 @@ impl IdAware<AnyWorkId> for Glyph {
 impl Persistable for Glyph {
     fn read(from: &mut dyn Read) -> Self {
         let (name, bytes): (GlyphName, Vec<u8>) = bincode::deserialize_from(from).unwrap();
-        let glyph = RawGlyph::read(bytes.as_slice().into()).unwrap();
+        let glyph = FontRead::read(bytes.as_slice().into()).unwrap();
         Glyph { name, data: glyph }
     }
 
@@ -638,51 +637,6 @@ impl Persistable for LocaFormatWrapper {
 
 pub type BeWork = dyn Work<Context, AnyWorkId, Error> + Send;
 
-/// Allows us to implement [Persistable] w/o violating the orphan rules
-///
-/// Other than that just kind of gets in the way
-pub struct BeValue<T>(pub Option<T>);
-
-impl<T> Persistable for BeValue<T>
-where
-    for<'a> T: FontRead<'a> + FontWrite + Validate,
-{
-    fn read(from: &mut dyn Read) -> Self {
-        let mut buf = Vec::new();
-        from.read_to_end(&mut buf).unwrap();
-        (!buf.is_empty())
-            .then(|| T::read(FontData::new(&buf)).unwrap())
-            .into()
-    }
-
-    fn write(&self, to: &mut dyn io::Write) {
-        let bytes = self
-            .0
-            .as_ref()
-            .map(|t| dump_table(t).unwrap())
-            .unwrap_or_default();
-        to.write_all(&bytes).unwrap();
-    }
-}
-
-impl<T> From<T> for BeValue<T>
-where
-    T: FontWrite + Validate,
-{
-    fn from(value: T) -> Self {
-        BeValue(Some(value))
-    }
-}
-
-impl<T> From<Option<T>> for BeValue<T>
-where
-    T: FontWrite + Validate,
-{
-    fn from(value: Option<T>) -> Self {
-        BeValue(value)
-    }
-}
-
 pub struct BePersistentStorage {
     active: bool,
     pub(crate) paths: Paths,
@@ -734,31 +688,31 @@ pub struct Context {
     pub glyphs: BeContextMap<Glyph>,
 
     // Allow avar to be explicitly None to record a noop avar being generated
-    pub avar: BeContextItem<BeValue<Avar>>,
-    pub cmap: BeContextItem<BeValue<Cmap>>,
-    pub fvar: BeContextItem<BeValue<Fvar>>,
+    pub avar: BeContextItem<PossiblyEmptyAvar>,
+    pub cmap: BeContextItem<Cmap>,
+    pub fvar: BeContextItem<Fvar>,
     pub glyf: BeContextItem<Bytes>,
-    pub gsub: BeContextItem<BeValue<Gsub>>,
-    pub gpos: BeContextItem<BeValue<Gpos>>,
-    pub gdef: BeContextItem<BeValue<Gdef>>,
+    pub gsub: BeContextItem<Gsub>,
+    pub gpos: BeContextItem<Gpos>,
+    pub gdef: BeContextItem<Gdef>,
     pub gvar: BeContextItem<Bytes>,
-    pub post: BeContextItem<BeValue<Post>>,
+    pub post: BeContextItem<Post>,
     pub loca: BeContextItem<Bytes>,
     pub loca_format: BeContextItem<LocaFormatWrapper>,
-    pub maxp: BeContextItem<BeValue<Maxp>>,
-    pub name: BeContextItem<BeValue<Name>>,
-    pub os2: BeContextItem<BeValue<Os2>>,
-    pub head: BeContextItem<BeValue<Head>>,
-    pub hhea: BeContextItem<BeValue<Hhea>>,
+    pub maxp: BeContextItem<Maxp>,
+    pub name: BeContextItem<Name>,
+    pub os2: BeContextItem<Os2>,
+    pub head: BeContextItem<Head>,
+    pub hhea: BeContextItem<Hhea>,
     pub hmtx: BeContextItem<Bytes>,
-    pub hvar: BeContextItem<BeValue<Hvar>>,
-    pub mvar: BeContextItem<BeValue<Mvar>>,
+    pub hvar: BeContextItem<Hvar>,
+    pub mvar: BeContextItem<Mvar>,
     pub all_kerning_pairs: BeContextItem<AllKerningPairs>,
     pub kern_fragments: BeContextMap<KernFragment>,
     pub fea_ast: BeContextItem<FeaAst>,
     pub fea_rs_kerns: BeContextItem<FeaRsKerns>,
     pub fea_rs_marks: BeContextItem<FeaRsMarks>,
-    pub stat: BeContextItem<BeValue<Stat>>,
+    pub stat: BeContextItem<Stat>,
     pub font: BeContextItem<Bytes>,
 }
 
@@ -912,11 +866,11 @@ impl Persistable for Bytes {
     }
 }
 
-pub(crate) fn to_bytes<T>(table: &BeValue<T>) -> Option<Vec<u8>>
+pub(crate) fn to_bytes<T>(table: &T) -> Option<Vec<u8>>
 where
     T: FontWrite + Validate,
 {
-    table.0.as_ref().map(|t| dump_table(t).unwrap())
+    write_fonts::dump_table(table).ok()
 }
 
 #[cfg(test)]

fontbe/src/os2.rs

@@ -215,10 +215,7 @@ pub fn create_os2_work() -> Box<BeWork> {
 /// <https://github.com/fonttools/fonttools/blob/115275cbf429d91b75ac5536f5f0b2d6fe9d823a/Lib/fontTools/ttLib/tables/O_S_2f_2.py#L336-L348>
 fn x_avg_char_width(context: &Context) -> Result<i16, Error> {
     let glyph_order = context.ir.glyph_order.get();
-    let arc_hhea = context.hhea.get();
-    let Some(hhea) = &arc_hhea.as_ref().0 else {
-        return Err(Error::MissingTable(Tag::new(b"hhea")));
-    };
+    let hhea = context.hhea.get();
     let raw_hmtx = context.hmtx.get();
     let num_glyphs = glyph_order.len() as u64;
     let hmtx = Hmtx::read(
@@ -497,9 +494,9 @@ fn apply_min_max_char_index(os2: &mut Os2, codepoints: &HashSet<u32>) {
 /// * <https://learn.microsoft.com/en-us/typography/opentype/spec/gpos#gpos-header>
 fn apply_max_context(os2: &mut Os2, context: &Context) {
     let gsub = context.gsub.try_get();
-    let gsub = gsub.as_deref().and_then(|x| x.0.as_ref());
+    let gsub = gsub.as_deref();
     let gpos = context.gpos.try_get();
-    let gpos = gpos.as_deref().and_then(|x| x.0.as_ref());
+    let gpos = gpos.as_deref();
 
     os2.us_max_context = Some(max_context::compute_max_context_value(gpos, gsub));
 }
@@ -598,7 +595,7 @@ impl Work<Context, AnyWorkId, Error> for Os2Work {
 
         apply_max_context(&mut os2, context);
 
-        context.os2.set_unconditionally(os2.into());
+        context.os2.set(os2);
         Ok(())
     }
 }