feat: UFO round-trip preservation contract

crates/shift-backends/src/traits.rs

@@ -1,7 +1,7 @@
 use crate::errors::FormatBackendResult;
 use shift_font::{
-    Axis, FeatureData, Font, FontMetadata, FontMetrics, Glyph, GlyphName, Guideline, KerningData,
-    LibData, Source, SourceId,
+    Axis, BinaryData, FeatureData, Font, FontMetadata, FontMetrics, Glyph, GlyphName, Guideline,
+    KerningData, LibData, Source, SourceId,
 };
 
 pub trait FontView {
@@ -16,6 +16,9 @@ pub trait FontView {
     fn features(&self) -> &FeatureData;
     fn guidelines(&self) -> &[Guideline];
     fn lib(&self) -> &LibData;
+    fn fontinfo_remainder(&self) -> &LibData;
+    fn data_files(&self) -> &BinaryData;
+    fn images(&self) -> &BinaryData;
 }
 
 impl FontView for Font {
@@ -62,6 +65,18 @@ impl FontView for Font {
     fn lib(&self) -> &LibData {
         self.lib()
     }
+
+    fn fontinfo_remainder(&self) -> &LibData {
+        self.fontinfo_remainder()
+    }
+
+    fn data_files(&self) -> &BinaryData {
+        self.data_files()
+    }
+
+    fn images(&self) -> &BinaryData {
+        self.images()
+    }
 }
 
 pub trait FontReader: Send + Sync {

crates/shift-backends/src/ufo/reader.rs

@@ -11,6 +11,34 @@ use std::path::Path;
 
 pub struct UfoReader;
 
+/// The `fontinfo.plist` keys Shift models on [`shift_font::FontMetadata`],
+/// [`shift_font::FontMetrics`], and font guidelines. These keys are owned by
+/// the IR: they never enter the fontinfo remainder, and on save they are
+/// written from the IR fields.
+pub(crate) const MAPPED_FONTINFO_KEYS: &[&str] = &[
+    "familyName",
+    "styleName",
+    "versionMajor",
+    "versionMinor",
+    "copyright",
+    "trademark",
+    "openTypeNameDesigner",
+    "openTypeNameDesignerURL",
+    "openTypeNameManufacturer",
+    "openTypeNameManufacturerURL",
+    "openTypeNameLicense",
+    "openTypeNameLicenseURL",
+    "openTypeNameDescription",
+    "note",
+    "unitsPerEm",
+    "ascender",
+    "descender",
+    "capHeight",
+    "xHeight",
+    "italicAngle",
+    "guidelines",
+];
+
 struct PendingComponent {
     layer_id: LayerId,
     base_glyph_name: String,
@@ -72,17 +100,26 @@ impl UfoReader {
     }
 
     fn convert_guideline(guideline: &norad::Guideline) -> Guideline {
-        match guideline.line {
+        let mut converted = match guideline.line {
             Line::Horizontal(y) => Guideline::horizontal(y),
             Line::Vertical(x) => Guideline::vertical(x),
             Line::Angle { x, y, degrees } => Guideline::angled(x, y, degrees),
-        }
+        };
+        converted.set_name(guideline.name.as_ref().map(|name| name.to_string()));
+        converted.set_color(guideline.color.as_ref().map(|color| color.to_rgba_string()));
+        converted
     }
 
     fn convert_plist_to_lib_value(plist: &plist::Value) -> LibValue {
         match plist {
             plist::Value::String(s) => LibValue::String(s.clone()),
-            plist::Value::Integer(i) => LibValue::Integer(i.as_signed().unwrap_or(0)),
+            plist::Value::Integer(i) => match i.as_signed() {
+                Some(value) => LibValue::Integer(value),
+                None => LibValue::UnsignedInteger(
+                    i.as_unsigned()
+                        .expect("plist integer is signed or unsigned"),
+                ),
+            },
             plist::Value::Real(f) => LibValue::Float(*f),
             plist::Value::Boolean(b) => LibValue::Boolean(*b),
             plist::Value::Array(arr) => {
@@ -96,10 +133,45 @@ impl UfoReader {
                 LibValue::Dict(map)
             }
             plist::Value::Data(d) => LibValue::Data(d.clone()),
+            plist::Value::Date(d) => LibValue::Date(d.to_xml_format()),
+            plist::Value::Uid(u) => LibValue::Uid(u.get()),
             _ => LibValue::String(String::new()),
         }
     }
 
+    /// Captures every `fontinfo.plist` field Shift does not model as a
+    /// plist-shaped map, so unknown-to-Shift fields survive a round-trip.
+    fn convert_fontinfo_remainder(
+        font_info: &norad::FontInfo,
+    ) -> FormatBackendResult<Option<LibData>> {
+        let value = plist::to_value(font_info)
+            .map_err(|e| FormatBackendError::Ufo(format!("failed to capture fontinfo: {e}")))?;
+        let plist::Value::Dictionary(mut dict) = value else {
+            return Ok(None);
+        };
+
+        for key in MAPPED_FONTINFO_KEYS {
+            dict.remove(key);
+        }
+
+        // An empty style-map family is "unset" to the reference Python
+        // toolchain but poisons compilers that consume it verbatim (it
+        // becomes the exported family name), so it is not carried.
+        if dict
+            .get("styleMapFamilyName")
+            .and_then(plist::Value::as_string)
+            == Some("")
+        {
+            dict.remove("styleMapFamilyName");
+        }
+
+        if dict.is_empty() {
+            Ok(None)
+        } else {
+            Ok(Some(Self::convert_lib(&dict)))
+        }
+    }
+
     fn convert_lib(lib: &plist::Dictionary) -> LibData {
         let mut data = HashMap::new();
         for (k, v) in lib.iter() {
@@ -285,6 +357,10 @@ impl FontReader for UfoReader {
         font.metrics_mut().x_height = norad_font.font_info.x_height;
         font.metrics_mut().italic_angle = norad_font.font_info.italic_angle;
 
+        if let Some(remainder) = Self::convert_fontinfo_remainder(&norad_font.font_info)? {
+            *font.fontinfo_remainder_mut() = remainder;
+        }
+
         let norad_default_layer_name = norad_font.layers.default_layer().name().clone();
         let mut pending_components = Vec::new();
         for layer in norad_font.layers.iter() {
@@ -294,6 +370,13 @@ impl FontReader for UfoReader {
                 font.add_source(Source::new(layer.name().to_string(), Location::new()))
             };
 
+            if let Some(source) = font.source_mut(source_id.clone()) {
+                source.set_color(layer.color.as_ref().map(|color| color.to_rgba_string()));
+                if !layer.lib.is_empty() {
+                    *source.lib_mut() = Self::convert_lib(&layer.lib);
+                }
+            }
+
             for norad_glyph in layer.iter() {
                 let (glyph, glyph_components) =
                     Self::convert_glyph_layer(norad_glyph, LayerId::new(), source_id.clone());
@@ -321,6 +404,22 @@ impl FontReader for UfoReader {
             *font.lib_mut() = Self::convert_lib(&norad_font.lib);
         }
 
+        for (data_path, contents) in norad_font.data.iter() {
+            let bytes = contents.map_err(|e| {
+                FormatBackendError::Ufo(format!("failed to read data file {data_path:?}: {e}"))
+            })?;
+            font.data_files_mut()
+                .insert(data_path.to_string_lossy().into_owned(), bytes.to_vec());
+        }
+
+        for (image_path, contents) in norad_font.images.iter() {
+            let bytes = contents.map_err(|e| {
+                FormatBackendError::Ufo(format!("failed to read image file {image_path:?}: {e}"))
+            })?;
+            font.images_mut()
+                .insert(image_path.to_string_lossy().into_owned(), bytes.to_vec());
+        }
+
         Ok(font)
     }
 }

crates/shift-backends/src/ufo/writer.rs

@@ -4,7 +4,7 @@ use norad::{Font as NoradFont, Glyph as NoradGlyph, Line, Name};
 use shift_font::{
     Contour, Font, Glyph, GlyphLayer, Guideline, KerningSide, LibData, LibValue, Point, PointType,
 };
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 pub struct UfoWriter;
 
@@ -214,13 +214,19 @@ impl UfoWriter {
             .name()
             .map(|name| ufo_name("guideline", name))
             .transpose()?;
-        Ok(norad::Guideline::new(line, name, None, None))
+        // Colors that are not valid UFO color strings (possible for
+        // app-authored guidelines) cannot be written and are dropped.
+        let color = guideline
+            .color()
+            .and_then(|color| color.parse::<norad::Color>().ok());
+        Ok(norad::Guideline::new(line, name, color, None))
     }
 
     fn convert_lib_value_to_plist(value: &LibValue) -> plist::Value {
         match value {
             LibValue::String(s) => plist::Value::String(s.clone()),
             LibValue::Integer(i) => plist::Value::Integer((*i).into()),
+            LibValue::UnsignedInteger(u) => plist::Value::Integer((*u).into()),
             LibValue::Float(f) => plist::Value::Real(*f),
             LibValue::Boolean(b) => plist::Value::Boolean(*b),
             LibValue::Array(arr) => {
@@ -234,6 +240,10 @@ impl UfoWriter {
                 plist::Value::Dictionary(plist_dict)
             }
             LibValue::Data(d) => plist::Value::Data(d.clone()),
+            LibValue::Date(d) => plist::Date::from_xml_format(d)
+                .map(plist::Value::Date)
+                .unwrap_or_else(|_| plist::Value::String(d.clone())),
+            LibValue::Uid(u) => plist::Value::Uid(plist::Uid::new(*u)),
         }
     }
 
@@ -297,36 +307,75 @@ impl Default for UfoWriter {
 }
 
 impl UfoWriter {
+    /// Writes a source's carried `layerinfo.plist` metadata (color and layer
+    /// lib) onto the norad layer it maps to.
+    fn apply_layer_metadata(
+        source: &shift_font::Source,
+        layer: &mut norad::Layer,
+    ) -> FormatBackendResult<()> {
+        layer.color = source
+            .color()
+            .map(|color| {
+                color
+                    .parse::<norad::Color>()
+                    .map_err(|_| FormatBackendError::Ufo(format!("invalid layer color {color:?}")))
+            })
+            .transpose()?;
+
+        if !source.lib().is_empty() {
+            layer.lib = Self::convert_lib(source.lib());
+        }
+
+        Ok(())
+    }
+
     pub fn save_view(&self, font: &impl FontView, path: &str) -> FormatBackendResult<()> {
         let norad_font = Self::build_norad_font(font)?;
         Self::write_atomic(&norad_font, Path::new(path))
     }
 
+    /// Rebuilds `fontinfo.plist` from the carried remainder, then writes every
+    /// Shift-modeled field from the IR so the IR always wins for those keys.
+    fn build_font_info(font: &impl FontView) -> FormatBackendResult<norad::FontInfo> {
+        let remainder = font.fontinfo_remainder();
+        let mut font_info = if remainder.is_empty() {
+            norad::FontInfo::default()
+        } else {
+            let dict = Self::convert_lib(remainder);
+            plist::from_value(&plist::Value::Dictionary(dict)).map_err(|e| {
+                FormatBackendError::Ufo(format!("invalid preserved fontinfo data: {e}"))
+            })?
+        };
+
+        font_info.family_name = font.metadata().family_name.clone();
+        font_info.style_name = font.metadata().style_name.clone();
+        font_info.version_major = font.metadata().version_major;
+        font_info.version_minor = font.metadata().version_minor.map(|v| v as u32);
+        font_info.copyright = font.metadata().copyright.clone();
+        font_info.trademark = font.metadata().trademark.clone();
+        font_info.open_type_name_designer = font.metadata().designer.clone();
+        font_info.open_type_name_designer_url = font.metadata().designer_url.clone();
+        font_info.open_type_name_manufacturer = font.metadata().manufacturer.clone();
+        font_info.open_type_name_manufacturer_url = font.metadata().manufacturer_url.clone();
+        font_info.open_type_name_license = font.metadata().license.clone();
+        font_info.open_type_name_license_url = font.metadata().license_url.clone();
+        font_info.open_type_name_description = font.metadata().description.clone();
+        font_info.note = font.metadata().note.clone();
+
+        font_info.units_per_em = Some((font.metrics().units_per_em as u32).into());
+        font_info.ascender = Some(font.metrics().ascender);
+        font_info.descender = Some(font.metrics().descender);
+        font_info.cap_height = font.metrics().cap_height;
+        font_info.x_height = font.metrics().x_height;
+        font_info.italic_angle = font.metrics().italic_angle;
+        font_info.guidelines = None;
+
+        Ok(font_info)
+    }
+
     fn build_norad_font(font: &impl FontView) -> FormatBackendResult<NoradFont> {
         let mut norad_font = NoradFont::new();
-
-        norad_font.font_info.family_name = font.metadata().family_name.clone();
-        norad_font.font_info.style_name = font.metadata().style_name.clone();
-        norad_font.font_info.version_major = font.metadata().version_major;
-        norad_font.font_info.version_minor = font.metadata().version_minor.map(|v| v as u32);
-        norad_font.font_info.copyright = font.metadata().copyright.clone();
-        norad_font.font_info.trademark = font.metadata().trademark.clone();
-        norad_font.font_info.open_type_name_designer = font.metadata().designer.clone();
-        norad_font.font_info.open_type_name_designer_url = font.metadata().designer_url.clone();
-        norad_font.font_info.open_type_name_manufacturer = font.metadata().manufacturer.clone();
-        norad_font.font_info.open_type_name_manufacturer_url =
-            font.metadata().manufacturer_url.clone();
-        norad_font.font_info.open_type_name_license = font.metadata().license.clone();
-        norad_font.font_info.open_type_name_license_url = font.metadata().license_url.clone();
-        norad_font.font_info.open_type_name_description = font.metadata().description.clone();
-        norad_font.font_info.note = font.metadata().note.clone();
-
-        norad_font.font_info.units_per_em = Some((font.metrics().units_per_em as u32).into());
-        norad_font.font_info.ascender = Some(font.metrics().ascender);
-        norad_font.font_info.descender = Some(font.metrics().descender);
-        norad_font.font_info.cap_height = font.metrics().cap_height;
-        norad_font.font_info.x_height = font.metrics().x_height;
-        norad_font.font_info.italic_angle = font.metrics().italic_angle;
+        norad_font.font_info = Self::build_font_info(font)?;
 
         let groups = font
             .kerning()
@@ -383,6 +432,14 @@ impl UfoWriter {
             }
         }
 
+        if let Some(default_source) = font
+            .sources()
+            .iter()
+            .find(|source| Some(source.id()) == default_source_id)
+        {
+            Self::apply_layer_metadata(default_source, default_layer)?;
+        }
+
         for source in font.sources() {
             if Some(source.id()) == default_source_id {
                 continue;
@@ -392,6 +449,7 @@ impl UfoWriter {
                 .layers
                 .new_layer(source.name())
                 .map_err(|e| FormatBackendError::Ufo(e.to_string()))?;
+            Self::apply_layer_metadata(source, norad_layer)?;
 
             for glyph in font.glyphs() {
                 if let Some(layer_data) = glyph.layer_for_source(source.id()) {
@@ -405,6 +463,24 @@ impl UfoWriter {
             norad_font.features = fea_source.to_string();
         }
 
+        for (data_path, bytes) in font.data_files().iter() {
+            norad_font
+                .data
+                .insert(PathBuf::from(data_path), bytes.clone())
+                .map_err(|e| {
+                    FormatBackendError::Ufo(format!("invalid data file {data_path:?}: {e}"))
+                })?;
+        }
+
+        for (image_path, bytes) in font.images().iter() {
+            norad_font
+                .images
+                .insert(PathBuf::from(image_path), bytes.clone())
+                .map_err(|e| {
+                    FormatBackendError::Ufo(format!("invalid image file {image_path:?}: {e}"))
+                })?;
+        }
+
         Ok(norad_font)
     }