From 30350ac6cde7995a8cbc491fb60f818d98052e69 Mon Sep 17 00:00:00 2001 From: Ricky Atkins Date: Thu, 9 Mar 2023 12:38:10 +0000 Subject: [PATCH 1/3] Add LayerSet::get_or_create_layer --- src/layer.rs | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/layer.rs b/src/layer.rs index 091e6ae3..a8da5071 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -136,6 +136,14 @@ impl LayerSet { self.layers.iter().map(|l| &l.name) } + fn create_layer(&mut self, name: Name) -> &mut Layer { + let path = util::default_file_name_for_layer_name(&name, &self.path_set); + let layer = Layer::new(name, path); + self.path_set.insert(layer.path.to_string_lossy().to_lowercase()); + self.layers.push(layer); + self.layers.last_mut().unwrap() + } + /// Returns a new layer with the given name. pub fn new_layer(&mut self, name: &str) -> Result<&mut Layer, NamingError> { if name == DEFAULT_LAYER_NAME { @@ -144,11 +152,22 @@ impl LayerSet { Err(NamingError::Duplicate(name.to_string())) } else { let name = Name::new(name).map_err(|_| NamingError::Invalid(name.into()))?; - let path = crate::util::default_file_name_for_layer_name(&name, &self.path_set); - let layer = Layer::new(name, path); - self.path_set.insert(layer.path.to_string_lossy().to_lowercase()); - self.layers.push(layer); - Ok(self.layers.last_mut().unwrap()) + Ok(self.create_layer(name)) + } + } + + /// Returns a mutable reference to a layer, by name, creating the layer if it doesn't exist + pub fn get_or_create_layer(&mut self, name: &str) -> Result<&mut Layer, NamingError> { + let index = self.layers.iter().position(|l| l.name.as_str() == name); + match index { + Some(its_here) => Ok(&mut self.layers[its_here]), + None => { + // We can avoid the formal checks that new_layer does because by checking if it's already in the list, + // we know that name is not the default name or a duplicate + // Skipping it also avoids iterating through self.layers a second time + let name = Name::new(name)?; + Ok(self.create_layer(name)) + } } } From 6d70fffb63addcd9061fac1db2230fd5ed2e0ecd Mon Sep 17 00:00:00 2001 From: Ricky Atkins Date: Thu, 9 Mar 2023 12:55:54 +0000 Subject: [PATCH 2/3] Test LayerSet::get_or_create_layer --- src/layer.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/layer.rs b/src/layer.rs index a8da5071..85d09333 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -748,6 +748,32 @@ mod tests { ); } + #[test] + fn layer_get_or_create() { + let mut ufo = crate::Font::load("testdata/MutatorSansLightWide.ufo").unwrap(); + assert_eq!( + ufo.layers.len(), + 2, + "number of layers in test data MutatorSansLightWide changed" + ); + + let result = ufo.layers.get_or_create_layer("background"); + assert!(result.is_ok()); + assert_eq!(ufo.layers.len(), 2); + + let result = ufo.layers.get_or_create_layer("middleground"); + assert!(result.is_ok()); + assert_eq!(ufo.layers.len(), 3, "new layer wasn't created"); + assert!( + ufo.layers.layers.iter().any(|layer| layer.name().as_str() == "foreground"), + "new layer was created with the wrong name" + ); + + let result = ufo.layers.get_or_create_layer("\t"); + assert!(matches!(result, Err(NamingError::Invalid(_)))); + assert_eq!(ufo.layers.len(), 3); + } + #[test] fn rename_layer() { let mut layer_set = LayerSet::default(); From 3b2b83f8520de33804e6df8c56b6d8ab09ec3551 Mon Sep 17 00:00:00 2001 From: Ricky Atkins Date: Fri, 10 Mar 2023 10:04:55 +0000 Subject: [PATCH 3/3] Make get_or_create_layer more robust At the cost of some efficiency (now iterates through layers twice) --- src/layer.rs | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/layer.rs b/src/layer.rs index 85d09333..452aec15 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -136,14 +136,6 @@ impl LayerSet { self.layers.iter().map(|l| &l.name) } - fn create_layer(&mut self, name: Name) -> &mut Layer { - let path = util::default_file_name_for_layer_name(&name, &self.path_set); - let layer = Layer::new(name, path); - self.path_set.insert(layer.path.to_string_lossy().to_lowercase()); - self.layers.push(layer); - self.layers.last_mut().unwrap() - } - /// Returns a new layer with the given name. pub fn new_layer(&mut self, name: &str) -> Result<&mut Layer, NamingError> { if name == DEFAULT_LAYER_NAME { @@ -152,7 +144,11 @@ impl LayerSet { Err(NamingError::Duplicate(name.to_string())) } else { let name = Name::new(name).map_err(|_| NamingError::Invalid(name.into()))?; - Ok(self.create_layer(name)) + let path = util::default_file_name_for_layer_name(&name, &self.path_set); + let layer = Layer::new(name, path); + self.path_set.insert(layer.path.to_string_lossy().to_lowercase()); + self.layers.push(layer); + Ok(self.layers.last_mut().unwrap()) } } @@ -161,13 +157,7 @@ impl LayerSet { let index = self.layers.iter().position(|l| l.name.as_str() == name); match index { Some(its_here) => Ok(&mut self.layers[its_here]), - None => { - // We can avoid the formal checks that new_layer does because by checking if it's already in the list, - // we know that name is not the default name or a duplicate - // Skipping it also avoids iterating through self.layers a second time - let name = Name::new(name)?; - Ok(self.create_layer(name)) - } + None => self.new_layer(name), } } @@ -772,6 +762,10 @@ mod tests { let result = ufo.layers.get_or_create_layer("\t"); assert!(matches!(result, Err(NamingError::Invalid(_)))); assert_eq!(ufo.layers.len(), 3); + + let result = ufo.layers.get_or_create_layer(DEFAULT_LAYER_NAME); + assert!(matches!(result, Err(NamingError::ReservedName))); + assert_eq!(ufo.layers.len(), 3); } #[test]