Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Designspace v5 fields #340

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions src/designspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ pub struct DesignSpaceDocument {
/// One or more rules.
#[serde(default, skip_serializing_if = "Rules::is_empty")]
pub rules: Rules,
/// Zero or more variable fonts.
#[serde(
rename = "variable-fonts",
with = "serde_impls::variable_fonts",
default,
skip_serializing_if = "Vec::is_empty"
)]
pub variable_fonts: Vec<VariableFont>,
/// One or more sources.
#[serde(with = "serde_impls::sources", skip_serializing_if = "Vec::is_empty")]
pub sources: Vec<Source>,
Expand Down Expand Up @@ -162,6 +170,55 @@ pub struct Condition {
pub maximum: Option<f32>,
}

/// Describes a single variable font.
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]
pub struct VariableFont {
/// The name of the variable font.
#[serde(rename = "@name")]
pub name: String,
/// The optional file name of the variable font.
#[serde(rename = "@filename", default, skip_serializing_if = "Option::is_none")]
pub filename: Option<String>,
/// The axis subset the variable font represents.
#[serde(
rename = "axis-subsets",
with = "serde_impls::axis_subsets",
skip_serializing_if = "Vec::is_empty"
)]
pub axis_subsets: Vec<AxisSubset>,
/// Additional arbitrary user data
#[serde(default, with = "serde_plist", skip_serializing_if = "Dictionary::is_empty")]
pub lib: Dictionary,
}

/// Describes a single axis subset for a variable font.
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[serde(untagged)]
pub enum AxisSubset {
/// Describes a range of an axis.
Range {
/// The name of the axis under consideration.
name: String,
/// Optionally, the lower end of the range, in user coordinates
#[serde(rename = "@userminimum", default, skip_serializing_if = "Option::is_none")]
user_minimum: Option<f64>,
/// Optionally, the upper end of the range, in user coordinates
#[serde(rename = "@usermaximum", default, skip_serializing_if = "Option::is_none")]
user_maximum: Option<f64>,
/// Optionally, the new default value of subset axis, in user coordinates.
#[serde(rename = "@userdefault", default, skip_serializing_if = "Option::is_none")]
user_default: Option<f64>,
},
/// Describes a single point of an axis.
Discrete {
/// The name of the axis under consideration.
name: String,
/// The single point of the axis.
#[serde(rename = "@uservalue")]
user_value: f64,
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more useful/helpful to use inner structs for the range/discrete values here. By keeping the values in the enum itself, you have to hold an AxisSubset and can only access inner fields with matching, whereas if there's an inner struct you can pull (a reference to) that out the first time you match and hold that for future use.

So, unless you specifically expect users to only need to match on this enum and inspect its values once, I'd recommend creating AxisSubsetRange and AxisSubsetDiscrete structs (or similar)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By this you mean going for an enum AxisSubset { Range(AxisSubsetRange), Discrete(AxisSubsetDiscrete) }?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry if that wasn't clear!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both with the current and your suggested approach, I run into DeError(Custom("data did not match any variant of untagged enum AxisSubset")) when trying to load MutatorSans2.designspace 🤔 Not sure what's going on there...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the name fields need a #[serde(rename = "@name")] annotation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh DUH, thanks. Now I get a different error. Progress.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I move the structs out, I am back at the "data did not match any variant" error and when I leave them as they are, the discrete variant is deserialized as a ranged one. The joys of untagged data.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the discrete variant first in the enum declaration, as it's harder to match (requiring name and user_value). If serde isn't rejecting unknown fields, then discrete can match range.

As for why decomposing the enum isn't working, it probably something to do with how (IIRC) the XML deserialisation cares about the struct name(?) Worth finding a workaround to before we merge this though imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the variant up top also didn't work... Maybe I'll need to write a custom function for this.


/// A [source].
///
/// [source]: https://fonttools.readthedocs.io/en/latest/designspaceLib/xml.html#id25
Expand Down Expand Up @@ -335,6 +392,7 @@ mod serde_impls {
{
use ::serde::Deserialize as _;
#[derive(::serde::Deserialize)]
#[serde(rename_all = "kebab-case")]
struct Helper {
$field_name: Vec<$inner>,
}
Expand All @@ -350,6 +408,7 @@ mod serde_impls {
{
use ::serde::Serialize as _;
#[derive(::serde::Serialize)]
#[serde(rename_all = "kebab-case")]
struct Helper<'a> {
$field_name: &'a [$inner],
}
Expand All @@ -364,6 +423,8 @@ mod serde_impls {
serde_from_field!(instances, instance, crate::designspace::Instance);
serde_from_field!(axes, axis, crate::designspace::Axis);
serde_from_field!(sources, source, crate::designspace::Source);
serde_from_field!(variable_fonts, variable_font, crate::designspace::VariableFont);
serde_from_field!(axis_subsets, axis_subset, crate::designspace::AxisSubset);
}

#[cfg(test)]
Expand Down Expand Up @@ -575,4 +636,46 @@ mod tests {
}
);
}

#[test]
fn load_save_round_trip_mutatorsans2() {
// Given
let dir = TempDir::new().unwrap();
let ds_test_save_location = dir.path().join("MutatorSans2.designspace");

// When
let ds_initial = DesignSpaceDocument::load("testdata/MutatorSans2.designspace").unwrap();
ds_initial.save(&ds_test_save_location).expect("failed to save designspace");
let ds_after = DesignSpaceDocument::load(ds_test_save_location)
.expect("failed to load saved designspace");

let mut vf_lib = Dictionary::new();
let mut vf_fontinfo = Dictionary::new();
vf_fontinfo.insert("familyName".into(), "My Font Narrow VF".into());
vf_fontinfo.insert("styleName".into(), "Regular".into());
vf_fontinfo.insert("postscriptFontName".into(), "MyFontNarrVF-Regular".into());
vf_fontinfo
.insert("trademark".into(), "My Font Narrow VF is a registered trademark...".into());
vf_lib.insert("public.fontInfo".into(), vf_fontinfo.into());

// Then
assert_eq!(
&ds_after.variable_fonts,
&[VariableFont {
name: "MyFontNarrVF".into(),
filename: None,
axis_subsets: vec![
AxisSubset::Range {
name: "Weight".into(),
user_minimum: None,
user_maximum: None,
user_default: None
},
AxisSubset::Discrete { name: "Width".into(), user_value: 75.0 },
],
lib: vf_lib
}]
);
assert_eq!(ds_initial, ds_after);
}
}
2 changes: 2 additions & 0 deletions src/fontinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,8 @@ struct FontInfoV1 {
year: Option<Integer>, // Does not appear in spec but ufoLib.
}

// TODO: add from_bytes method for loading fontinfo from designspace libs

impl FontInfo {
/// Returns [`FontInfo`] from a file, upgrading from the supplied `format_version` to the highest
/// internally supported version.
Expand Down
15 changes: 11 additions & 4 deletions src/serde_xml_plist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ mod de {
use super::*;
use serde::de::{Error as DeError, Visitor};
use serde::Deserialize;
use std::borrow::Cow;
use std::fmt::Display;
use std::marker::PhantomData;
use std::str::FromStr;
Expand Down Expand Up @@ -106,15 +107,21 @@ mod de {
Some(ValueKeyword::Data) => {
//FIXME: remove this + base64 dep when/if we merge
//<https://github.com/ebarnard/rust-plist/pull/122>
let b64_str = map.next_value::<&str>()?;
// NOTE: Use Cow here because serde needs ownership if it has
madig marked this conversation as resolved.
Show resolved Hide resolved
// to modify the string in any form, e.g. when deserializing
// from a Designspace file.
let b64_str = map.next_value::<Cow<'_, str>>()?;
base64_standard
.decode(b64_str)
.decode(&*b64_str)
.map(Value::Data)
.map_err(|e| A::Error::custom(format!("Invalid XML data: '{e}'")))
}
Some(ValueKeyword::Date) => {
let date_str = map.next_value::<&str>()?;
plist::Date::from_xml_format(date_str).map_err(A::Error::custom).map(Value::Date)
// NOTE: Use Cow here because serde needs ownership if it has
// to modify the string in any form, e.g. when deserializing
// from a Designspace file.
let date_str = map.next_value::<Cow<'_, str>>()?;
plist::Date::from_xml_format(&date_str).map_err(A::Error::custom).map(Value::Date)
}
Some(ValueKeyword::Real) => map.next_value::<f64>().map(Value::Real),
Some(ValueKeyword::Integer) => {
Expand Down
Loading