Skip to content

Commit

Permalink
Make get_or_create_layer more robust
Browse files Browse the repository at this point in the history
At the cost of some efficiency (now iterates through layers twice)
  • Loading branch information
RickyDaMa authored and cmyr committed Mar 10, 2023
1 parent b513f52 commit 4015621
Showing 1 changed file with 10 additions and 16 deletions.
26 changes: 10 additions & 16 deletions src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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())
}
}

Expand All @@ -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),
}
}

Expand Down Expand Up @@ -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]
Expand Down

0 comments on commit 4015621

Please sign in to comment.