From 3dee2346707df657a01487c59e129e9ee1467694 Mon Sep 17 00:00:00 2001 From: otdavies Date: Sun, 2 Jan 2022 18:30:40 -0800 Subject: [PATCH] Known cases of crash / incorrect behavior resolved --- editor/src/document/document_file.rs | 38 +++++++++++++------ .../src/document/document_message_handler.rs | 11 ++++-- graphene/src/document.rs | 3 +- graphene/src/layers/folder.rs | 1 - 4 files changed, 36 insertions(+), 17 deletions(-) diff --git a/editor/src/document/document_file.rs b/editor/src/document/document_file.rs index 049fe78655..2abae6c1a5 100644 --- a/editor/src/document/document_file.rs +++ b/editor/src/document/document_file.rs @@ -277,15 +277,31 @@ impl DocumentMessageHandler { self.layer_data.iter().filter_map(|(path, data)| data.selected.then(|| path.as_slice())) } - pub fn selected_layers_without_children(&self) -> impl Iterator { - let selected_folders: Vec<&Folder> = self - .layer_data - .iter() - .filter_map(|(path, data)| (data.selected && self.graphene_document.is_folder(path)).then(|| self.graphene_document.folder(path).unwrap())) - .collect(); + pub fn selected_layers_without_children(&self) -> Vec> { + let mut without_children: Vec> = vec![]; + recurse_layer_tree(self, vec![], &mut without_children, false); - self.selected_layers() - .filter(move |path| selected_folders.is_empty() || !selected_folders.iter().any(|folder| (*folder).folder_contains(path[path.len() - 1]))) + // Traversing the layer tree was chosen for both readability and instead of an n^2 comparison approach. + // A future optmiziation would be not needing to start at the root [] + fn recurse_layer_tree(ctx: &DocumentMessageHandler, mut path: Vec, without_children: &mut Vec>, 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(); + } + } + } + without_children } pub fn selected_layers_contains(&self, path: &[LayerId]) -> bool { @@ -579,10 +595,10 @@ impl MessageHandler for DocumentMessageHand responses.push_back(DocumentMessage::SetLayerExpansion(path, true).into()); } GroupSelectedLayers => { - let selected_layers = self.selected_layers(); // TODO simplify and protect unwrap - let mut new_folder_path: Vec = self.graphene_document.deepest_common_folder(selected_layers).unwrap().to_vec(); + let mut new_folder_path: Vec = self.graphene_document.shallowest_common_folder(self.selected_layers()).unwrap_or(&[]).to_vec(); + // 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); } @@ -638,7 +654,7 @@ impl MessageHandler for DocumentMessageHand DeleteSelectedLayers => { self.backup(responses); - for path in self.selected_layers_without_children().map(|path| path.to_vec()) { + for path in self.selected_layers_without_children() { responses.push_front(DocumentOperation::DeleteLayer { path }.into()); } diff --git a/editor/src/document/document_message_handler.rs b/editor/src/document/document_message_handler.rs index 8e2a1be73c..2190b96cc6 100644 --- a/editor/src/document/document_message_handler.rs +++ b/editor/src/document/document_message_handler.rs @@ -359,7 +359,7 @@ impl MessageHandler 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_data(layer_path)) { + match (active_document.graphene_document.layer(&layer_path).map(|t| t.clone()), *active_document.layer_data(&layer_path)) { (Ok(layer), layer_data) => { copy_buffer[clipboard as usize].push(CopyBufferEntry { layer, layer_data }); } @@ -373,11 +373,16 @@ impl MessageHandler for DocumentsMessageHa } Paste(clipboard) => { let document = self.active_document(); - let shallowest_common_folder = document + let mut 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"); + // We want to paste folders at the same depth as their copy source + if !shallowest_common_folder.is_empty() && document.selected_layers_contains(shallowest_common_folder) { + shallowest_common_folder = &shallowest_common_folder[..shallowest_common_folder.len() - 1]; + } + responses.push_back( PasteIntoFolder { clipboard, diff --git a/graphene/src/document.rs b/graphene/src/document.rs index 40a3c44c28..15e8ff17d2 100644 --- a/graphene/src/document.rs +++ b/graphene/src/document.rs @@ -95,7 +95,7 @@ impl Document { self.folder_mut(path)?.layer_mut(id).ok_or_else(|| DocumentError::LayerNotFound(path.into())) } - pub fn deepest_common_folder<'a>(&self, layers: impl Iterator) -> Result<&'a [LayerId], DocumentError> { + pub fn shallowest_common_folder<'a>(&self, layers: impl Iterator) -> Result<&'a [LayerId], DocumentError> { let common_prefix_of_path = self.common_layer_path_prefix(layers); Ok(match self.layer(common_prefix_of_path)?.data { @@ -546,7 +546,6 @@ impl Document { Some(vec![LayerChanged { path: path.clone() }]) } Operation::CreateFolder { path } => { - log::debug!("Creating a folder with path {:?}", path); self.set_layer(path, Layer::new(LayerDataType::Folder(Folder::default()), DAffine2::IDENTITY.to_cols_array()), -1)?; self.mark_as_dirty(path)?; diff --git a/graphene/src/layers/folder.rs b/graphene/src/layers/folder.rs index 65de0ddf36..2f92efa758 100644 --- a/graphene/src/layers/folder.rs +++ b/graphene/src/layers/folder.rs @@ -102,7 +102,6 @@ impl Folder { } pub fn folder_contains(&self, id: LayerId) -> bool { - log::debug!("For {:?} Folder does contain {:?}", id, self.layer_ids.contains(&id)); self.layer_ids.contains(&id) }