From 3cccab517a3527c4caa8aa6aeb9020a56c61a3e4 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Mon, 28 Jun 2021 12:29:13 -0400 Subject: [PATCH 1/2] Rename ufo.rs to font.rs --- src/{ufo.rs => font.rs} | 4 ++-- src/layer.rs | 3 ++- src/lib.rs | 6 +++--- src/upconversion.rs | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) rename src/{ufo.rs => font.rs} (99%) diff --git a/src/ufo.rs b/src/font.rs similarity index 99% rename from src/ufo.rs rename to src/font.rs index d583f377..864ecb7d 100644 --- a/src/ufo.rs +++ b/src/font.rs @@ -157,12 +157,12 @@ impl Default for MetaInfo { } impl Font { - /// Create a new `Ufo`. + /// Create a new, empty `Font` object. pub fn new() -> Self { Font::default() } - /// Create a new `Ufo` only with certain fields + /// Create a new `Font` only with certain fields. pub fn with_fields(data_request: DataRequest) -> Self { let mut ufo = Self::new(); ufo.data_request = data_request; diff --git a/src/layer.rs b/src/layer.rs index 547f7440..e3bb437c 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -32,7 +32,7 @@ pub struct LayerSet { layers: Vec, } -#[allow(clippy::clippy::len_without_is_empty)] // never empty +#[allow(clippy::len_without_is_empty)] // never empty impl LayerSet { /// Load the layers from the provided path. /// @@ -79,6 +79,7 @@ impl LayerSet { /// The number of layers in the set. /// /// This should be non-zero. + #[allow(clippy::len_without_is_empty)] pub fn len(&self) -> usize { self.layers.len() } diff --git a/src/lib.rs b/src/lib.rs index c975e6d9..80314e37 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -22,6 +22,7 @@ extern crate serde_derive; extern crate serde_repr; pub mod error; +mod font; pub mod fontinfo; mod glyph; mod guideline; @@ -29,11 +30,11 @@ mod identifier; mod layer; mod names; mod shared_types; -mod ufo; mod upconversion; pub mod util; pub use error::Error; +pub use font::{DataRequest, Font, FormatVersion, MetaInfo}; pub use fontinfo::FontInfo; pub use glyph::{ AffineTransform, Anchor, Component, Contour, ContourPoint, GlifVersion, Glyph, GlyphName, @@ -43,7 +44,6 @@ pub use guideline::{Guideline, Line}; pub use identifier::Identifier; pub use layer::{Layer, LayerSet}; pub use shared_types::{Color, IntegerOrFloat, NonNegativeIntegerOrFloat, Plist}; -pub use ufo::{DataRequest, Font, FormatVersion, MetaInfo}; #[allow(deprecated)] -pub use ufo::Ufo; +pub use font::Ufo; diff --git a/src/upconversion.rs b/src/upconversion.rs index 0c551727..1a76ea42 100644 --- a/src/upconversion.rs +++ b/src/upconversion.rs @@ -1,10 +1,10 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::Path; +use crate::font::{Groups, Kerning}; use crate::fontinfo::FontInfo; use crate::names::NameList; use crate::shared_types::IntegerOrFloat; -use crate::ufo::{Groups, Kerning}; use crate::Error; /// Convert kerning groups and pairs from v1 and v2 informal conventions to @@ -216,8 +216,8 @@ mod tests { extern crate maplit; use super::*; + use crate::font::{Font, FormatVersion}; use crate::glyph::GlyphName; - use crate::ufo::{Font, FormatVersion}; use maplit::btreemap; #[test] From a9b118ab6afa6065f994c52b2126ef730d84cf5e Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Mon, 28 Jun 2021 12:44:12 -0400 Subject: [PATCH 2/2] Rework loading methods This is mainly intended to clear up confusion around how to use load_ufo; the answer is "don't". I believe that the previous API of splitting up the creation of the font from the loading was unnecessary. I also suspect that having the Font object hold on to the DataRequest is unnecessary, although there are possible arguments in favor of this approach. In any case, the main confusion with the previous API is that loading with a data request took `&self` by reference but returned a new font object, which is confusing. I would expect it either to take a DataRequest as an argument to an associated function, or I would expect it to take `&mut self` and load the contents of the file into the current font? --- src/font.rs | 54 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/src/font.rs b/src/font.rs index 864ecb7d..80192b7e 100644 --- a/src/font.rs +++ b/src/font.rs @@ -163,35 +163,47 @@ impl Font { } /// Create a new `Font` only with certain fields. + #[doc(hidden)] + #[deprecated( + since = "0.4.1", + note = "To load only specific fields, use Font::load_requestd_data" + )] pub fn with_fields(data_request: DataRequest) -> Self { let mut ufo = Self::new(); ufo.data_request = data_request; ufo } - /// Attempt to load a font object from a file. `path` must point to - /// a directory with the structure described in [v3 of the Unified Font Object][v3] - /// spec. + /// Attempt to load a font object from a file. + /// + /// `path` must point to a directory with the structure described in + /// [v3 of the Unified Font Object][v3] spec. /// - /// NOTE: This will consume the `public.objectLibs` key in the global lib and in glyph - /// libs and assign object libs found therein to global guidelines and glyph objects - /// with the matching identifier, respectively. + /// # Note + /// + /// This will consume the `public.objectLibs` key in the global lib + /// and in glyph libs and assign object libs found therein to global + /// guidelines and glyph objects with the matching identifier, respectively. /// /// [v3]: http://unifiedfontobject.org/versions/ufo3/ pub fn load>(path: P) -> Result { - Self::new().load_ufo(path) + Self::load_requested_data(path, DataRequest::default()) } - pub fn load_ufo>(&self, path: P) -> Result { + /// Attempt to load the requested elements of a font object from a file. + pub fn load_requested_data( + path: impl AsRef, + request: DataRequest, + ) -> Result { let path = path.as_ref(); // minimize monomorphization - let load_impl = |ufo: &Font, path: &Path| -> Result { + let load_impl = |path: &Path| -> Result { let meta_path = path.join(METAINFO_FILE); let mut meta: MetaInfo = plist::from_file(meta_path)?; let lib_path = path.join(LIB_FILE); - let mut lib = if lib_path.exists() && self.data_request.lib { + let mut lib = if lib_path.exists() && request.lib { plist::Value::from_file(&lib_path)?.into_dictionary().ok_or_else(|| { Error::ExpectedPlistDictionary(lib_path.to_string_lossy().into_owned()) })? @@ -209,7 +221,7 @@ impl Font { }; let groups_path = path.join(GROUPS_FILE); - let groups = if groups_path.exists() && self.data_request.groups { + let groups = if groups_path.exists() && request.groups { let groups: Groups = plist::from_file(groups_path)?; validate_groups(&groups).map_err(Error::InvalidGroups)?; Some(groups) @@ -218,7 +230,7 @@ impl Font { }; let kerning_path = path.join(KERNING_FILE); - let kerning = if kerning_path.exists() && self.data_request.kerning { + let kerning = if kerning_path.exists() && request.kerning { let kerning: Kerning = plist::from_file(kerning_path)?; Some(kerning) } else { @@ -226,7 +238,7 @@ impl Font { }; let features_path = path.join(FEATURES_FILE); - let mut features = if features_path.exists() && self.data_request.features { + let mut features = if features_path.exists() && request.features { let features = fs::read_to_string(features_path)?; Some(features) } else { @@ -234,7 +246,7 @@ impl Font { }; let glyph_names = NameList::default(); - let layers = if self.data_request.layers { + let layers = if request.layers { if meta.format_version == FormatVersion::V3 && !path.join(LAYER_CONTENTS_FILE).exists() { @@ -283,11 +295,19 @@ impl Font { groups, kerning, features, - data_request: ufo.data_request, + data_request: request, }) }; - load_impl(&self, path) + load_impl(path) + } + + #[deprecated( + since = "0.4.1", + note = "To load only specific fields, use Font::load_requestd_data" + )] + pub fn load_ufo>(&self, path: P) -> Result { + Font::load_requested_data(path, self.data_request) } /// Attempt to save this UFO to the given path, overriding any existing contents. @@ -573,7 +593,7 @@ mod tests { #[test] fn data_request() { let path = "testdata/mutatorSans/MutatorSansLightWide.ufo"; - let font_obj = Font::with_fields(DataRequest::none()).load_ufo(path).unwrap(); + let font_obj = Font::load_requested_data(path, DataRequest::none()).unwrap(); assert_eq!(font_obj.iter_layers().count(), 1); assert!(font_obj.layers.default_layer().is_empty()); assert_eq!(font_obj.lib, Plist::new());