Final code review pass

Author: Keavon

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

@@ -190,8 +190,6 @@ impl MessageHandler<GraphOperationMessage, GraphOperationMessageData<'_>> for Gr
 				insert_index,
 				alias,
 			} => {
-				//trace!("Inserting new layer {id} as a child of {parent:?} at index {insert_index}");
-
 				let mut modify_inputs = ModifyInputsContext::new(document_network, document_metadata, node_graph, responses);
 
 				if let Some(layer) = modify_inputs.create_layer_with_insert_index(id, insert_index, parent) {
@@ -232,7 +230,7 @@ impl MessageHandler<GraphOperationMessage, GraphOperationMessageData<'_>> for Gr
 
 					modify_inputs.responses.add(NodeGraphMessage::RunDocumentGraph);
 				} else {
-					error!("Create failed");
+					error!("Creating new custom layer failed");
 				}
 
 				load_network_structure(document_network, document_metadata, selected_nodes, collapsed);

editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs

@@ -135,10 +135,8 @@ impl<'a> ModifyInputsContext<'a> {
 	/// ->  Layer3  input_index: 3
 	///             if skip_layer_nodes == 3, return (Layer3, None, 0)
 	pub fn get_post_node_with_index(network: &NodeNetwork, mut post_node_id: NodeId, insert_index: usize) -> (NodeId, Option<NodeId>, usize) {
-		let mut post_node_input_index = 1; // Assume post node is a layer type.
-		if post_node_id == NodeId(0) {
-			post_node_input_index = 0; // Output node input index.
-		}
+		let mut post_node_input_index = if post_node_id == NodeId(0) { 0 } else { 1 };
+
 		// Skip layers based on skip_layer_nodes, which inserts the new layer at a certain index of the layer stack.
 		let mut current_index = 0;
 		loop {
@@ -159,13 +157,17 @@ impl<'a> ModifyInputsContext<'a> {
 				if next_node_in_stack.is_layer {
 					current_index += 1;
 				}
+
 				post_node_id = next_node_in_stack_id;
-				post_node_input_index = 0; //Input as a sibling to the Layer node above
+
+				// Input as a sibling to the Layer node above
+				post_node_input_index = 0;
 			} else {
 				log::error!("Error creating layer: insert_index out of bounds");
 				break;
 			};
 		}
+
 		// Move post_node to the end of the non layer chain that feeds into post_node, such that pre_node is the layer node at index 1 + insert_index
 		let mut post_node = network.nodes.get(&post_node_id).expect("Post node should always exist");
 		let mut pre_node_id = post_node
@@ -195,7 +197,7 @@ impl<'a> ModifyInputsContext<'a> {
 	pub fn create_layer(&mut self, new_id: NodeId, output_node_id: NodeId, skip_layer_nodes: usize) -> Option<NodeId> {
 		assert!(!self.document_network.nodes.contains_key(&new_id), "Creating already existing layer");
 
-		// Get the node which the new layer will output to (post node). First check if the output_node_id is the Output node, and set the output_node_id to the top most artboard,
+		// Get the node which the new layer will output to (post node). First check if the output_node_id is the Output node, and set the output_node_id to the top-most artboard,
 		// if there is one. Then skip layers based on skip_layer_nodes from the post_node.
 		// TODO: Smarter placement of layers into artboards https://github.com/GraphiteEditor/Graphite/issues/1507
 
@@ -210,25 +212,21 @@ impl<'a> ModifyInputsContext<'a> {
 			}
 		}
 		let (post_node_id, pre_node_id, post_node_input_index) = Self::get_post_node_with_index(self.document_network, post_node_id, skip_layer_nodes);
+		let new_layer_node = resolve_document_node_type("Merge").expect("Merge node").default_document_node();
 		if let Some(pre_node_id) = pre_node_id {
-			let new_layer_node = resolve_document_node_type("Merge").expect("Merge node").default_document_node();
 			self.insert_between(
 				new_id,
 				new_layer_node,
 				NodeInput::node(pre_node_id, 0),
-				0, // Pre_node is a sibling so it connects to the first input.
+				0, // pre_node is a sibling so it connects to the first input
 				post_node_id,
 				NodeInput::node(new_id, 0),
 				post_node_input_index,
 				IVec2::new(0, 3),
 			);
 		} else {
-			let new_layer_node = resolve_document_node_type("Merge").expect("Merge node").default_document_node();
-			if post_node_input_index == 1 {
-				self.insert_node_before(new_id, post_node_id, post_node_input_index, new_layer_node, IVec2::new(-8, 3));
-			} else {
-				self.insert_node_before(new_id, post_node_id, post_node_input_index, new_layer_node, IVec2::new(0, 3));
-			}
+			let offset = if post_node_input_index == 1 { IVec2::new(-8, 3) } else { IVec2::new(0, 3) };
+			self.insert_node_before(new_id, post_node_id, post_node_input_index, new_layer_node, offset);
 		}
 
 		Some(new_id)
@@ -248,7 +246,6 @@ impl<'a> ModifyInputsContext<'a> {
 	/// Creates an artboard that outputs to the output node.
 	pub fn create_artboard(&mut self, new_id: NodeId, artboard: Artboard) -> Option<NodeId> {
 		let output_node_id = self.document_network.original_outputs()[0].node_id;
-		let mut shift = IVec2::new(0, 3);
 
 		let artboard_node = resolve_document_node_type("Artboard").expect("Node").to_document_node_default_inputs(
 			[
@@ -277,18 +274,17 @@ impl<'a> ModifyInputsContext<'a> {
 				output_node_id,
 				NodeInput::node(new_id, 0),
 				0,
-				shift,
+				IVec2::new(0, 3),
 			)
 		} else {
-			shift = IVec2::new(-8, 3);
-			self.insert_node_before(new_id, output_node_id, 0, artboard_node, shift)
+			self.insert_node_before(new_id, output_node_id, 0, artboard_node, IVec2::new(-8, 3))
 		};
 
 		if let Some(new_id) = created_node_id {
 			let new_child = LayerNodeIdentifier::new_unchecked(new_id);
 			LayerNodeIdentifier::ROOT.push_front_child(self.document_metadata, new_child);
 		}
-		//self.responses.add(NodeGraphMessage::RunDocumentGraph);
+
 		created_node_id
 	}
 	pub fn insert_vector_data(&mut self, subpaths: Vec<Subpath<ManipulatorGroupId>>, layer: NodeId) {
@@ -357,6 +353,7 @@ impl<'a> ModifyInputsContext<'a> {
 		if shift_self {
 			shift_nodes.insert(node_id);
 		}
+
 		let mut stack = vec![node_id];
 		while let Some(node_id) = stack.pop() {
 			let Some(node) = self.document_network.nodes.get(&node_id) else { continue };
@@ -618,18 +615,20 @@ impl<'a> ModifyInputsContext<'a> {
 
 	pub fn resize_artboard(&mut self, location: IVec2, dimensions: IVec2) {
 		self.modify_inputs("Artboard", false, |inputs, _node_id, _metadata| {
-			let mut new_dimensions = dimensions;
-			let mut new_location = location;
+			let mut dimensions = dimensions;
+			let mut location = location;
+
 			if dimensions.x < 0 {
-				new_dimensions.x = -dimensions.x;
-				new_location.x += dimensions.x;
+				dimensions.x *= -1;
+				location.x += dimensions.x;
 			}
 			if dimensions.y < 0 {
-				new_dimensions.y = -dimensions.y;
-				new_location.y += dimensions.y;
+				dimensions.y *= -1;
+				location.y += dimensions.y;
 			}
-			inputs[2] = NodeInput::value(TaggedValue::IVec2(new_location), false);
-			inputs[3] = NodeInput::value(TaggedValue::IVec2(new_dimensions), false);
+
+			inputs[2] = NodeInput::value(TaggedValue::IVec2(location), false);
+			inputs[3] = NodeInput::value(TaggedValue::IVec2(dimensions), false);
 		});
 	}
 }

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

@@ -150,20 +150,25 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 						continue;
 					};
 
-					let node = document_network.nodes.get(&node_id).expect("node should always exist");
+					let Some(node) = document_network.nodes.get(&node_id) else {
+						continue;
+					};
 					let child_id = node.inputs.get(1).and_then(|input| if let NodeInput::Node { node_id, .. } = input { Some(node_id) } else { None });
 					let Some(child_id) = child_id else {
 						continue;
 					};
 
 					let outward_links = document_network.collect_outwards_links();
-					for (_node, upstream_id) in document_network.upstream_flow_back_from_nodes(vec![*child_id], graph_craft::document::FlowType::UpstreamFlow) {
+
+					for (_, upstream_id) in document_network.upstream_flow_back_from_nodes(vec![*child_id], graph_craft::document::FlowType::UpstreamFlow) {
 						// TODO: move into a document_network function .is_sole_dependent. This function does a downstream traversal starting from the current node,
-						// and only traverses for nodes that are not in the delete_nodes set. If all downstream nodes converge to some node in the delete_nodes set,
-						// then it is a sole dependent. If the output node is eventually reached, then it is not a sole dependent. This means disconnected branches
-						// that do not feed into the delete_nodes set or the output node will be deleted.
+						// TODO: and only traverses for nodes that are not in the delete_nodes set. If all downstream nodes converge to some node in the delete_nodes set,
+						// TODO: then it is a sole dependent. If the output node is eventually reached, then it is not a sole dependent. This means disconnected branches
+						// TODO: that do not feed into the delete_nodes set or the output node will be deleted.
+
 						let mut stack = vec![upstream_id];
 						let mut can_delete = true;
+
 						// TODO: Add iteration limit to force break in case of infinite while loop
 						while let Some(current_node) = stack.pop() {
 							if let Some(downstream_nodes) = outward_links.get(&current_node) {
@@ -176,7 +181,9 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 									// Continue traversing over the downstream sibling, which happens if the current node is a sibling to a node in node_ids
 									else {
 										for deleted_node_id in &node_ids {
-											let output_node: &DocumentNode = document_network.nodes.get(&deleted_node_id).expect("node should always exist");
+											let Some(output_node) = document_network.nodes.get(&deleted_node_id) else {
+												continue;
+											};
 											let Some(input) = output_node.inputs.get(0) else {
 												continue;
 											};
@@ -191,26 +198,24 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 								}
 							}
 						}
+
 						if can_delete {
 							delete_nodes.insert(upstream_id);
 						}
 					}
 				}
+
 				for delete_node_id in delete_nodes {
-					let delete_node = document_network.nodes.get(&delete_node_id).expect("node should always exist");
+					let Some(delete_node) = document_network.nodes.get(&delete_node_id) else {
+						continue;
+					};
+
 					if delete_node.is_layer {
 						// Delete node from document metadata
 						let layer_node = LayerNodeIdentifier::new(delete_node_id, document_network);
 						layer_node.delete(document_metadata);
-						// Shift the position of the upstream sibling nodes for a deleted layer node. This ends up causing more positioning issues than it solves.
-						// let shift = IVec2::new(0, -3);
-						// let node_ids = document_network.upstream_flow_back_from_nodes(vec![delete_node_id], false).map(|(_, id)| id).collect::<Vec<_>>();
-
-						// for node_id in node_ids {
-						// 	let Some(node) = document_network.nodes.get_mut(&node_id) else { continue };
-						// 	node.metadata.position += shift;
-						// }
 					}
+
 					self.remove_node(document_network, selected_nodes, delete_node_id, responses, reconnect);
 				}
 
@@ -220,20 +225,18 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 						responses.add(NodeGraphMessage::RunDocumentGraph);
 					}
 				}
-				// No need to call this since the metadata is already updated
-				// load_network_structure(document_network, document_metadata, selected_nodes, collapsed);
-			}
 
-			// Deletes selected_nodes. If reconnect is true, then all children nodes (secondary input) of the selected nodes are deleted and the siblings(primary input/output) are reconnected.
-			// If reconnect is false, then only the selected nodes are deleted and not reconnected.
+				// There is no need to call `load_network_structure()` since the metadata is already updated
+			}
+			// Deletes selected_nodes. If `reconnect` is true, then all children nodes (secondary input) of the selected nodes are deleted and the siblings (primary input/output) are reconnected.
+			// If `reconnect` is false, then only the selected nodes are deleted and not reconnected.
 			NodeGraphMessage::DeleteSelectedNodes { reconnect } => {
 				responses.add(DocumentMessage::StartTransaction);
 				responses.add(NodeGraphMessage::DeleteNodes {
 					node_ids: selected_nodes.selected_nodes().copied().collect(),
 					reconnect,
 				});
 			}
-
 			NodeGraphMessage::DisconnectNodes { node_id, input_index } => {
 				let Some(network) = document_network.nested_network(&self.network) else {
 					warn!("No network");
@@ -247,14 +250,16 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 					warn!("Node {} not in library", node.name);
 					return;
 				};
-
 				let Some((input_index, existing_input)) = node.inputs.iter().enumerate().filter(|(_, input)| input.is_exposed()).nth(input_index) else {
 					return;
 				};
+
 				let mut input = node_type.inputs[input_index].default.clone();
 				if let NodeInput::Value { exposed, .. } = &mut input {
 					*exposed = existing_input.is_exposed();
 				}
+
+				responses.add(DocumentMessage::StartTransaction);
 				responses.add(NodeGraphMessage::SetNodeInput { node_id, input_index, input });
 
 				if network.connected_to_output(node_id) {
@@ -343,7 +348,6 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 				}
 
 				responses.add(NodeGraphMessage::SetNodeInput { node_id, input_index, input });
-
 				responses.add(NodeGraphMessage::EnforceLayerHasNoMultiParams { node_id });
 				responses.add(PropertiesPanelMessage::Refresh);
 				responses.add(NodeGraphMessage::SendGraph);
@@ -382,6 +386,7 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 					error!("Failed to find input index {insert_node_input_index} on node {insert_node_id:#?}");
 					return;
 				};
+
 				responses.add(DocumentMessage::StartTransaction);
 
 				let post_input = NodeInput::node(insert_node_id, insert_node_output_index);
@@ -414,6 +419,7 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 						node.metadata.position += IVec2::new(displacement_x, displacement_y)
 					}
 				}
+
 				// Since document structure doesn't change, just update the nodes
 				if graph_view_overlay_open {
 					let links = Self::collect_links(network);
@@ -662,19 +668,20 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 				self.update_selection_action_buttons(document_network, document_metadata, selected_nodes, responses);
 			}
 			NodeGraphMessage::ToggleSelectedLayers => {
-				if let Some(network) = document_network.nested_network_mut(&self.network) {
-					for node_id in selected_nodes.selected_nodes() {
-						if let Some(node) = network.nodes.get_mut(&node_id) {
-							if node.has_primary_output {
-								responses.add(NodeGraphMessage::SetToNodeOrLayer {
-									node_id: *node_id,
-									is_layer: !node.is_layer,
-								});
-							}
-							if network.connected_to_output(*node_id) {
-								responses.add(NodeGraphMessage::RunDocumentGraph);
-							}
-						}
+				let Some(network) = document_network.nested_network_mut(&self.network) else { return };
+
+				for node_id in selected_nodes.selected_nodes() {
+					let Some(node) = network.nodes.get_mut(&node_id) else { continue };
+
+					if node.has_primary_output {
+						responses.add(NodeGraphMessage::SetToNodeOrLayer {
+							node_id: *node_id,
+							is_layer: !node.is_layer,
+						});
+					}
+
+					if network.connected_to_output(*node_id) {
+						responses.add(NodeGraphMessage::RunDocumentGraph);
 					}
 				}
 			}
@@ -683,13 +690,13 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphHandlerData<'a>> for NodeGrap
 					return;
 				}
 
-				if let Some(network) = document_network.nested_network_mut(&self.network) {
-					if let Some(node) = network.nodes.get_mut(&node_id) {
-						node.is_layer = is_layer;
-					}
-					responses.add(NodeGraphMessage::RunDocumentGraph);
-					responses.add(DocumentMessage::DocumentStructureChanged);
+				let Some(network) = document_network.nested_network_mut(&self.network) else { return };
+
+				if let Some(node) = network.nodes.get_mut(&node_id) {
+					node.is_layer = is_layer;
 				}
+				responses.add(NodeGraphMessage::RunDocumentGraph);
+				responses.add(DocumentMessage::DocumentStructureChanged);
 			}
 			NodeGraphMessage::SetName { node_id, name } => {
 				responses.add(DocumentMessage::StartTransaction);
@@ -1136,27 +1143,17 @@ impl NodeGraphMessageHandler {
 	}
 
 	pub fn eligible_to_be_layer(&self, document_network: &NodeNetwork, node_id: NodeId) -> bool {
-		let Some(network) = document_network.nested_network(&self.network) else {
-			return false;
-		};
-		let Some(node) = network.nodes.get(&node_id) else {
-			return false;
-		};
+		let Some(network) = document_network.nested_network(&self.network) else { return false };
+		let Some(node) = network.nodes.get(&node_id) else { return false };
+		let Some(definition) = resolve_document_node_type(&node.name) else { return false };
 
-		let exposed_value_count = node
-			.inputs
-			.iter()
-			.filter(|input| if let NodeInput::Value { tagged_value: _, exposed } = input { *exposed } else { false })
-			.count();
+		let exposed_value_count = node.inputs.iter().filter(|input| if let NodeInput::Value { exposed, .. } = input { *exposed } else { false }).count();
 		let node_input_count = node.inputs.iter().filter(|input| if let NodeInput::Node { .. } = input { true } else { false }).count();
 		let input_count = node_input_count + exposed_value_count;
-		let Some(definition) = resolve_document_node_type(&node.name) else { return false };
 		let output_count = definition.outputs.len();
+
 		// TODO: Eventually allow nodes at the bottom of a stack to be layers, where `input_count` is 0
-		if !node.has_primary_output || output_count != 1 || input_count == 0 || input_count > 2 {
-			return false;
-		}
-		true
+		node.has_primary_output && output_count == 1 && (input_count == 1 || input_count == 2)
 	}
 
 	fn untitled_layer_label(node: &DocumentNode) -> String {

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

@@ -2422,9 +2422,8 @@ pub fn artboard_properties(document_node: &DocumentNode, node_id: NodeId, _conte
 	let location = vec2_widget(document_node, node_id, 2, "Location", "X", "Y", " px", None, add_blank_assist);
 	let dimensions = vec2_widget(document_node, node_id, 3, "Dimensions", "W", "H", " px", None, add_blank_assist);
 	let background = color_widget(document_node, node_id, 4, "Background", ColorButton::default().allow_none(false), true);
-	let clip = LayoutGroup::Row {
-		widgets: bool_widget(document_node, node_id, 5, "Clip", true),
-	};
+	let clip = bool_widget(document_node, node_id, 5, "Clip", true);
+	let clip = LayoutGroup::Row { widgets: clip };
 	vec![location, dimensions, background, clip]
 }