From 1e93188f50aaf84b7c93b21e2b06d4946891a427 Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Thu, 2 Jul 2026 18:40:15 +0100 Subject: [PATCH 1/4] feat: distinguish designspace masters from layer-only sources in the IR Source now carries a role (Master vs Layer) and an optional UFO layer binding. The UFO reader marks synthesized non-default-layer sources as layer-only, the designspace reader records each binding, and the UFO writer returns layer-bound data to the layer it came from instead of the source name. --- .../shift-backends/src/designspace/reader.rs | 16 ++--- crates/shift-backends/src/ufo/reader.rs | 8 ++- crates/shift-backends/src/ufo/writer.rs | 5 +- crates/shift-font/src/ir/mod.rs | 2 +- crates/shift-font/src/ir/source.rs | 59 +++++++++++++++++++ 5 files changed, 75 insertions(+), 15 deletions(-) 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/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..04894670 100644 --- a/crates/shift-backends/src/ufo/writer.rs +++ b/crates/shift-backends/src/ufo/writer.rs @@ -445,9 +445,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-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..c40ce09f 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,26 @@ impl Source { self.filename.as_deref() } + pub fn role(&self) -> SourceRole { + self.role + } + + pub fn is_master(&self) -> bool { + self.role == SourceRole::Master + } + + /// 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> { From 91ad20893d4f173354a1c91f7b0ba1eae4b86c59 Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Thu, 2 Jul 2026 18:41:06 +0100 Subject: [PATCH 2/4] fix: save designspace sources back to their own UFOs with atomic XML writes The designspace writer previously collapsed every project into a single {stem}.ufo with all non-default sources as layers, restructuring multi-UFO projects and leaving the original master UFOs stale on disk. Sources are now grouped by their stored filename and written back to the UFO files they came from, mirroring the declared layer shape; masters created inside Shift get deterministic collision-safe filenames. Layer-only sources are written as UFO layers without gaining entries. The designspace XML itself now goes through a shared crash-safe helper (temp file + fsync + rename + platform-gated parent fsync, extracted from the UFO writer) instead of a direct overwrite, and is always written after the UFO data so the pointer file never references half-written masters. --- Cargo.lock | 1 + crates/shift-backends/Cargo.toml | 1 + crates/shift-backends/src/atomic.rs | 81 ++++ .../shift-backends/src/designspace/writer.rs | 379 ++++++++++++++---- crates/shift-backends/src/lib.rs | 1 + crates/shift-backends/src/ufo/writer.rs | 17 +- 6 files changed, 395 insertions(+), 85 deletions(-) create mode 100644 crates/shift-backends/src/atomic.rs 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/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/writer.rs b/crates/shift-backends/src/ufo/writer.rs index 04894670..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)] From 8b78d95434d3533a16fe8c6095308ac05a547634 Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Thu, 2 Jul 2026 18:41:23 +0100 Subject: [PATCH 3/4] test: designspace multi-UFO structure, save atomicity, and layer promotion Covers the multi-UFO round trip (same UFO filenames, source-to-file- to-layer assignment, locations, and geometry after reopen), that a failed save leaves the existing designspace XML intact, and that plain UFO extra layers stay UFO layers instead of becoming designspace masters, with and without axes. --- crates/shift-backends/tests/round_trip.rs | 1 + .../tests/round_trip/designspace.rs | 358 ++++++++++++++++++ 2 files changed, 359 insertions(+) create mode 100644 crates/shift-backends/tests/round_trip/designspace.rs 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" + ); +} From d52c95c1ac19093597c2eb37ac800205421192ed Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Thu, 2 Jul 2026 18:49:50 +0100 Subject: [PATCH 4/4] feat: persist source roles and layer bindings in the native stores The master-vs-layer distinction and designspace layer bindings now survive both persistence layers: the SQLite store gains a layer_name column and writes the real source kind instead of hardcoding 'master', and .shift's sources.json carries role and layerName. Both default to Master/absent so existing data reads unchanged. --- crates/shift-font/src/ir/source.rs | 4 ++ crates/shift-source/src/package.rs | 10 +++- crates/shift-source/tests/package_test.rs | 38 ++++++++++++++- crates/shift-store/src/change_set.rs | 59 ++++++++++++++++++----- crates/shift-store/src/font_state.rs | 10 +++- crates/shift-store/src/schema.rs | 1 + crates/shift-store/src/source.rs | 25 +++++++++- crates/shift-store/tests/store_test.rs | 35 ++++++++++++++ 8 files changed, 163 insertions(+), 19 deletions(-) diff --git a/crates/shift-font/src/ir/source.rs b/crates/shift-font/src/ir/source.rs index c40ce09f..8da09a40 100644 --- a/crates/shift-font/src/ir/source.rs +++ b/crates/shift-font/src/ir/source.rs @@ -118,6 +118,10 @@ impl Source { 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 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); +}