From e16e80f5345c71fd0daef343ae295206f92e34b1 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Mon, 28 Jun 2021 12:44:12 -0400 Subject: [PATCH] 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());