diff --git a/Cargo.lock b/Cargo.lock index d1223a0b..d391d1e1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1887,6 +1887,7 @@ dependencies = [ "norad 0.16.0", "plist", "quick-xml", + "serde", "shift-font", "shift-source", "skrifa", diff --git a/crates/shift-backends/Cargo.toml b/crates/shift-backends/Cargo.toml index 947d73a9..892bfb85 100644 --- a/crates/shift-backends/Cargo.toml +++ b/crates/shift-backends/Cargo.toml @@ -18,6 +18,7 @@ fontc = "0.2.0" glyphs-reader = "0.2.0" plist = "1" quick-xml = "0.37.5" +serde = "1" tempfile = "3" thiserror = "2.0.18" diff --git a/crates/shift-backends/src/atomic.rs b/crates/shift-backends/src/atomic.rs new file mode 100644 index 00000000..f0ea2e6b --- /dev/null +++ b/crates/shift-backends/src/atomic.rs @@ -0,0 +1,81 @@ +//! Crash-safe filesystem writes shared by the format backends. + +use std::io::Write; +use std::path::Path; + +/// Fsyncs the parent directory so a rename into it is durable. +#[cfg(unix)] +pub(crate) fn sync_parent(path: &Path) -> std::io::Result<()> { + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + std::fs::File::open(parent)?.sync_all()?; + } + } + Ok(()) +} + +// Windows cannot open directory handles this way; NTFS journals metadata itself. +#[cfg(not(unix))] +pub(crate) fn sync_parent(_path: &Path) -> std::io::Result<()> { + Ok(()) +} + +/// Writes `bytes` to `path` without ever exposing a partial file: the +/// content is staged in a temp file in the same directory, fsynced, renamed +/// over the target, and the parent directory is fsynced so the rename +/// itself is durable. If any step fails, the existing file is untouched. +pub(crate) fn write_file_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { + let parent = match path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent, + _ => Path::new("."), + }; + + let mut staged = tempfile::Builder::new() + .prefix(".shift-staged-") + .tempfile_in(parent)?; + staged.write_all(bytes)?; + staged.as_file().sync_all()?; + staged.persist(path).map_err(|error| error.error)?; + + sync_parent(path) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn replaces_existing_file_content() { + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("file.xml"); + + write_file_atomic(&target, b"first").unwrap(); + write_file_atomic(&target, b"second").unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"second"); + let entries: Vec<_> = std::fs::read_dir(temp.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec!["file.xml"], "no staging leftovers"); + } + + #[cfg(unix)] + #[test] + fn failed_write_leaves_existing_file_intact() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("file.xml"); + std::fs::write(&target, b"original").unwrap(); + + // A read-only directory makes staging the temp file fail before the + // target is ever touched. + std::fs::set_permissions(temp.path(), std::fs::Permissions::from_mode(0o555)).unwrap(); + let result = write_file_atomic(&target, b"replacement"); + std::fs::set_permissions(temp.path(), std::fs::Permissions::from_mode(0o755)).unwrap(); + + result.expect_err("write into a read-only directory should fail"); + assert_eq!(std::fs::read(&target).unwrap(), b"original"); + } +} diff --git a/crates/shift-backends/src/designspace/reader.rs b/crates/shift-backends/src/designspace/reader.rs index e0e31969..967e0d69 100644 --- a/crates/shift-backends/src/designspace/reader.rs +++ b/crates/shift-backends/src/designspace/reader.rs @@ -158,11 +158,9 @@ impl DesignspaceReader { let name = source_name(ds_source, idx); let location = location_from_dimensions(&ds_source.location, &doc, font.axes()); - let source_id = font.add_source(Source::with_filename( - name, - location, - ds_source.filename.clone(), - )); + let mut source = Source::with_filename(name, location, ds_source.filename.clone()); + source.set_layer_name(ds_source.layer.clone()); + let source_id = font.add_source(source); // Copy glyphs from the resolved layer into the new layer. for source_glyph in source_font.glyphs() { @@ -289,11 +287,9 @@ fn load_axisless_designspace( }; let name = axisless_source_name(ds_source, idx); - let source_id = font.add_source(Source::with_filename( - name, - Location::new(), - ds_source.filename.clone(), - )); + let mut source = Source::with_filename(name, Location::new(), ds_source.filename.clone()); + source.set_layer_name(ds_source.layer.clone()); + let source_id = font.add_source(source); for source_glyph in source_font.glyphs() { if let Some(source_layer) = source_glyph.layer_for_source(source_source_id.clone()) { diff --git a/crates/shift-backends/src/designspace/writer.rs b/crates/shift-backends/src/designspace/writer.rs index 1e24fef7..8cb8fe7d 100644 --- a/crates/shift-backends/src/designspace/writer.rs +++ b/crates/shift-backends/src/designspace/writer.rs @@ -1,30 +1,236 @@ use super::error::{DesignspaceError, DesignspaceResult}; +use crate::atomic::write_file_atomic; use crate::errors::{FormatBackendError, FormatBackendResult}; -use crate::traits::FontWriter; +use crate::traits::{FontView, FontWriter}; use crate::ufo::UfoWriter; use norad::designspace::{Axis as DsAxis, DesignSpaceDocument, Dimension, Source as DsSource}; use quick_xml::events::{BytesDecl, BytesEnd, BytesStart, Event}; use quick_xml::Writer; -use shift_font::{Axis, Font, Location, Source}; +use serde::Serialize; +use shift_font::{ + Axis, BinaryData, FeatureData, Font, FontMetadata, FontMetrics, Glyph, Guideline, KerningData, + LibData, Location, Source, SourceId, +}; +use std::collections::HashSet; use std::fs; use std::path::Path; pub struct DesignspaceWriter; +/// One UFO file of the designspace project: the sources whose data lives in +/// that file and, if any, the source that owns the file's default layer. +struct UfoFileGroup { + filename: String, + sources: Vec, + file_default: Option, +} + +/// A [`FontView`] scoped to one UFO file of a multi-UFO designspace: only +/// the sources assigned to that file are visible, and the file's own +/// default-layer source stands in as the view's default source. +struct UfoFileView<'a> { + font: &'a Font, + metadata: FontMetadata, + sources: &'a [Source], + default_source_id: Option, +} + +impl<'a> UfoFileView<'a> { + fn new(font: &'a Font, group: &'a UfoFileGroup) -> Self { + let mut metadata = font.metadata().clone(); + + // Companion UFOs conventionally carry their own master's style name. + if group.file_default != font.default_source_id() { + if let Some(source) = group + .sources + .iter() + .find(|source| Some(source.id()) == group.file_default) + { + metadata.style_name = Some(source.name().to_string()); + } + } + + Self { + font, + metadata, + sources: &group.sources, + default_source_id: group.file_default.clone(), + } + } +} + +impl FontView for UfoFileView<'_> { + fn metadata(&self) -> &FontMetadata { + &self.metadata + } + + fn metrics(&self) -> &FontMetrics { + self.font.metrics() + } + + fn axes(&self) -> &[Axis] { + self.font.axes() + } + + fn sources(&self) -> &[Source] { + self.sources + } + + fn default_source_id(&self) -> Option { + self.default_source_id.clone() + } + + fn glyphs(&self) -> Vec<&Glyph> { + self.font.glyphs().collect() + } + + fn glyph(&self, name: &str) -> Option<&Glyph> { + self.font.glyph_by_name(name) + } + + fn kerning(&self) -> &KerningData { + self.font.kerning() + } + + fn features(&self) -> &FeatureData { + self.font.features() + } + + fn guidelines(&self) -> &[Guideline] { + self.font.guidelines() + } + + fn lib(&self) -> &LibData { + self.font.lib() + } + + fn fontinfo_remainder(&self) -> &LibData { + self.font.fontinfo_remainder() + } + + fn data_files(&self) -> &BinaryData { + self.font.data_files() + } + + fn images(&self) -> &BinaryData { + self.font.images() + } +} + impl DesignspaceWriter { pub fn new() -> Self { Self } - fn companion_ufo_filename(path: &Path) -> DesignspaceResult { - let stem = path - .file_stem() + fn designspace_stem(path: &Path) -> DesignspaceResult<&str> { + path.file_stem() .and_then(|name| name.to_str()) .ok_or_else(|| DesignspaceError::InvalidDesignspacePath { path: path.to_path_buf(), - })?; + }) + } + + /// Assigns every source to the UFO file it will be written into. + /// + /// Sources keep the filename they were read from. Layer-only sources + /// without a filename ride along in the default source's file. Masters + /// without a filename (created inside Shift) get a deterministic, + /// collision-safe filename derived from the designspace stem and their + /// style name. + fn group_sources(font: &Font, stem: &str) -> Vec { + let font_default = font.default_source_id(); + let default_filename = font + .default_source() + .and_then(|source| source.filename()) + .map(str::to_string) + .unwrap_or_else(|| format!("{stem}.ufo")); + + let mut used: HashSet = font + .sources() + .iter() + .filter_map(|source| source.filename().map(str::to_string)) + .collect(); + used.insert(default_filename.clone()); + + let mut groups = vec![UfoFileGroup { + filename: default_filename.clone(), + sources: Vec::new(), + file_default: None, + }]; + + for source in font.sources() { + let filename = match source.filename() { + Some(filename) => filename.to_string(), + None if Some(source.id()) == font_default || !source.is_master() => { + default_filename.clone() + } + None => Self::generated_filename(stem, source.name(), &mut used), + }; + + match groups.iter_mut().find(|group| group.filename == filename) { + Some(group) => group.sources.push(source.clone()), + None => groups.push(UfoFileGroup { + filename, + sources: vec![source.clone()], + file_default: None, + }), + } + } + + for group in &mut groups { + group.file_default = Self::file_default(&group.sources, &font_default); + } + + groups + } + + fn generated_filename(stem: &str, source_name: &str, used: &mut HashSet) -> String { + let sanitized: String = source_name + .chars() + .map(|c| if matches!(c, '/' | '\\') { '-' } else { c }) + .collect(); + let sanitized = sanitized.trim(); + let base = if sanitized.is_empty() { + format!("{stem}-Master") + } else { + format!("{stem}-{sanitized}") + }; + + let mut candidate = format!("{base}.ufo"); + let mut counter = 2; + while !used.insert(candidate.clone()) { + candidate = format!("{base}-{counter}.ufo"); + counter += 1; + } + + candidate + } + + /// The source that owns the file's default layer: the font default if it + /// lives in this file, otherwise the file's master without a layer + /// binding. `None` means the file has only named layers and its default + /// layer is written empty. + fn file_default(sources: &[Source], font_default: &Option) -> Option { + if let Some(id) = font_default { + if sources.iter().any(|source| source.id() == *id) { + return Some(id.clone()); + } + } - Ok(format!("{stem}.ufo")) + sources + .iter() + .find(|source| source.is_master() && source.layer_name().is_none()) + .map(Source::id) + } + + /// The `layer` attribute for a master's `` entry: absent for the + /// file's default layer, otherwise the UFO layer holding its data. + fn ds_layer_attr<'a>(source: &'a Source, file_default: &Option) -> Option<&'a str> { + if Some(source.id()) == *file_default { + None + } else { + Some(source.layer_name().unwrap_or_else(|| source.name())) + } } fn axis(axis: &Axis) -> DsAxis { @@ -49,36 +255,29 @@ impl DesignspaceWriter { .collect() } - fn source(source: &Source, font: &Font, filename: &str, axes: &[Axis]) -> DsSource { - let layer = if Some(source.id()) == font.default_source_id() { - None - } else { - Some(source.name().to_string()) - }; - + fn source( + source: &Source, + font: &Font, + filename: &str, + layer: Option<&str>, + axes: &[Axis], + ) -> DsSource { DsSource { familyname: font.metadata().family_name.clone(), stylename: Some(source.name().to_string()), name: Some(source.name().to_string()), filename: filename.to_string(), - layer, + layer: layer.map(str::to_string), location: Self::location(source.location(), axes), } } - fn source_layer(source: &Source, font: &Font) -> Option { - if Some(source.id()) == font.default_source_id() { - None - } else { - Some(source.name().to_string()) - } - } - fn write_axisless_source( writer: &mut Writer>, font: &Font, filename: &str, - source: Option<&Source>, + stylename: &str, + layer: Option<&str>, ) -> DesignspaceResult<()> { let mut event = BytesStart::new("source"); event.push_attribute(("filename", filename)); @@ -87,16 +286,10 @@ impl DesignspaceWriter { event.push_attribute(("familyname", familyname)); } - let stylename = source - .map(|source| source.name().to_string()) - .or_else(|| Some("Regular".to_string())); - if let Some(stylename) = stylename.as_deref() { - event.push_attribute(("stylename", stylename)); - event.push_attribute(("name", stylename)); - } + event.push_attribute(("stylename", stylename)); + event.push_attribute(("name", stylename)); - let layer = source.and_then(|source| Self::source_layer(source, font)); - if let Some(layer) = layer.as_deref() { + if let Some(layer) = layer { event.push_attribute(("layer", layer)); } @@ -110,7 +303,7 @@ impl DesignspaceWriter { fn save_axisless_designspace( font: &Font, path: &Path, - ufo_filename: &str, + groups: &[UfoFileGroup], ) -> DesignspaceResult<()> { let mut writer = Writer::new_with_indent(Vec::new(), b' ', 2); writer @@ -132,15 +325,24 @@ impl DesignspaceWriter { details: error.to_string(), })?; - let sources = font.sources(); - if sources.is_empty() { - Self::write_axisless_source(&mut writer, font, ufo_filename, None)?; - } else { - for source in sources { - Self::write_axisless_source(&mut writer, font, ufo_filename, Some(source))?; + let mut wrote_source = false; + for group in groups { + for source in group.sources.iter().filter(|source| source.is_master()) { + Self::write_axisless_source( + &mut writer, + font, + &group.filename, + source.name(), + Self::ds_layer_attr(source, &group.file_default), + )?; + wrote_source = true; } } + if !wrote_source { + Self::write_axisless_source(&mut writer, font, &groups[0].filename, "Regular", None)?; + } + writer .write_event(Event::End(BytesEnd::new("sources"))) .map_err(|error| DesignspaceError::ParseAxislessXml { @@ -152,7 +354,30 @@ impl DesignspaceWriter { details: error.to_string(), })?; - fs::write(path, writer.into_inner()).map_err(|source| DesignspaceError::WriteFile { + write_file_atomic(path, &writer.into_inner()).map_err(|source| { + DesignspaceError::WriteFile { + path: path.to_path_buf(), + source, + } + }) + } + + /// Serializes exactly like norad's `DesignSpaceDocument::save`, but + /// routed through a temp-file + fsync + rename so a failed save never + /// truncates the existing designspace. + fn save_document_atomic(document: &DesignSpaceDocument, path: &Path) -> DesignspaceResult<()> { + let mut xml = String::from("\n"); + let mut serializer = quick_xml::se::Serializer::new(&mut xml); + serializer.indent(' ', 2); + document + .serialize(serializer) + .map_err(|error| DesignspaceError::SaveDesignspace { + path: path.to_path_buf(), + details: error.to_string(), + })?; + xml.push('\n'); + + write_file_atomic(path, xml.as_bytes()).map_err(|source| DesignspaceError::WriteFile { path: path.to_path_buf(), source, }) @@ -176,48 +401,64 @@ impl DesignspaceWriter { source, })?; - let ufo_filename = Self::companion_ufo_filename(path)?; - let ufo_path = parent.join(&ufo_filename); - UfoWriter::new() - .save(font, Self::path_to_str(&ufo_path)?) - .map_err(|source| DesignspaceError::SaveUfo { - path: ufo_path.clone(), - details: source.to_string(), - })?; + let stem = Self::designspace_stem(path)?; + let groups = Self::group_sources(font, stem); + + // Data before pointer: every UFO is written (each atomically) before + // the designspace XML that references them. If the save fails + // partway, the old XML is untouched and still names the same UFO + // files, each of which is either its previous revision or a + // completed new one — the project on disk never points at + // half-written data. UFOs newly created by an aborted save are + // unreferenced and harmless. + for group in &groups { + let ufo_path = parent.join(&group.filename); + let view = UfoFileView::new(font, group); + UfoWriter::new() + .save_view(&view, Self::path_to_str(&ufo_path)?) + .map_err(|source| DesignspaceError::SaveUfo { + path: ufo_path.clone(), + details: source.to_string(), + })?; + } let axes = font.axes(); if axes.is_empty() { - return Self::save_axisless_designspace(font, path, &ufo_filename); + return Self::save_axisless_designspace(font, path, &groups); } - let mut document = DesignSpaceDocument { - format: 5.0, - axes: axes.iter().map(Self::axis).collect(), - sources: font - .sources() - .iter() - .map(|source| Self::source(source, font, &ufo_filename, axes)) - .collect(), - ..Default::default() - }; + let mut sources = Vec::new(); + for group in &groups { + for source in group.sources.iter().filter(|source| source.is_master()) { + sources.push(Self::source( + source, + font, + &group.filename, + Self::ds_layer_attr(source, &group.file_default), + axes, + )); + } + } - if document.sources.is_empty() { - document.sources.push(DsSource { + if sources.is_empty() { + sources.push(DsSource { familyname: font.metadata().family_name.clone(), stylename: Some("Regular".to_string()), name: Some("Regular".to_string()), - filename: ufo_filename, + filename: groups[0].filename.clone(), location: Self::location(&Location::new(), axes), ..Default::default() }); } - document - .save(path) - .map_err(|source| DesignspaceError::SaveDesignspace { - path: path.to_path_buf(), - details: source.to_string(), - }) + let document = DesignSpaceDocument { + format: 5.0, + axes: axes.iter().map(Self::axis).collect(), + sources, + ..Default::default() + }; + + Self::save_document_atomic(&document, path) } } diff --git a/crates/shift-backends/src/lib.rs b/crates/shift-backends/src/lib.rs index ba5f7a54..922ca1bc 100644 --- a/crates/shift-backends/src/lib.rs +++ b/crates/shift-backends/src/lib.rs @@ -1,3 +1,4 @@ +mod atomic; pub mod binary; pub mod designspace; pub mod errors; diff --git a/crates/shift-backends/src/ufo/reader.rs b/crates/shift-backends/src/ufo/reader.rs index af34474c..e405ee80 100644 --- a/crates/shift-backends/src/ufo/reader.rs +++ b/crates/shift-backends/src/ufo/reader.rs @@ -3,8 +3,7 @@ use crate::traits::FontReader; use norad::{Font as NoradFont, Line}; use shift_font::{ Anchor, Component, Contour, FeatureData, Font, Glyph, GlyphLayer, Guideline, KerningData, - KerningPair, KerningSide, LayerId, LibData, LibValue, Location, PointType, Source, SourceId, - Transform, + KerningPair, KerningSide, LayerId, LibData, LibValue, PointType, Source, SourceId, Transform, }; use std::collections::HashMap; use std::path::Path; @@ -367,7 +366,10 @@ impl FontReader for UfoReader { let source_id = if layer.name() == &norad_default_layer_name { default_source_id.clone() } else { - font.add_source(Source::new(layer.name().to_string(), Location::new())) + // Non-default UFO layers are carried as layer-only sources: + // they are not designspace masters and must not gain + // `` entries if the font is saved as a designspace. + font.add_source(Source::layer(layer.name().to_string())) }; if let Some(source) = font.source_mut(source_id.clone()) { diff --git a/crates/shift-backends/src/ufo/writer.rs b/crates/shift-backends/src/ufo/writer.rs index 17af8658..3114e79a 100644 --- a/crates/shift-backends/src/ufo/writer.rs +++ b/crates/shift-backends/src/ufo/writer.rs @@ -1,3 +1,4 @@ +use crate::atomic::sync_parent; use crate::errors::{FormatBackendError, FormatBackendResult}; use crate::traits::{FontView, FontWriter}; use norad::{Font as NoradFont, Glyph as NoradGlyph, Line, Name}; @@ -19,22 +20,6 @@ fn io_error(action: &str, path: &Path, error: std::io::Error) -> FormatBackendEr FormatBackendError::Ufo(format!("failed to {action} '{}': {error}", path.display())) } -#[cfg(unix)] -fn sync_parent(path: &Path) -> std::io::Result<()> { - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::File::open(parent)?.sync_all()?; - } - } - Ok(()) -} - -// Windows cannot open directory handles this way; NTFS journals metadata itself. -#[cfg(not(unix))] -fn sync_parent(_path: &Path) -> std::io::Result<()> { - Ok(()) -} - /// Fsyncs every file under `root`, then each directory bottom-up, so the /// staged UFO is durable before it is renamed into place. #[cfg(unix)] @@ -445,9 +430,12 @@ impl UfoWriter { continue; } + // A designspace-declared layer binding wins over the source name + // so `` data returns to the layer it came from. + let layer_name = source.layer_name().unwrap_or_else(|| source.name()); let norad_layer = norad_font .layers - .new_layer(source.name()) + .new_layer(layer_name) .map_err(|e| FormatBackendError::Ufo(e.to_string()))?; Self::apply_layer_metadata(source, norad_layer)?; diff --git a/crates/shift-backends/tests/round_trip.rs b/crates/shift-backends/tests/round_trip.rs index f0ad9f67..db3a39a1 100644 --- a/crates/shift-backends/tests/round_trip.rs +++ b/crates/shift-backends/tests/round_trip.rs @@ -1,3 +1,4 @@ mod round_trip { + mod designspace; mod ufo; } diff --git a/crates/shift-backends/tests/round_trip/designspace.rs b/crates/shift-backends/tests/round_trip/designspace.rs new file mode 100644 index 00000000..7b5a45c2 --- /dev/null +++ b/crates/shift-backends/tests/round_trip/designspace.rs @@ -0,0 +1,358 @@ +use std::path::{Path, PathBuf}; + +use norad::designspace::{Axis as DsAxis, DesignSpaceDocument, Dimension, Source as DsSource}; +use shift_backends::font_loader::FontLoader; +use shift_font::Font; + +fn load_font(path: &Path) -> Font { + FontLoader::new() + .read_font(path.to_str().unwrap()) + .unwrap_or_else(|error| panic!("failed to load {}: {error}", path.display())) +} + +fn save_font(font: &Font, path: &Path) { + FontLoader::new() + .write_font(font, path.to_str().unwrap()) + .unwrap_or_else(|error| panic!("failed to save {}: {error}", path.display())); +} + +fn triangle_glyph(name: &str, width: f64, peak_y: f64) -> norad::Glyph { + let mut glyph = norad::Glyph::new(name); + glyph.width = width; + glyph.codepoints.insert('A'); + glyph.contours.push(norad::Contour::new( + vec![ + norad::ContourPoint::new(0.0, 0.0, norad::PointType::Line, false, None, None), + norad::ContourPoint::new( + width / 2.0, + peak_y, + norad::PointType::Line, + false, + None, + None, + ), + norad::ContourPoint::new(width, 0.0, norad::PointType::Line, false, None, None), + ], + None, + )); + glyph +} + +fn master_ufo(dir: &Path, filename: &str, style: &str, width: f64, peak_y: f64) -> PathBuf { + let mut font = norad::Font::new(); + font.font_info.family_name = Some("Grouped".to_string()); + font.font_info.style_name = Some(style.to_string()); + font.font_info.units_per_em = Some(1000_u32.into()); + font.font_info.ascender = Some(800.0); + font.font_info.descender = Some(-200.0); + font.layers + .default_layer_mut() + .insert_glyph(triangle_glyph("A", width, peak_y)); + + let path = dir.join(filename); + font.save(&path).expect("fixture UFO should save"); + path +} + +fn weight_dimension(value: f32) -> Dimension { + Dimension { + name: "Weight".to_string(), + xvalue: Some(value), + ..Default::default() + } +} + +fn ds_source(style: &str, filename: &str, layer: Option<&str>, weight: f32) -> DsSource { + DsSource { + familyname: Some("Grouped".to_string()), + stylename: Some(style.to_string()), + name: Some(style.to_string()), + filename: filename.to_string(), + layer: layer.map(str::to_string), + location: vec![weight_dimension(weight)], + } +} + +/// A designspace spread over two master UFOs, with an intermediate master +/// declared as a layer of the bold UFO. +fn multi_ufo_designspace(dir: &Path) -> PathBuf { + master_ufo(dir, "Grouped-Light.ufo", "Light", 400.0, 600.0); + let bold_path = master_ufo(dir, "Grouped-Bold.ufo", "Bold", 700.0, 750.0); + + let mut bold = norad::Font::load(&bold_path).unwrap(); + let medium = bold.layers.new_layer("Medium").unwrap(); + medium.insert_glyph(triangle_glyph("A", 550.0, 675.0)); + bold.save(&bold_path).unwrap(); + + let document = DesignSpaceDocument { + format: 5.0, + axes: vec![DsAxis { + name: "Weight".to_string(), + tag: "wght".to_string(), + minimum: Some(300.0), + default: 300.0, + maximum: Some(700.0), + ..Default::default() + }], + sources: vec![ + ds_source("Light", "Grouped-Light.ufo", None, 300.0), + ds_source("Bold", "Grouped-Bold.ufo", None, 700.0), + ds_source("Medium", "Grouped-Bold.ufo", Some("Medium"), 500.0), + ], + ..Default::default() + }; + + let path = dir.join("Grouped.designspace"); + document + .save(&path) + .expect("fixture designspace should save"); + path +} + +/// (name, filename, layer name, weight location) per source. +type SourceShape = (String, Option, Option, Option); + +fn source_shape(font: &Font) -> Vec { + let weight = font.axis_id_by_tag("wght").expect("wght axis should exist"); + font.sources() + .iter() + .map(|source| { + ( + source.name().to_string(), + source.filename().map(str::to_string), + source.layer_name().map(str::to_string), + source.location().get(&weight), + ) + }) + .collect() +} + +fn layer_width(font: &Font, source_name: &str, glyph_name: &str) -> f64 { + let source = font + .sources() + .iter() + .find(|source| source.name() == source_name) + .unwrap_or_else(|| panic!("source {source_name} should exist")); + font.glyph_by_name(glyph_name) + .unwrap_or_else(|| panic!("glyph {glyph_name} should exist")) + .layer_for_source(source.id()) + .unwrap_or_else(|| panic!("{glyph_name} should have a layer for {source_name}")) + .width() +} + +#[test] +fn multi_ufo_designspace_round_trips_file_structure_and_sources() { + let fixture_dir = tempfile::tempdir().unwrap(); + let ds_path = multi_ufo_designspace(fixture_dir.path()); + let original = load_font(&ds_path); + + // Save under a different stem to prove filenames come from the stored + // source filenames, not from the designspace path. + let out_dir = tempfile::tempdir().unwrap(); + let out_path = out_dir.path().join("Renamed.designspace"); + save_font(&original, &out_path); + + let mut entries: Vec<_> = std::fs::read_dir(out_dir.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + entries.sort(); + assert_eq!( + entries, + vec![ + "Grouped-Bold.ufo", + "Grouped-Light.ufo", + "Renamed.designspace" + ], + "the project must keep its original UFO files, not be restructured" + ); + + let saved = DesignSpaceDocument::load(&out_path).unwrap(); + let saved_shape: Vec<_> = saved + .sources + .iter() + .map(|source| { + ( + source.stylename.clone().unwrap(), + source.filename.clone(), + source.layer.clone(), + source.location[0].xvalue.unwrap(), + ) + }) + .collect(); + assert_eq!( + saved_shape, + vec![ + ( + "Light".to_string(), + "Grouped-Light.ufo".to_string(), + None, + 300.0 + ), + ( + "Bold".to_string(), + "Grouped-Bold.ufo".to_string(), + None, + 700.0 + ), + ( + "Medium".to_string(), + "Grouped-Bold.ufo".to_string(), + Some("Medium".to_string()), + 500.0 + ), + ] + ); + + let saved_bold = norad::Font::load(out_dir.path().join("Grouped-Bold.ufo")).unwrap(); + assert_eq!( + saved_bold + .layers + .default_layer() + .get_glyph("A") + .expect("Bold default layer should keep A") + .width, + 700.0 + ); + assert_eq!( + saved_bold + .layers + .get("Medium") + .expect("Medium must stay a layer of the Bold UFO") + .get_glyph("A") + .expect("Medium layer should keep A") + .width, + 550.0 + ); + + let reloaded = load_font(&out_path); + assert_eq!(source_shape(&reloaded), source_shape(&original)); + + let axes: Vec<_> = reloaded + .axes() + .iter() + .map(|axis| (axis.tag(), axis.minimum(), axis.default(), axis.maximum())) + .collect(); + let original_axes: Vec<_> = original + .axes() + .iter() + .map(|axis| (axis.tag(), axis.minimum(), axis.default(), axis.maximum())) + .collect(); + assert_eq!(axes, original_axes); + + for source_name in ["Light", "Bold", "Medium"] { + assert_eq!( + layer_width(&reloaded, source_name, "A"), + layer_width(&original, source_name, "A"), + "glyph geometry for {source_name} should survive the round trip" + ); + } +} + +#[cfg(unix)] +#[test] +fn failed_save_leaves_existing_designspace_xml_intact() { + use std::os::unix::fs::PermissionsExt; + + let fixture_dir = tempfile::tempdir().unwrap(); + let ds_path = multi_ufo_designspace(fixture_dir.path()); + let font = load_font(&ds_path); + + let out_dir = tempfile::tempdir().unwrap(); + let out_path = out_dir.path().join("Grouped.designspace"); + save_font(&font, &out_path); + let saved_xml = std::fs::read(&out_path).unwrap(); + + // A read-only project directory makes every staged write fail before + // any existing file is touched. + std::fs::set_permissions(out_dir.path(), std::fs::Permissions::from_mode(0o555)).unwrap(); + let result = FontLoader::new().write_font(&font, out_path.to_str().unwrap()); + std::fs::set_permissions(out_dir.path(), std::fs::Permissions::from_mode(0o755)).unwrap(); + + result.expect_err("saving into a read-only directory should fail"); + assert_eq!( + std::fs::read(&out_path).unwrap(), + saved_xml, + "a failed save must leave the existing designspace XML intact" + ); +} + +fn background_layer_ufo(dir: &Path) -> PathBuf { + let mut font = norad::Font::new(); + font.font_info.family_name = Some("Layered".to_string()); + font.font_info.style_name = Some("Regular".to_string()); + font.font_info.units_per_em = Some(1000_u32.into()); + font.layers + .default_layer_mut() + .insert_glyph(triangle_glyph("A", 500.0, 700.0)); + + let background = font.layers.new_layer("background").unwrap(); + background.insert_glyph(triangle_glyph("A", 500.0, 650.0)); + + let path = dir.join("Layered.ufo"); + font.save(&path).expect("fixture UFO should save"); + path +} + +#[test] +fn plain_ufo_layers_are_not_promoted_to_designspace_masters() { + let fixture_dir = tempfile::tempdir().unwrap(); + let ufo_path = background_layer_ufo(fixture_dir.path()); + let font = load_font(&ufo_path); + + let out_dir = tempfile::tempdir().unwrap(); + let out_path = out_dir.path().join("Layered.designspace"); + save_font(&font, &out_path); + + let xml = std::fs::read_to_string(&out_path).unwrap(); + assert_eq!( + xml.matches(" entry: {xml}" + ); + assert!( + !xml.contains("background"), + "the background layer must not become a designspace source: {xml}" + ); + + let saved_ufo = norad::Font::load(out_dir.path().join("Layered.ufo")).unwrap(); + assert_eq!( + saved_ufo + .layers + .get("background") + .expect("background must survive as a UFO layer") + .get_glyph("A") + .expect("background A should survive") + .width, + 500.0 + ); +} + +#[test] +fn plain_ufo_layers_stay_layers_when_axes_exist() { + let fixture_dir = tempfile::tempdir().unwrap(); + let ufo_path = background_layer_ufo(fixture_dir.path()); + let mut font = load_font(&ufo_path); + font.add_axis(shift_font::Axis::new( + "wght".to_string(), + "Weight".to_string(), + 300.0, + 400.0, + 700.0, + )); + + let out_dir = tempfile::tempdir().unwrap(); + let out_path = out_dir.path().join("Layered.designspace"); + save_font(&font, &out_path); + + let saved = DesignSpaceDocument::load(&out_path).unwrap(); + assert_eq!(saved.sources.len(), 1); + assert_eq!(saved.sources[0].stylename.as_deref(), Some("Regular")); + assert!(saved.axes.iter().any(|axis| axis.tag == "wght")); + + let saved_ufo = norad::Font::load(out_dir.path().join("Layered.ufo")).unwrap(); + assert!( + saved_ufo.layers.get("background").is_some(), + "background must stay a UFO layer" + ); +} diff --git a/crates/shift-font/src/ir/mod.rs b/crates/shift-font/src/ir/mod.rs index cc66be80..f6968ffd 100644 --- a/crates/shift-font/src/ir/mod.rs +++ b/crates/shift-font/src/ir/mod.rs @@ -38,4 +38,4 @@ pub use lib_data::{LibData, LibValue}; pub use metrics::FontMetrics; pub use point::{Point, PointType}; pub use segment::{CurveSegment, CurveSegmentIter}; -pub use source::Source; +pub use source::{Source, SourceRole}; diff --git a/crates/shift-font/src/ir/source.rs b/crates/shift-font/src/ir/source.rs index 3d0df4ae..8da09a40 100644 --- a/crates/shift-font/src/ir/source.rs +++ b/crates/shift-font/src/ir/source.rs @@ -3,6 +3,20 @@ use crate::entity::SourceId; use crate::lib_data::LibData; use serde::{Deserialize, Serialize}; +/// How a source participates in the font. A `Master` is a genuine +/// designspace source: it has a design-space location and earns a +/// `` entry when the font is written as a designspace. A `Layer` +/// only carries a format layer that rides along with its file's masters +/// (e.g. a plain UFO background layer) and must never gain a designspace +/// `` entry on save. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum SourceRole { + #[default] + Master, + Layer, +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct Source { @@ -14,6 +28,10 @@ pub struct Source { color: Option, #[serde(default)] lib: LibData, + #[serde(default)] + role: SourceRole, + #[serde(default)] + layer_name: Option, } impl Source { @@ -25,6 +43,8 @@ impl Source { filename: None, color: None, lib: LibData::new(), + role: SourceRole::Master, + layer_name: None, } } @@ -36,6 +56,8 @@ impl Source { filename: Some(filename), color: None, lib: LibData::new(), + role: SourceRole::Master, + layer_name: None, } } @@ -52,6 +74,23 @@ impl Source { filename, color: None, lib: LibData::new(), + role: SourceRole::Master, + layer_name: None, + } + } + + /// A layer-only source: it carries a format layer named `name` (e.g. a + /// UFO background layer) but is not a designspace master. + pub fn layer(name: String) -> Self { + Self { + id: SourceId::new(), + name, + location: Location::new(), + filename: None, + color: None, + lib: LibData::new(), + role: SourceRole::Layer, + layer_name: None, } } @@ -71,6 +110,30 @@ impl Source { self.filename.as_deref() } + pub fn role(&self) -> SourceRole { + self.role + } + + pub fn is_master(&self) -> bool { + self.role == SourceRole::Master + } + + pub fn set_role(&mut self, role: SourceRole) { + self.role = role; + } + + /// The UFO layer inside [`Self::filename`] that holds this source's + /// data, when a designspace `` declared it explicitly with a + /// `layer` attribute. `None` means the file's default layer for + /// masters, or a layer named after the source for layer-only sources. + pub fn layer_name(&self) -> Option<&str> { + self.layer_name.as_deref() + } + + pub fn set_layer_name(&mut self, layer_name: Option) { + self.layer_name = layer_name; + } + /// The source's display color from the format's layer metadata /// (e.g. UFO `layerinfo.plist`), as an `r,g,b,a` string. pub fn color(&self) -> Option<&str> { diff --git a/crates/shift-source/src/package.rs b/crates/shift-source/src/package.rs index b0ad15a7..ce57e8ba 100644 --- a/crates/shift-source/src/package.rs +++ b/crates/shift-source/src/package.rs @@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize}; use shift_font::{ Anchor, Axis, AxisId, Component, ComponentId, Contour, DecomposedTransform, FeatureData, Font, FontMetadata, FontMetrics, Glyph, GlyphLayer, GlyphName, Guideline, KerningData, KerningPair, - KerningSide, LibData, LibValue, Location, Point, PointType, Source, SourceId, + KerningSide, LibData, LibValue, Location, Point, PointType, Source, SourceId, SourceRole, }; use zip::{CompressionMethod, ZipArchive, ZipWriter, result::ZipError, write::SimpleFileOptions}; @@ -269,6 +269,10 @@ struct SourceDoc { filename: Option, #[serde(default)] color: Option, + #[serde(default)] + role: SourceRole, + #[serde(default)] + layer_name: Option, } #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] @@ -1710,6 +1714,8 @@ impl TryFrom<&Source> for SourceDoc { location, filename: source.filename().map(str::to_string), color: source.color().map(str::to_string), + role: source.role(), + layer_name: source.layer_name().map(str::to_string), }) } } @@ -1734,6 +1740,8 @@ impl TryFrom for Source { doc.filename, ); source.set_color(doc.color); + source.set_role(doc.role); + source.set_layer_name(doc.layer_name); Ok(source) } } diff --git a/crates/shift-source/tests/package_test.rs b/crates/shift-source/tests/package_test.rs index f93ad1b1..8d92b924 100644 --- a/crates/shift-source/tests/package_test.rs +++ b/crates/shift-source/tests/package_test.rs @@ -1,7 +1,7 @@ use std::fs::File; use shift_font::{ - Axis, AxisId, Font, KerningPair, LibValue, Location, Source, SourceId, + Axis, AxisId, Font, KerningPair, LibValue, Location, Source, SourceId, SourceRole, test_support::sample_font, }; use shift_source::{ @@ -401,3 +401,39 @@ fn writes_handwritten_tree_as_openable_zip() { ShiftSourcePackage::open(&package_path).unwrap(); } + +#[test] +fn shift_round_trip_preserves_source_roles_and_layer_names() { + let temp = tempfile::tempdir().unwrap(); + let package_path = temp.path().join("Roles.shift"); + + let mut font = Font::new(); + let mut medium = Source::with_filename( + "Medium".to_string(), + Location::new(), + "Family-Bold.ufo".to_string(), + ); + medium.set_layer_name(Some("Medium".to_string())); + font.add_source(medium); + font.add_source(Source::layer("background".to_string())); + + ShiftSourcePackage::save_font(&package_path, &font).unwrap(); + let loaded = ShiftSourcePackage::load_font(&package_path).unwrap(); + + let medium = loaded + .sources() + .iter() + .find(|source| source.name() == "Medium") + .expect("Medium source should survive"); + assert_eq!(medium.role(), SourceRole::Master); + assert_eq!(medium.layer_name(), Some("Medium")); + assert_eq!(medium.filename(), Some("Family-Bold.ufo")); + + let background = loaded + .sources() + .iter() + .find(|source| source.name() == "background") + .expect("background source should survive"); + assert_eq!(background.role(), SourceRole::Layer); + assert_eq!(background.layer_name(), None); +} diff --git a/crates/shift-store/src/change_set.rs b/crates/shift-store/src/change_set.rs index 810d3d90..6263cc2c 100644 --- a/crates/shift-store/src/change_set.rs +++ b/crates/shift-store/src/change_set.rs @@ -1,7 +1,9 @@ use rusqlite::{Transaction, params}; use shift_font as font; -use crate::{ShiftStore, StoreError, workspace_state::mark_workspace_dirty_in_tx}; +use crate::{ + ShiftStore, StoreError, source::SourceKind, workspace_state::mark_workspace_dirty_in_tx, +}; impl ShiftStore { pub fn apply_change_set(&mut self, change_set: &font::FontChangeSet) -> Result<(), StoreError> { @@ -65,10 +67,14 @@ impl ShiftStore { upsert_source( &tx, &source.id(), - Some(source.name()), - source.filename(), - source.color(), - order_index as i64, + SourceRow { + name: Some(source.name()), + filename: source.filename(), + color: source.color(), + kind: SourceKind::from(source.role()), + layer_name: source.layer_name(), + order_index: order_index as i64, + }, )?; replace_lib_data( &tx, @@ -129,7 +135,18 @@ fn apply_change(tx: &Transaction<'_>, change: &font::FontChange) -> Result<(), S Ok(()) } font::FontChange::SourceCreated(change) => { - upsert_source(tx, &change.source_id, Some(&change.name), None, None, 0)?; + upsert_source( + tx, + &change.source_id, + SourceRow { + name: Some(&change.name), + filename: None, + color: None, + kind: SourceKind::Master, + layer_name: None, + order_index: 0, + }, + )?; for axis_value in &change.location { upsert_source_location( @@ -313,25 +330,41 @@ fn upsert_source_location( Ok(()) } +struct SourceRow<'a> { + name: Option<&'a str>, + filename: Option<&'a str>, + color: Option<&'a str>, + kind: SourceKind, + layer_name: Option<&'a str>, + order_index: i64, +} + fn upsert_source( tx: &Transaction<'_>, source_id: &font::SourceId, - name: Option<&str>, - filename: Option<&str>, - color: Option<&str>, - order_index: i64, + row: SourceRow<'_>, ) -> Result<(), StoreError> { tx.execute( " - INSERT INTO sources (id, name, filename, color, kind, order_index) - VALUES (?1, ?2, ?3, ?4, 'master', ?5) + INSERT INTO sources (id, name, filename, color, kind, layer_name, order_index) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7) ON CONFLICT(id) DO UPDATE SET name = COALESCE(excluded.name, sources.name), filename = excluded.filename, color = excluded.color, + kind = excluded.kind, + layer_name = excluded.layer_name, order_index = excluded.order_index ", - params![source_id.to_string(), name, filename, color, order_index], + params![ + source_id.to_string(), + row.name, + row.filename, + row.color, + row.kind.as_str(), + row.layer_name, + row.order_index + ], )?; Ok(()) } diff --git a/crates/shift-store/src/font_state.rs b/crates/shift-store/src/font_state.rs index f10f0a60..ec25bbec 100644 --- a/crates/shift-store/src/font_state.rs +++ b/crates/shift-store/src/font_state.rs @@ -4,7 +4,7 @@ use std::str::FromStr; use rusqlite::params; use shift_font as font; -use crate::{FontInfo, ShiftStore, StoreError}; +use crate::{FontInfo, ShiftStore, StoreError, source::SourceKind}; impl ShiftStore { pub fn load_font_state(&self) -> Result { @@ -141,7 +141,7 @@ fn load_axes(conn: &rusqlite::Connection) -> Result, StoreError> fn load_sources(conn: &rusqlite::Connection) -> Result, StoreError> { let mut stmt = conn.prepare( " - SELECT id, name, filename, color + SELECT id, name, filename, color, kind, layer_name FROM sources ORDER BY order_index, id ", @@ -157,6 +157,12 @@ fn load_sources(conn: &rusqlite::Connection) -> Result, StoreE row.get(2)?, ); source.set_color(row.get(3)?); + + let kind = SourceKind::parse(row.get(4)?).map_err(|err| { + rusqlite::Error::FromSqlConversionFailure(4, rusqlite::types::Type::Text, Box::new(err)) + })?; + source.set_role(kind.into()); + source.set_layer_name(row.get(5)?); Ok(source) })?; diff --git a/crates/shift-store/src/schema.rs b/crates/shift-store/src/schema.rs index 63f724bd..735cf780 100644 --- a/crates/shift-store/src/schema.rs +++ b/crates/shift-store/src/schema.rs @@ -52,6 +52,7 @@ CREATE TABLE IF NOT EXISTS sources ( style_name TEXT, filename TEXT, color TEXT, + layer_name TEXT, kind TEXT NOT NULL, order_index INTEGER NOT NULL DEFAULT 0 ); diff --git a/crates/shift-store/src/source.rs b/crates/shift-store/src/source.rs index 92447b9e..62b4f0a8 100644 --- a/crates/shift-store/src/source.rs +++ b/crates/shift-store/src/source.rs @@ -43,23 +43,44 @@ pub struct SourceRecord { #[derive(Clone, Debug, Eq, PartialEq)] pub enum SourceKind { Master, + Layer, } impl SourceKind { - fn as_str(&self) -> &'static str { + pub(crate) fn as_str(&self) -> &'static str { match self { SourceKind::Master => "master", + SourceKind::Layer => "layer", } } - fn parse(value: String) -> Result { + pub(crate) fn parse(value: String) -> Result { match value.as_str() { "master" => Ok(SourceKind::Master), + "layer" => Ok(SourceKind::Layer), _ => Err(StoreError::UnknownSourceKind(value)), } } } +impl From for SourceKind { + fn from(role: shift_font::SourceRole) -> Self { + match role { + shift_font::SourceRole::Master => SourceKind::Master, + shift_font::SourceRole::Layer => SourceKind::Layer, + } + } +} + +impl From for shift_font::SourceRole { + fn from(kind: SourceKind) -> Self { + match kind { + SourceKind::Master => shift_font::SourceRole::Master, + SourceKind::Layer => shift_font::SourceRole::Layer, + } + } +} + #[derive(Clone, Debug, PartialEq)] pub struct SourceAxisLocation { pub source_id: SourceId, diff --git a/crates/shift-store/tests/store_test.rs b/crates/shift-store/tests/store_test.rs index 0863d246..8766655d 100644 --- a/crates/shift-store/tests/store_test.rs +++ b/crates/shift-store/tests/store_test.rs @@ -1069,3 +1069,38 @@ fn refuses_stores_from_newer_schema_versions() { std::fs::remove_dir_all(path.parent().unwrap()).ok(); } + +#[test] +fn replace_and_load_font_state_preserves_source_roles_and_layer_names() { + let mut store = ShiftStore::open_memory_for_test().expect("memory store should open"); + + let mut font = shift_font::Font::new(); + let mut medium = shift_font::Source::with_filename( + "Medium".to_string(), + shift_font::Location::new(), + "Family-Bold.ufo".to_string(), + ); + medium.set_layer_name(Some("Medium".to_string())); + font.add_source(medium); + font.add_source(shift_font::Source::layer("background".to_string())); + + store.replace_font_state(&font).expect("replace font state"); + let loaded = store.load_font_state().expect("load font state"); + + let medium = loaded + .sources() + .iter() + .find(|source| source.name() == "Medium") + .expect("Medium source should survive"); + assert_eq!(medium.role(), shift_font::SourceRole::Master); + assert_eq!(medium.layer_name(), Some("Medium")); + assert_eq!(medium.filename(), Some("Family-Bold.ufo")); + + let background = loaded + .sources() + .iter() + .find(|source| source.name() == "background") + .expect("background source should survive"); + assert_eq!(background.role(), shift_font::SourceRole::Layer); + assert_eq!(background.layer_name(), None); +}