Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions src/designspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,11 @@ impl DesignSpaceDocument {
close_already::fs::write(path, buf)?;
Ok(())
}

/// Returns a [`DesignSpaceDocument`] loaded from an XML string.
pub fn load_str(contents: &str) -> Result<DesignSpaceDocument, DesignSpaceLoadError> {
quick_xml::de::from_str(contents).map_err(DesignSpaceLoadError::DeError)
}
Comment on lines +316 to +319
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

total nit but I would prefer this to be right below the main load fn, above.

}

impl Rules {
Expand Down
11 changes: 11 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,17 @@ pub enum FontLoadError {
/// The underlying error.
source: PlistError,
},
/// Failed to load a file through a custom source resolver.
#[error("failed to read '{path}' from source resolver")]
ResolverIo {
/// The requested path.
path: PathBuf,
/// The underlying error.
source: IoError,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think io::Error is the right error type, here; the error type on SourceResolver also isn't right, see below.

},
/// Resolver-based loading currently supports only UFO v3 fonts.
#[error("resolver-based loading currently supports only UFO v3")]
ResolverUnsupportedFormatVersion,
/// Norad can currently only open UFO (directory) packages.
#[error("only UFO (directory) packages are supported")]
UfoNotADir,
Expand Down
277 changes: 277 additions & 0 deletions src/font.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

#![deny(rustdoc::broken_intra_doc_links)]

use std::collections::BTreeMap;
use std::fs;
use std::io::{Cursor, ErrorKind};
use std::path::{Path, PathBuf};

use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -34,6 +36,73 @@ static DEFAULT_METAINFO_CREATOR: &str = "org.linebender.norad";
pub(crate) static DATA_DIR: &str = "data";
pub(crate) static IMAGES_DIR: &str = "images";

/// Abstraction over loading UTF-8 UFO files.
///
/// This can be implemented by filesystem-backed loaders, in-memory maps,
/// or custom loaders.
pub trait SourceResolver {
/// Return UTF-8 contents for the provided path.
///
/// Returning `Ok(None)` indicates a missing file.
fn get_contents(&self, path: &Path) -> Result<Option<String>, FontLoadError>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What we want here is some very customizable error type that the implementor can use to communicate why the given resource isn't available; FontLoadError covers a lot of specific cases that aren't relevant to the job of "give me a string for this path".

The 'best' option is probably an associated type, so this would look like:

trait SourceResolver {
    type Error;
    fn get_contents(&self, path: &Path) -> Result<Option<String>, Self::Error>;
}

but this might be a bit fiddly if you're not that comfortable with rust? Because we would probably need bounds on the error type elsewhere, so that the top-level Resolver variant would contain a Box<dyn std::error::Error>...


/// Resolve a raw request path.
fn resolve_raw_path(&self, path: &Path, _requested_from: Option<&Path>) -> PathBuf {
path.to_path_buf()
}

/// Canonicalize a path if needed.
fn canonicalize(&self, path: &Path) -> Result<PathBuf, FontLoadError> {
Ok(path.to_path_buf())
}
Comment on lines +49 to +57
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

both of these methods are important for resolving include statements in a FEA file but are (I believe) irrelevant for your use-case, and can be removed.

}

impl<F> SourceResolver for F
where
F: Fn(&Path) -> Option<String>,
{
fn get_contents(&self, path: &Path) -> Result<Option<String>, FontLoadError> {
Ok((self)(path))
}
}

/// Filesystem-backed [`SourceResolver`].
#[derive(Default)]
pub struct FileSystemResolver {
project_root: PathBuf,
}

impl FileSystemResolver {
/// Create a resolver rooted at `project_root`.
pub fn new(project_root: PathBuf) -> Self {
Self { project_root }
}
}

impl SourceResolver for FileSystemResolver {
fn get_contents(&self, path: &Path) -> Result<Option<String>, FontLoadError> {
match fs::read_to_string(path) {
Ok(contents) => Ok(Some(contents)),
Err(source) if source.kind() == ErrorKind::NotFound => Ok(None),
Err(source) => Err(FontLoadError::ResolverIo { path: path.to_path_buf(), source }),
}
}

fn resolve_raw_path(&self, path: &Path, requested_from: Option<&Path>) -> PathBuf {
if path.is_absolute() {
return path.to_path_buf();
}
if let Some(parent) = requested_from.and_then(Path::parent) {
return parent.join(path);
}
self.project_root.join(path)
}

fn canonicalize(&self, path: &Path) -> Result<PathBuf, FontLoadError> {
Ok(path.to_path_buf())
}
}

Comment on lines +69 to +105
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you even need a file-system backed resolver? this is used in fea-rs because the resolver API is the only API for handling include statements, but norad already has an API for loading from the file system...

/// A font object, corresponding to a [UFO directory].
/// A Unified Font Object.
///
Expand Down Expand Up @@ -212,6 +281,115 @@ impl Font {
Self::load_impl(path.as_ref(), request)
}

/// Returns a [`Font`] loaded via a custom [`SourceResolver`].
///
/// Paths are resolved relative to the resolver root and must be UFO-root
/// relative (for example `metainfo.plist`, `glyphs/contents.plist`, etc.).
///
/// Note: resolver-based loading currently supports only UFO v3 and does not
/// include data/images stores.
pub fn load_with_resolver(
request: DataRequest,
resolver: impl SourceResolver,
) -> Result<Font, FontLoadError> {
Self::load_with_resolver_impl(request, &resolver)
}

fn load_with_resolver_impl(
request: DataRequest,
resolver: &dyn SourceResolver,
) -> Result<Font, FontLoadError> {
Comment on lines +298 to +301
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

have a separate _impl method that we delegate too is unnecessary here (and is actually unnecessary in our main load fn too now, I believe it's a relic from an earlier time)

let metainfo_str = required_file(resolver, Path::new(METAINFO_FILE), None)?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this required_file method is only called once, i'd just inline that bit?

let mut meta: MetaInfo = plist::from_reader(Cursor::new(metainfo_str.as_bytes()))
.map_err(|source| FontLoadError::ParsePlist { name: METAINFO_FILE, source })?;

if meta.format_version != FormatVersion::V3 {
return Err(FontLoadError::ResolverUnsupportedFormatVersion);
}

let mut lib = if request.lib {
match optional_file(resolver, Path::new(LIB_FILE), None)? {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think optional_file can also just be removed, we don't need to worry about canonicalization etc so should just call resolver.get_contents directly?

Some(lib_str) => plist::Value::from_reader(Cursor::new(lib_str.as_bytes()))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you shouldn't need Cursor here, just Value::from_reader_xml(lib_str.as_bytes())?

.map_err(|source| FontLoadError::ParsePlist { name: LIB_FILE, source })?
.into_dictionary()
.ok_or(FontLoadError::LibFileMustBeDictionary)?,
None => Plist::new(),
}
} else {
Plist::new()
};

let font_info = if let Some(fontinfo_str) =
optional_file(resolver, Path::new(FONTINFO_FILE), None)?
{
let mut font_info: FontInfo = plist::from_reader(Cursor::new(fontinfo_str.as_bytes()))
.map_err(|source| FontLoadError::ParsePlist { name: FONTINFO_FILE, source })?;
font_info
.validate()
.map_err(crate::error::FontInfoLoadError::InvalidData)
.map_err(FontLoadError::FontInfo)?;
font_info.load_object_libs(&mut lib).map_err(FontLoadError::FontInfo)?;
font_info
} else {
Default::default()
};

let groups = if request.groups {
match optional_file(resolver, Path::new(GROUPS_FILE), None)? {
Some(groups_str) => {
let groups: Groups =
plist::from_reader(Cursor::new(groups_str.as_bytes())).map_err(
|source| FontLoadError::ParsePlist { name: GROUPS_FILE, source },
)?;
validate_groups(&groups).map_err(FontLoadError::InvalidGroups)?;
Some(groups)
}
None => None,
}
} else {
None
};

let kerning = if request.kerning {
match optional_file(resolver, Path::new(KERNING_FILE), None)? {
Some(kerning_str) => {
let kerning: Kerning =
plist::from_reader(Cursor::new(kerning_str.as_bytes())).map_err(
|source| FontLoadError::ParsePlist { name: KERNING_FILE, source },
)?;
Some(kerning)
}
None => None,
}
} else {
None
};

let features = if request.features {
optional_file(resolver, Path::new(FEATURES_FILE), None)?.unwrap_or_default()
} else {
Default::default()
};

let layers = load_layer_set_from_resolver(resolver, &request.layers)?;

let (groups, kerning) = (groups, kerning);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?


meta.format_version = FormatVersion::V3;

Ok(Font {
layers,
meta,
font_info,
lib,
groups: groups.unwrap_or_default(),
kerning: kerning.unwrap_or_default(),
features,
data: Default::default(),
images: Default::default(),
})
}

fn load_impl(path: &Path, request: DataRequest) -> Result<Font, FontLoadError> {
let metadata = path.metadata().map_err(FontLoadError::AccessUfoDir)?;
if !metadata.is_dir() {
Expand Down Expand Up @@ -606,6 +784,105 @@ impl Font {
}
}

fn optional_file(
resolver: &dyn SourceResolver,
path: &Path,
requested_from: Option<&Path>,
) -> Result<Option<String>, FontLoadError> {
let resolved = resolver.resolve_raw_path(path, requested_from);
let canonical = resolver.canonicalize(&resolved)?;
resolver.get_contents(&canonical)
}

fn required_file(
resolver: &dyn SourceResolver,
path: &Path,
requested_from: Option<&Path>,
) -> Result<String, FontLoadError> {
optional_file(resolver, path, requested_from)?.ok_or(FontLoadError::MissingMetaInfoFile)
}

fn load_layer_set_from_resolver(
resolver: &dyn SourceResolver,
filter: &LayerFilter,
) -> Result<LayerContents, FontLoadError> {
let layer_descriptors: Vec<(Name, PathBuf)> =
match optional_file(resolver, Path::new(LAYER_CONTENTS_FILE), None)? {
Some(layercontents_str) => {
plist::from_reader(Cursor::new(layercontents_str.as_bytes())).map_err(|source| {
FontLoadError::ParsePlist { name: LAYER_CONTENTS_FILE, source }
})?
}
None => vec![(Name::new_raw("public.default"), PathBuf::from("glyphs"))],
};

let mut layers = LayerContents::default();
for (layer_name, layer_dir) in layer_descriptors {
if !filter.should_load(&layer_name, &layer_dir) {
continue;
}

let layer = if layer_dir == Path::new("glyphs") {
layers.default_layer_mut()
} else {
let layer = layers.get_or_create_layer(layer_name.as_str()).map_err(|_| {
FontLoadError::Layer {
name: layer_name.to_string(),
path: layer_dir.clone(),
source: Box::new(crate::error::LayerLoadError::MissingContentsFile),
}
})?;
layer.path = layer_dir.clone();
layer
};

let contents_path = layer_dir.join("contents.plist");
let contents_str =
optional_file(resolver, &contents_path, None)?.ok_or(FontLoadError::Layer {
name: layer_name.to_string(),
path: contents_path.clone(),
source: Box::new(crate::error::LayerLoadError::MissingContentsFile),
})?;

let glyph_files: BTreeMap<Name, PathBuf> =
plist::from_reader(Cursor::new(contents_str.as_bytes())).map_err(|source| {
FontLoadError::Layer {
name: layer_name.to_string(),
path: contents_path.clone(),
source: Box::new(crate::error::LayerLoadError::ParsePlist {
name: "contents.plist",
source,
}),
}
})?;

for (_glyph_name, glif_relative_path) in glyph_files {
let glif_path = layer_dir.join(&glif_relative_path);
let glif_contents =
optional_file(resolver, &glif_path, None)?.ok_or(FontLoadError::Layer {
name: layer_name.to_string(),
path: glif_path.clone(),
source: Box::new(crate::error::LayerLoadError::MissingContentsFile),
})?;
let mut glyph = Glyph::parse_raw(glif_contents.as_bytes()).map_err(|source| {
FontLoadError::Layer {
name: layer_name.to_string(),
path: glif_path.clone(),
source: Box::new(crate::error::LayerLoadError::Glyph {
name: glif_relative_path.to_string_lossy().to_string(),
path: glif_path.clone(),
source,
}),
}
})?;
glyph.name = Name::new_raw(&glyph.name);
layer.insert_glyph(glyph);
}
}

Ok(layers)
}

fn load_lib(lib_path: &Path) -> Result<plist::Dictionary, FontLoadError> {
plist::Value::from_file(lib_path)
.map_err(|source| FontLoadError::ParsePlist { name: LIB_FILE, source })?
Expand Down
2 changes: 1 addition & 1 deletion src/fontinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,7 @@ impl FontInfo {

/// Move libs from the font lib's `public.objectLibs` key into the actual objects.
/// The key will be removed from the font lib.
fn load_object_libs(&mut self, lib: &mut Plist) -> Result<(), FontInfoLoadError> {
pub(crate) fn load_object_libs(&mut self, lib: &mut Plist) -> Result<(), FontInfoLoadError> {
let mut object_libs = match lib.remove(PUBLIC_OBJECT_LIBS_KEY) {
Some(lib) => {
lib.into_dictionary().ok_or(FontInfoLoadError::PublicObjectLibsMustBeDictionary)?
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ pub(crate) mod util;
mod write;

pub use data_request::DataRequest;
pub use font::{Font, FormatVersion, MetaInfo};
pub use font::{FileSystemResolver, Font, FormatVersion, MetaInfo, SourceResolver};
pub use fontinfo::FontInfo;
pub use glyph::{
AffineTransform, Anchor, Codepoints, Component, Contour, ContourPoint, Glyph, Image, PointType,
Expand Down
Loading