Fix deleting layers (#1441)

* Fix deleting layers

* Code review comments

document-legacy/src/document_metadata.rs

@@ -77,7 +77,7 @@ impl DocumentMetadata {
 		self.structure.entry(node_identifier).or_default()
 	}
 
-	pub fn shallowest_unique_layers<'a>(&self, layers: impl Iterator<Item = &'a LayerNodeIdentifier>) -> Vec<Vec<LayerNodeIdentifier>> {
+	pub fn shallowest_unique_layers(&self, layers: impl Iterator<Item = LayerNodeIdentifier>) -> Vec<Vec<LayerNodeIdentifier>> {
 		let mut sorted_layers = layers
 			.map(|layer| {
 				let mut layer_path = layer.ancestors(self).collect::<Vec<_>>();

editor/src/messages/portfolio/document/document_message_handler.rs

@@ -100,14 +100,24 @@ impl Default for DocumentMessageHandler {
 	}
 }
 
-impl MessageHandler<DocumentMessage, (u64, &InputPreprocessorMessageHandler, &PersistentData, &PreferencesMessageHandler, &mut NodeGraphExecutor)> for DocumentMessageHandler {
+pub struct DocumentInputs<'a> {
+	pub document_id: u64,
+	pub ipp: &'a InputPreprocessorMessageHandler,
+	pub persistent_data: &'a PersistentData,
+	pub preferences: &'a PreferencesMessageHandler,
+	pub executor: &'a mut NodeGraphExecutor,
+}
+
+impl MessageHandler<DocumentMessage, DocumentInputs<'_>> for DocumentMessageHandler {
 	#[remain::check]
-	fn process_message(
+	fn process_message(&mut self, message: DocumentMessage, responses: &mut VecDeque<Message>, document_inputs: DocumentInputs) {
-		&mut self,
+		let DocumentInputs {
-		message: DocumentMessage,
+			document_id,
-		responses: &mut VecDeque<Message>,
+			ipp,
-		(document_id, ipp, persistent_data, preferences, executor): (u64, &InputPreprocessorMessageHandler, &PersistentData, &PreferencesMessageHandler, &mut NodeGraphExecutor),
+			persistent_data,
-	) {
+			preferences,
+			executor,
+		} = document_inputs;
 		use DocumentMessage::*;
 
 		let render_data = RenderData::new(&persistent_data.font_cache, self.view_mode, Some(ipp.document_bounds()));
@@ -300,8 +310,10 @@ impl MessageHandler<DocumentMessage, (u64, &InputPreprocessorMessageHandler, &Pe
 				self.backup(responses);
 
 				responses.add_front(BroadcastEvent::SelectionChanged);
-				for path in self.selected_layers_without_children() {
+				for path in self.metadata().shallowest_unique_layers(self.metadata().selected_layers()) {
-					responses.add_front(DocumentMessage::DeleteLayer { layer_path: path.to_vec() });
+					responses.add_front(DocumentMessage::DeleteLayer {
+						layer_path: path.last().unwrap().to_path(),
+					});
 				}
 
 				responses.add(BroadcastEvent::DocumentIsDirty);
@@ -906,6 +918,12 @@ impl MessageHandler<DocumentMessage, (u64, &InputPreprocessorMessageHandler, &Pe
 	}
 
 	fn actions(&self) -> ActionList {
+		unimplemented!("Must use `actions_with_graph_open` instead (unless we change every implementation of the MessageHandler trait).")
+	}
+}
+
+impl DocumentMessageHandler {
+	pub fn actions_with_graph_open(&self, graph_open: bool) -> ActionList {
 		let mut common = actions!(DocumentMessageDiscriminant;
 			Undo,
 			Redo,
@@ -922,7 +940,7 @@ impl MessageHandler<DocumentMessage, (u64, &InputPreprocessorMessageHandler, &Pe
 			CreateEmptyFolder,
 		);
 
-		if self.layer_metadata.values().any(|data| data.selected) {
+		if self.metadata().selected_layers().next().is_some() {
 			let select = actions!(DocumentMessageDiscriminant;
 				DeleteSelectedLayers,
 				DuplicateSelectedLayers,
@@ -937,7 +955,7 @@ impl MessageHandler<DocumentMessage, (u64, &InputPreprocessorMessageHandler, &Pe
 			common.extend(select);
 		}
 		common.extend(self.navigation_handler.actions());
-		common.extend(self.node_graph_handler.actions());
+		common.extend(self.node_graph_handler.actions_with_node_graph_open(graph_open));
 		common
 	}
 }

editor/src/messages/portfolio/document/mod.rs

@@ -10,4 +10,4 @@ pub mod utility_types;
 #[doc(inline)]
 pub use document_message::{DocumentMessage, DocumentMessageDiscriminant};
 #[doc(inline)]
-pub use document_message_handler::DocumentMessageHandler;
+pub use document_message_handler::{DocumentInputs, DocumentMessageHandler};

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

@@ -347,13 +347,15 @@ impl NodeGraphMessageHandler {
 			return false;
 		}
 
-		let mut first_input_node: Option<NodeInput> = None;
+		let mut reconnect_to_input: Option<NodeInput> = None;
 
 		if reconnect {
 			// Check whether the being-deleted node's first (primary) input is a node
 			if let Some(node) = network.nodes.get(&deleting_node_id) {
-				if matches!(node.inputs.first(), Some(NodeInput::Node { .. })) {
+				// Reconnect to the node below when deleting a layer node.
-					first_input_node = Some(node.inputs[0].clone());
+				let reconnect_from_input_index = if node.name == "Layer" { 7 } else { 0 };
+				if matches!(&node.inputs.get(reconnect_from_input_index), Some(NodeInput::Node { .. })) {
+					reconnect_to_input = Some(node.inputs[reconnect_from_input_index].clone());
 				}
 			}
 		}
@@ -385,10 +387,10 @@ impl NodeGraphMessageHandler {
 
 					// Use the first input node as the new input if deleting node's first input is a node,
 					// and the current node uses its primary output too
-					if let Some(input_node) = &first_input_node {
+					if let Some(reconnect_to_input) = &reconnect_to_input {
 						if *output_index == 0 {
 							refers_to_output_node = true;
-							*input = input_node.clone()
+							*input = reconnect_to_input.clone()
 						}
 					}
 
@@ -481,7 +483,6 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 					error!("Failed to find actual index of connector index {input_node_connector_index} on node {input_node:#?}");
 					return;
 				};
-				document.metadata.load_structure(&document.document_network);
 				responses.add(DocumentMessage::DocumentStructureChanged);
 
 				responses.add(DocumentMessage::StartTransaction);
@@ -779,7 +780,15 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 			NodeGraphMessage::SetNodeInput { node_id, input_index, input } => {
 				if let Some(network) = document.document_network.nested_network_mut(&self.network) {
 					if let Some(node) = network.nodes.get_mut(&node_id) {
-						node.inputs[input_index] = input
+						let Some(node_input) = node.inputs.get_mut(input_index) else {
+							error!("Tried to set input {input_index} to {input:?}, but the index was invalid. Node {node_id}:\n{node:#?}");
+							return;
+						};
+						let structure_changed = node_input.as_node().is_some() || input.as_node().is_some();
+						*node_input = input;
+						if structure_changed {
+							document.metadata.load_structure(&document.document_network);
+						}
 					}
 				}
 			}
@@ -928,7 +937,13 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 	}
 
 	fn actions(&self) -> ActionList {
-		if self.has_selection {
+		unimplemented!("Must use `actions_with_graph_open` instead (unless we change every implementation of the MessageHandler trait).")
+	}
+}
+
+impl NodeGraphMessageHandler {
+	pub fn actions_with_node_graph_open(&self, graph_open: bool) -> ActionList {
+		if self.has_selection && graph_open {
 			actions!(NodeGraphMessageDiscriminant; DeleteSelectedNodes, Cut, Copy, DuplicateSelectedNodes, ToggleHidden)
 		} else {
 			actions!(NodeGraphMessageDiscriminant;)

editor/src/messages/portfolio/document/utility_types/transformation.rs

@@ -375,7 +375,7 @@ impl<'a> Selected<'a> {
 			let transformation = pivot * delta * pivot.inverse();
 
 			// TODO: Cache the result of `shallowest_unique_layers` to avoid this heavy computation every frame of movement, see https://github.com/GraphiteEditor/Graphite/pull/481
-			for layer_ancestors in self.document.metadata.shallowest_unique_layers(self.selected.iter()) {
+			for layer_ancestors in self.document.metadata.shallowest_unique_layers(self.selected.iter().copied()) {
 				let layer = *layer_ancestors.last().unwrap();
 				let parent = layer.parent(&self.document.metadata);
 				if *self.tool_type == ToolType::Select {

editor/src/messages/portfolio/portfolio_message_handler.rs

@@ -6,6 +6,7 @@ use crate::messages::frontend::utility_types::FrontendDocumentDetails;
 use crate::messages::input_mapper::utility_types::macros::action_keys;
 use crate::messages::layout::utility_types::widget_prelude::*;
 use crate::messages::portfolio::document::utility_types::clipboards::{Clipboard, CopyBufferEntry, INTERNAL_CLIPBOARD_COUNT};
+use crate::messages::portfolio::document::DocumentInputs;
 use crate::messages::prelude::*;
 use crate::messages::tool::utility_types::{HintData, HintGroup};
 use crate::node_graph_executor::NodeGraphExecutor;
@@ -45,7 +46,14 @@ impl MessageHandler<PortfolioMessage, (&InputPreprocessorMessageHandler, &Prefer
 			PortfolioMessage::Document(message) => {
 				if let Some(document_id) = self.active_document_id {
 					if let Some(document) = self.documents.get_mut(&document_id) {
-						document.process_message(message, responses, (document_id, ipp, &self.persistent_data, preferences, &mut self.executor))
+						let document_inputs = DocumentInputs {
+							document_id,
+							ipp,
+							persistent_data: &self.persistent_data,
+							preferences,
+							executor: &mut self.executor,
+						};
+						document.process_message(message, responses, document_inputs)
 					}
 				}
 			}
@@ -54,7 +62,14 @@ impl MessageHandler<PortfolioMessage, (&InputPreprocessorMessageHandler, &Prefer
 			#[remain::unsorted]
 			PortfolioMessage::DocumentPassMessage { document_id, message } => {
 				if let Some(document) = self.documents.get_mut(&document_id) {
-					document.process_message(message, responses, (document_id, ipp, &self.persistent_data, preferences, &mut self.executor))
+					let document_inputs = DocumentInputs {
+						document_id,
+						ipp,
+						persistent_data: &self.persistent_data,
+						preferences,
+						executor: &mut self.executor,
+					};
+					document.process_message(message, responses, document_inputs)
 				}
 			}
 			PortfolioMessage::AutoSaveActiveDocument => {
@@ -562,7 +577,7 @@ impl MessageHandler<PortfolioMessage, (&InputPreprocessorMessageHandler, &Prefer
 				);
 				common.extend(select);
 			}
-			common.extend(document.actions());
+			common.extend(document.actions_with_graph_open(self.graph_view_overlay_open));
 		}
 
 		common

editor/src/messages/tool/tool_messages/select_tool.rs

@@ -301,7 +301,7 @@ impl SelectToolData {
 		self.not_duplicated_layers = Some(self.layers_dragging.clone());
 
 		// Duplicate each previously selected layer and select the new ones.
-		for layer_ancestors in document.metadata().shallowest_unique_layers(self.layers_dragging.iter()) {
+		for layer_ancestors in document.metadata().shallowest_unique_layers(self.layers_dragging.iter().copied()) {
 			let layer = layer_ancestors.last().unwrap();
 			// Moves the original back to its starting position.
 			responses.add_front(GraphOperationMessage::TransformChange {
@@ -352,14 +352,14 @@ impl SelectToolData {
 		responses.add(DocumentMessage::DeselectAllLayers);
 
 		// Delete the duplicated layers
-		for layer_ancestors in document.metadata().shallowest_unique_layers(self.layers_dragging.iter()) {
+		for layer_ancestors in document.metadata().shallowest_unique_layers(self.layers_dragging.iter().copied()) {
 			responses.add(GraphOperationMessage::DeleteLayer {
 				id: layer_ancestors.last().unwrap().to_node(),
 			});
 		}
 
 		// Move the original to under the mouse
-		for layer_ancestors in document.metadata().shallowest_unique_layers(originals.iter()) {
+		for layer_ancestors in document.metadata().shallowest_unique_layers(originals.iter().copied()) {
 			responses.add_front(GraphOperationMessage::TransformChange {
 				layer: layer_ancestors.last().unwrap().to_path(),
 				transform: DAffine2::from_translation(self.drag_current - self.drag_start),
@@ -581,7 +581,7 @@ impl Fsm for SelectToolFsmState {
 
 				let closest_move = tool_data.snap_manager.snap_layers(responses, document, snap, mouse_delta);
 				// TODO: Cache the result of `shallowest_unique_layers` to avoid this heavy computation every frame of movement, see https://github.com/GraphiteEditor/Graphite/pull/481
-				for layer_ancestors in document.metadata().shallowest_unique_layers(tool_data.layers_dragging.iter()) {
+				for layer_ancestors in document.metadata().shallowest_unique_layers(tool_data.layers_dragging.iter().copied()) {
 					responses.add_front(GraphOperationMessage::TransformChange {
 						layer: layer_ancestors.last().unwrap().to_path(),
 						transform: DAffine2::from_translation(mouse_delta + closest_move),