Tidy up path handling in document_file (#464)

* Tidy up path handling in document_file

+ Improve #455

* Cargo Clippy lints

* Rename to_vec to map_to_vec

Co-authored-by: Oliver Davies <[email protected]>

Author: TrueDoctor

editor/src/communication/dispatcher.rs

@@ -339,10 +339,19 @@ mod test {
 		init_logger();
 		let mut editor = create_editor_with_three_layers();
 
-		let sorted_layers = editor.dispatcher.documents_message_handler.active_document().all_layers_sorted();
+		fn map_to_vec<'a>(paths: Vec<&'a [LayerId]>) -> Vec<Vec<LayerId>> {
+			paths.iter().map(|layer| layer.to_vec()).collect::<Vec<_>>()
+		}
+		let sorted_layers = map_to_vec(editor.dispatcher.documents_message_handler.active_document().all_layers_sorted());
 		println!("Sorted layers: {:?}", sorted_layers);
 
-		let verify_order = |handler: &mut DocumentMessageHandler| (handler.all_layers_sorted(), handler.non_selected_layers_sorted(), handler.selected_layers_sorted());
+		let verify_order = |handler: &mut DocumentMessageHandler| {
+			(
+				map_to_vec(handler.all_layers_sorted()),
+				map_to_vec(handler.non_selected_layers_sorted()),
+				map_to_vec(handler.selected_layers_sorted()),
+			)
+		};
 
 		editor.handle_message(DocumentMessage::SetSelectedLayers(sorted_layers[..2].to_vec()));
 

editor/src/document/document_file.rs

@@ -273,31 +273,14 @@ impl DocumentMessageHandler {
 		self.layer_metadata.iter().filter_map(|(path, data)| data.selected.then(|| path.as_slice()))
 	}
 
-	pub fn selected_layers_without_children(&self) -> Vec<Vec<LayerId>> {
-		// TODO optimize this after MVP. Look at commit 1aaf5c0d7dbe67cd9f4ba8b536a4771a2cef6439, optimization was started
-		// Traversing the layer tree recursively was chosen for readability, but should be replaced with an optimized approach later
-		fn recurse_layer_tree(ctx: &DocumentMessageHandler, mut path: Vec<u64>, without_children: &mut Vec<Vec<LayerId>>, selected: bool) {
-			if let Ok(folder) = ctx.graphene_document.folder(&path) {
-				for child in folder.list_layers() {
-					path.push(*child);
-					let selected_or_parent_selected = selected || ctx.selected_layers_contains(&path);
-					let selected_without_any_parent_selected = !selected && ctx.selected_layers_contains(&path);
-					if ctx.graphene_document.is_folder(&path) {
-						if selected_without_any_parent_selected {
-							without_children.push(path.clone());
-						}
-						recurse_layer_tree(ctx, path.clone(), without_children, selected_or_parent_selected);
-					} else if selected_without_any_parent_selected {
-						without_children.push(path.clone());
-					}
-					path.pop();
-				}
-			}
-		}
+	pub fn selected_layers_without_children(&self) -> Vec<&[LayerId]> {
+		let mut sorted_layers = self.selected_layers().collect::<Vec<_>>();
+		// Sorting here creates groups of similar UUID paths
+		sorted_layers.sort();
+		sorted_layers.dedup_by(|a, b| a.starts_with(b));
 
-		let mut without_children: Vec<Vec<LayerId>> = vec![];
-		recurse_layer_tree(self, vec![], &mut without_children, false);
-		without_children
+		// We need to maintain layer ordering
+		self.sort_layers(sorted_layers.iter().copied())
 	}
 
 	pub fn selected_layers_contains(&self, path: &[LayerId]) -> bool {
@@ -365,23 +348,19 @@ impl DocumentMessageHandler {
 	}
 
 	/// Returns an unsorted list of all layer paths including folders at all levels, except the document's top-level root folder itself
-	pub fn all_layers(&self) -> Vec<Vec<LayerId>> {
-		self.layer_metadata.keys().filter(|path| !path.is_empty()).cloned().collect()
+	pub fn all_layers(&self) -> impl Iterator<Item = &[LayerId]> {
+		self.layer_metadata.keys().filter_map(|path| (!path.is_empty()).then(|| path.as_slice()))
 	}
 
 	/// Returns the paths to all layers in order, optionally including only selected or non-selected layers.
-	fn layers_sorted(&self, selected: Option<bool>) -> Vec<Vec<LayerId>> {
+	fn sort_layers<'a>(&self, paths: impl Iterator<Item = &'a [LayerId]>) -> Vec<&'a [LayerId]> {
 		// Compute the indices for each layer to be able to sort them
-		let mut layers_with_indices: Vec<(Vec<LayerId>, Vec<usize>)> = self
-
-			.layer_metadata
-			.iter()
+		let mut layers_with_indices: Vec<(&[LayerId], Vec<usize>)> = paths
 			// 'path.len() > 0' filters out root layer since it has no indices
-			.filter_map(|(path, data)| (!path.is_empty() && (data.selected == selected.unwrap_or(data.selected))).then(|| path.clone()))
+			.filter_map(|path| (!path.is_empty()).then(|| path))
 			.filter_map(|path| {
-				// TODO: Currently it is possible that `layer_metadata` contains layers that are don't actually exist (has been partially fixed in #281) and thus
 				// TODO: `indices_for_path` can return an error. We currently skip these layers and log a warning. Once this problem is solved this code can be simplified.
-				match self.graphene_document.indices_for_path(&path) {
+				match self.graphene_document.indices_for_path(path) {
 					Err(err) => {
 						warn!("layers_sorted: Could not get indices for the layer {:?}: {:?}", path, err);
 						None
@@ -396,19 +375,19 @@ impl DocumentMessageHandler {
 	}
 
 	/// Returns the paths to all layers in order
-	pub fn all_layers_sorted(&self) -> Vec<Vec<LayerId>> {
-		self.layers_sorted(None)
+	pub fn all_layers_sorted(&self) -> Vec<&[LayerId]> {
+		self.sort_layers(self.all_layers())
 	}
 
 	/// Returns the paths to all selected layers in order
-	pub fn selected_layers_sorted(&self) -> Vec<Vec<LayerId>> {
-		self.layers_sorted(Some(true))
+	pub fn selected_layers_sorted(&self) -> Vec<&[LayerId]> {
+		self.sort_layers(self.selected_layers())
 	}
 
 	/// Returns the paths to all non_selected layers in order
 	#[allow(dead_code)] // used for test cases
-	pub fn non_selected_layers_sorted(&self) -> Vec<Vec<LayerId>> {
-		self.layers_sorted(Some(false))
+	pub fn non_selected_layers_sorted(&self) -> Vec<&[LayerId]> {
+		self.sort_layers(self.all_layers().filter(|layer| !self.selected_layers().any(|path| &path == layer)))
 	}
 
 	pub fn layer_metadata(&self, path: &[LayerId]) -> &LayerMetadata {
@@ -652,7 +631,7 @@ impl MessageHandler<DocumentMessage, &InputPreprocessor> for DocumentMessageHand
 				self.backup(responses);
 
 				for path in self.selected_layers_without_children() {
-					responses.push_front(DocumentOperation::DeleteLayer { path }.into());
+					responses.push_front(DocumentOperation::DeleteLayer { path: path.to_vec() }.into());
 				}
 
 				responses.push_front(ToolMessage::DocumentIsDirty.into());
@@ -664,7 +643,7 @@ impl MessageHandler<DocumentMessage, &InputPreprocessor> for DocumentMessageHand
 			DuplicateSelectedLayers => {
 				self.backup(responses);
 				for path in self.selected_layers_sorted() {
-					responses.push_back(DocumentOperation::DuplicateLayer { path }.into());
+					responses.push_back(DocumentOperation::DuplicateLayer { path: path.to_vec() }.into());
 				}
 			}
 			SelectLayer(selected, ctrl, shift) => {
@@ -730,7 +709,7 @@ impl MessageHandler<DocumentMessage, &InputPreprocessor> for DocumentMessageHand
 			}
 			SelectAllLayers => {
 				let all_layer_paths = self.all_layers();
-				responses.push_front(SetSelectedLayers(all_layer_paths).into());
+				responses.push_front(SetSelectedLayers(all_layer_paths.map(|path| path.to_vec()).collect()).into());
 			}
 			DeselectAllLayers => {
 				responses.push_front(SetSelectedLayers(vec![]).into());

editor/src/document/document_message_handler.rs

@@ -359,7 +359,7 @@ impl MessageHandler<DocumentsMessage, &InputPreprocessor> for DocumentsMessageHa
 				copy_buffer[clipboard as usize].clear();
 
 				for layer_path in active_document.selected_layers_without_children() {
-					match (active_document.graphene_document.layer(&layer_path).map(|t| t.clone()), *active_document.layer_metadata(&layer_path)) {
+					match (active_document.graphene_document.layer(layer_path).map(|t| t.clone()), *active_document.layer_metadata(layer_path)) {
 						(Ok(layer), layer_metadata) => {
 							copy_buffer[clipboard as usize].push(CopyBufferEntry { layer, layer_metadata });
 						}

editor/src/document/overlay_message_handler.rs

@@ -8,7 +8,6 @@ use graphene::Operation as DocumentOperation;
 use graphene::document::Document as GrapheneDocument;
 use graphene::layers::style::ViewMode;
 use serde::{Deserialize, Serialize};
-use std::collections::{HashMap, VecDeque};
 
 #[impl_message(Message, DocumentMessage, Overlay)]
 #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
@@ -26,7 +25,6 @@ impl From<DocumentOperation> for OverlayMessage {
 #[derive(Debug, Clone, Default)]
 pub struct OverlayMessageHandler {
 	pub overlays_graphene_document: GrapheneDocument,
-	overlay_path_mapping: HashMap<Vec<LayerId>, Vec<LayerId>>,
 }
 
 impl MessageHandler<OverlayMessage, (&mut LayerMetadata, &Document, &InputPreprocessor)> for OverlayMessageHandler {

editor/src/tool/snapping.rs

@@ -5,26 +5,21 @@ use crate::consts::SNAP_TOLERANCE;
 
 use super::DocumentMessageHandler;
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Default)]
 pub struct SnapHandler {
 	snap_targets: Option<(Vec<f64>, Vec<f64>)>,
 }
-impl Default for SnapHandler {
-	fn default() -> Self {
-		Self { snap_targets: None }
-	}
-}
 
 impl SnapHandler {
 	/// Gets a list of snap targets for the X and Y axes in Viewport coords for the target layers (usually all layers or all non-selected layers.)
 	/// This should be called at the start of a drag.
-	pub fn start_snap(&mut self, document_message_handler: &DocumentMessageHandler, target_layers: Vec<Vec<LayerId>>, ignore_layers: &[Vec<LayerId>]) {
+	pub fn start_snap(&mut self, document_message_handler: &DocumentMessageHandler, target_layers: Vec<&[LayerId]>, ignore_layers: &[Vec<LayerId>]) {
 		if document_message_handler.snapping_enabled {
 			// Could be made into sorted Vec or a HashSet for more performant lookups.
 			self.snap_targets = Some(
 				target_layers
 					.iter()
-					.filter(|path| !ignore_layers.contains(path))
+					.filter(|path| !ignore_layers.iter().any(|layer| layer.as_slice() == **path))
 					.filter_map(|path| document_message_handler.graphene_document.viewport_bounding_box(path).ok()?)
 					.flat_map(|[bound1, bound2]| [bound1, bound2, ((bound1 + bound2) / 2.)])
 					.map(|vec| vec.into())

graphene/src/layers/mod.rs

@@ -189,17 +189,11 @@ impl<'a> IntoIterator for &'a Layer {
 	}
 }
 
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct LayerIter<'a> {
 	pub stack: Vec<&'a Layer>,
 }
 
-impl Default for LayerIter<'_> {
-	fn default() -> Self {
-		Self { stack: vec![] }
-	}
-}
-
 impl<'a> Iterator for LayerIter<'a> {
 	type Item = &'a Layer;