Fix group creation parenting (#455)

* WIP fix of folder crash / indent

* Fixed most crashes

* Known cases of crash / incorrect behavior resolved

* Removed todo & readability tweak

* Removed left over test

* Resolved clippy issue resulting from merging master

* Replace recursive tree traversal with prefix matching

* Reverted changes for #453 and added undo functionality to Paste

* Make uuid generator thread local for tests

* Maintain layer order, test still failing 50% of the time

* Reverting back to known working with the knowledge we can optimize later.

This reverts commit 1aaf5c0.

* Revert "Make uuid generator thread local for tests"

This reverts commit d68e3b9.

* Revert "Reverted changes for #453 and added undo functionality to Paste"

This reverts commit d66992a.

* Revert "Replace recursive tree traversal with prefix matching"

This reverts commit 6bcbb9f.

* Reverted to working state and added comment back to optimized commit hash

* Re-removed the changes involving #453

Co-authored-by: Dennis <[email protected]>

Author: otdavies

editor/src/document/document_file.rs

@@ -273,6 +273,37 @@ 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();
+				}
+			}
+		}
+
+		let mut without_children: Vec<Vec<LayerId>> = vec![];
+		recurse_layer_tree(self, vec![], &mut without_children, false);
+		without_children
+	}
+
+	pub fn selected_layers_contains(&self, path: &[LayerId]) -> bool {
+		self.layer_metadata.get(path).map(|layer| layer.selected).unwrap_or(false)
+	}
+
 	pub fn selected_visible_layers(&self) -> impl Iterator<Item = &[LayerId]> {
 		self.selected_layers().filter(|path| match self.graphene_document.layer(path) {
 			Ok(layer) => layer.visible,
@@ -562,12 +593,13 @@ impl MessageHandler<DocumentMessage, &InputPreprocessor> for DocumentMessageHand
 				responses.push_back(DocumentMessage::SetLayerExpansion(path, true).into());
 			}
 			GroupSelectedLayers => {
-				let selected_layers = self.selected_layers();
+				let mut new_folder_path: Vec<u64> = self.graphene_document.shallowest_common_folder(self.selected_layers()).unwrap_or(&[]).to_vec();
 
-				let common_prefix = self.graphene_document.common_layer_path_prefix(selected_layers);
-				let (_id, common_prefix) = common_prefix.split_last().unwrap_or((&0, &[]));
+				// Required for grouping parent folders with their own children
+				if !new_folder_path.is_empty() && self.selected_layers_contains(&new_folder_path) {
+					new_folder_path.remove(new_folder_path.len() - 1);
+				}
 
-				let mut new_folder_path = common_prefix.to_vec();
 				new_folder_path.push(generate_uuid());
 
 				responses.push_back(DocumentsMessage::Copy(Clipboard::System).into());
@@ -618,10 +650,12 @@ impl MessageHandler<DocumentMessage, &InputPreprocessor> for DocumentMessageHand
 			}
 			DeleteSelectedLayers => {
 				self.backup(responses);
-				responses.push_front(ToolMessage::DocumentIsDirty.into());
-				for path in self.selected_layers().map(|path| path.to_vec()) {
+
+				for path in self.selected_layers_without_children() {
 					responses.push_front(DocumentOperation::DeleteLayer { path }.into());
 				}
+
+				responses.push_front(ToolMessage::DocumentIsDirty.into());
 			}
 			SetViewMode(mode) => {
 				self.view_mode = mode;
@@ -652,6 +686,7 @@ impl MessageHandler<DocumentMessage, &InputPreprocessor> for DocumentMessageHand
 						let layer = self.layer_metadata_mut(&selected);
 						layer.selected = !layer.selected;
 						responses.push_back(LayerChanged(selected.clone()).into());
+						responses.push_back(ToolMessage::DocumentIsDirty.into());
 					} else {
 						paths.push(selected.clone());
 					}

editor/src/document/document_message_handler.rs

@@ -352,15 +352,18 @@ impl MessageHandler<DocumentsMessage, &InputPreprocessor> for DocumentsMessageHa
 				responses.push_back(DocumentsMessage::SelectDocument(prev_id).into());
 			}
 			Copy(clipboard) => {
-				let paths = self.active_document().selected_layers_sorted();
-				self.copy_buffer[clipboard as usize].clear();
-				for path in paths {
-					let document = self.active_document();
-					match (document.graphene_document.layer(&path).map(|t| t.clone()), *document.layer_metadata(&path)) {
+				// We can't use `self.active_document()` because it counts as an immutable borrow of the entirety of `self`
+				let active_document = self.documents.get(&self.active_document_id).unwrap();
+
+				let copy_buffer = &mut self.copy_buffer;
+				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)) {
 						(Ok(layer), layer_metadata) => {
-							self.copy_buffer[clipboard as usize].push(CopyBufferEntry { layer, layer_metadata });
+							copy_buffer[clipboard as usize].push(CopyBufferEntry { layer, layer_metadata });
 						}
-						(Err(e), _) => warn!("Could not access selected layer {:?}: {:?}", path, e),
+						(Err(e), _) => warn!("Could not access selected layer {:?}: {:?}", layer_path, e),
 					}
 				}
 			}
@@ -372,9 +375,9 @@ impl MessageHandler<DocumentsMessage, &InputPreprocessor> for DocumentsMessageHa
 				let document = self.active_document();
 				let shallowest_common_folder = document
 					.graphene_document
-					.deepest_common_folder(document.selected_layers())
+					.shallowest_common_folder(document.selected_layers())
 					.expect("While pasting, the selected layers did not exist while attempting to find the appropriate folder path for insertion");
-
+				responses.push_back(StartTransaction.into());
 				responses.push_back(
 					PasteIntoFolder {
 						clipboard,
@@ -383,6 +386,7 @@ impl MessageHandler<DocumentsMessage, &InputPreprocessor> for DocumentsMessageHa
 					}
 					.into(),
 				);
+				responses.push_back(CommitTransaction.into());
 			}
 			PasteIntoFolder { clipboard, path, insert_index } => {
 				let paste = |entry: &CopyBufferEntry, responses: &mut VecDeque<_>| {

graphene/src/document.rs

@@ -61,7 +61,7 @@ impl Document {
 	pub fn folder(&self, path: &[LayerId]) -> Result<&Folder, DocumentError> {
 		let mut root = &self.root;
 		for id in path {
-			root = root.as_folder()?.layer(*id).ok_or(DocumentError::LayerNotFound)?;
+			root = root.as_folder()?.layer(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 		}
 		root.as_folder()
 	}
@@ -72,7 +72,7 @@ impl Document {
 	fn folder_mut(&mut self, path: &[LayerId]) -> Result<&mut Folder, DocumentError> {
 		let mut root = &mut self.root;
 		for id in path {
-			root = root.as_folder_mut()?.layer_mut(*id).ok_or(DocumentError::LayerNotFound)?;
+			root = root.as_folder_mut()?.layer_mut(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 		}
 		root.as_folder_mut()
 	}
@@ -83,7 +83,7 @@ impl Document {
 			return Ok(&self.root);
 		}
 		let (path, id) = split_path(path)?;
-		self.folder(path)?.layer(id).ok_or(DocumentError::LayerNotFound)
+		self.folder(path)?.layer(id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))
 	}
 
 	/// Returns a mutable reference to the layer or folder at the path.
@@ -92,10 +92,10 @@ impl Document {
 			return Ok(&mut self.root);
 		}
 		let (path, id) = split_path(path)?;
-		self.folder_mut(path)?.layer_mut(id).ok_or(DocumentError::LayerNotFound)
+		self.folder_mut(path)?.layer_mut(id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))
 	}
 
-	pub fn deepest_common_folder<'a>(&self, layers: impl Iterator<Item = &'a [LayerId]>) -> Result<&'a [LayerId], DocumentError> {
+	pub fn shallowest_common_folder<'a>(&self, layers: impl Iterator<Item = &'a [LayerId]>) -> Result<&'a [LayerId], DocumentError> {
 		let common_prefix_of_path = self.common_layer_path_prefix(layers);
 
 		Ok(match self.layer(common_prefix_of_path)?.data {
@@ -113,6 +113,10 @@ impl Document {
 			.unwrap_or_default()
 	}
 
+	pub fn is_folder(&self, path: &[LayerId]) -> bool {
+		return self.folder(path).is_ok();
+	}
+
 	// Determines which layer is closer to the root, if path_a return true, if path_b return false
 	// Answers the question: Is A closer to the root than B?
 	pub fn layer_closer_to_root(&self, path_a: &[u64], path_b: &[u64]) -> bool {
@@ -136,9 +140,9 @@ impl Document {
 		false
 	}
 
-	// Is the target layer between a <-> b layers, inclusive
+	// Is  the target layer between a <-> b layers, inclusive
 	pub fn layer_is_between(&self, target: &[u64], path_a: &[u64], path_b: &[u64]) -> bool {
-		// If the target is a nonsense path, it isn't between
+		// If the target is the root, it isn't between
 		if target.is_empty() {
 			return false;
 		}
@@ -165,12 +169,12 @@ impl Document {
 
 		// TODO: appears to be n^2? should we maintain a lookup table?
 		for id in path {
-			let pos = root.layer_ids.iter().position(|x| *x == *id).ok_or(DocumentError::LayerNotFound)?;
+			let pos = root.layer_ids.iter().position(|x| *x == *id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 			indices.push(pos);
-			root = root.folder(*id).ok_or(DocumentError::LayerNotFound)?;
+			root = root.folder(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 		}
 
-		indices.push(root.layer_ids.iter().position(|x| *x == layer_id).ok_or(DocumentError::LayerNotFound)?);
+		indices.push(root.layer_ids.iter().position(|x| *x == layer_id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?);
 
 		Ok(indices)
 	}
@@ -264,7 +268,7 @@ impl Document {
 		let mut root = &mut self.root;
 		root.cache_dirty = true;
 		for id in path {
-			root = root.as_folder_mut()?.layer_mut(*id).ok_or(DocumentError::LayerNotFound)?;
+			root = root.as_folder_mut()?.layer_mut(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 			root.cache_dirty = true;
 		}
 		Ok(())
@@ -297,7 +301,7 @@ impl Document {
 		let mut transforms = vec![self.root.transform];
 		for id in path {
 			if let Ok(folder) = root.as_folder() {
-				root = folder.layer(*id).ok_or(DocumentError::LayerNotFound)?;
+				root = folder.layer(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 			}
 			transforms.push(root.transform);
 		}
@@ -309,7 +313,7 @@ impl Document {
 		let mut trans = self.root.transform;
 		for id in path {
 			if let Ok(folder) = root.as_folder() {
-				root = folder.layer(*id).ok_or(DocumentError::LayerNotFound)?;
+				root = folder.layer(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
 			}
 			trans = trans * root.transform;
 		}

graphene/src/layers/folder.rs

@@ -101,8 +101,12 @@ impl Folder {
 		Some(&mut self.layers[pos])
 	}
 
+	pub fn folder_contains(&self, id: LayerId) -> bool {
+		self.layer_ids.contains(&id)
+	}
+
 	pub fn position_of_layer(&self, layer_id: LayerId) -> Result<usize, DocumentError> {
-		self.layer_ids.iter().position(|x| *x == layer_id).ok_or(DocumentError::LayerNotFound)
+		self.layer_ids.iter().position(|x| *x == layer_id).ok_or_else(|| DocumentError::LayerNotFound([layer_id].into()))
 	}
 
 	pub fn folder(&self, id: LayerId) -> Option<&Folder> {

graphene/src/lib.rs

@@ -14,7 +14,7 @@ pub type LayerId = u64;
 
 #[derive(Debug, Clone, PartialEq)]
 pub enum DocumentError {
-	LayerNotFound,
+	LayerNotFound(Vec<LayerId>),
 	InvalidPath,
 	IndexOutOfBounds,
 	NotAFolder,