Improve expose input and remove cancel/commit transaction messages

Author: adamgerhant

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

@@ -182,8 +182,6 @@ pub enum DocumentMessage {
 	AddTransaction,
 	StartTransaction,
 	EndTransaction,
-	CommitTransaction,
-	CancelTransaction,
 	AbortTransaction,
 	RepeatedAbortTransaction {
 		undo_count: usize,

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

@@ -1292,46 +1292,28 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				responses.add_front(NodeGraphMessage::RunDocumentGraph);
 			}
 			DocumentMessage::AddTransaction => {
-				// Reverse order since they are added to the front
-				responses.add_front(DocumentMessage::CommitTransaction);
-				responses.add_front(DocumentMessage::StartTransaction);
+				self.start_transaction(responses);
+				self.commit_transaction(responses);
 			}
 			// Note: A transaction should never be started in a scope that mutates the network interface, since it will only be run after that scope ends.
 			DocumentMessage::StartTransaction => {
-				self.network_interface.start_transaction();
-				let network_interface_clone = self.network_interface.clone();
-				self.document_undo_history.push_back(network_interface_clone);
-				if self.document_undo_history.len() > crate::consts::MAX_UNDO_HISTORY_LEN {
-					self.document_undo_history.pop_front();
-				}
-				// Push the UpdateOpenDocumentsList message to the bus in order to update the save status of the open documents
-				responses.add(PortfolioMessage::UpdateOpenDocumentsList);
+				self.start_transaction(responses);
 			}
 			// Commits the transaction if the network was mutated since the transaction started, otherwise it cancels the transaction
 			DocumentMessage::EndTransaction => match self.network_interface.transaction_status() {
 				TransactionStatus::Started => {
-					responses.add_front(DocumentMessage::CancelTransaction);
+					self.network_interface.finish_transaction();
+					self.document_undo_history.pop_back();
 				}
 				TransactionStatus::Modified => {
-					responses.add_front(DocumentMessage::CommitTransaction);
+					self.commit_transaction(responses);
 				}
 				TransactionStatus::Finished => {}
 			},
-			DocumentMessage::CancelTransaction => {
-				self.network_interface.finish_transaction();
-				self.document_undo_history.pop_back();
-			}
-			DocumentMessage::CommitTransaction => {
-				if self.network_interface.transaction_status() == TransactionStatus::Finished {
-					return;
-				}
-				self.network_interface.finish_transaction();
-				self.document_redo_history.clear();
-				responses.add(PortfolioMessage::UpdateOpenDocumentsList);
-			}
 			DocumentMessage::AbortTransaction => match self.network_interface.transaction_status() {
 				TransactionStatus::Started => {
-					responses.add_front(DocumentMessage::CancelTransaction);
+					self.network_interface.finish_transaction();
+					self.document_undo_history.pop_back();
 				}
 				TransactionStatus::Modified => {
 					responses.add(DocumentMessage::RepeatedAbortTransaction { undo_count: 1 });
@@ -1829,6 +1811,26 @@ impl DocumentMessageHandler {
 		val.unwrap()
 	}
 
+	pub fn start_transaction(&mut self, responses: &mut VecDeque<Message>) {
+		self.network_interface.start_transaction();
+		let network_interface_clone = self.network_interface.clone();
+		self.document_undo_history.push_back(network_interface_clone);
+		if self.document_undo_history.len() > crate::consts::MAX_UNDO_HISTORY_LEN {
+			self.document_undo_history.pop_front();
+		}
+		// Push the UpdateOpenDocumentsList message to the bus in order to update the save status of the open documents
+		responses.add(PortfolioMessage::UpdateOpenDocumentsList);
+	}
+
+	pub fn commit_transaction(&mut self, responses: &mut VecDeque<Message>) {
+		if self.network_interface.transaction_status() == TransactionStatus::Finished {
+			return;
+		}
+		self.network_interface.finish_transaction();
+		self.document_redo_history.clear();
+		responses.add(PortfolioMessage::UpdateOpenDocumentsList);
+	}
+
 	pub fn deserialize_document(serialized_content: &str) -> Result<Self, EditorError> {
 		let document_message_handler = serde_json::from_str::<DocumentMessageHandler>(serialized_content)
 			.or_else(|e| {

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

@@ -63,9 +63,9 @@ pub enum NodeGraphMessage {
 	EnterNestedNetwork,
 	DuplicateSelectedNodes,
 	ExposeInput {
-		input_connector: InputConnector,
-		set_to_exposed: bool,
-		start_transaction: bool,
+		node_id: NodeId,
+		input_index: usize,
+		exposed: bool,
 	},
 	ExposeEncapsulatingPrimaryInput {
 		exposed: bool,

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

@@ -403,15 +403,7 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
 					responses.add(DocumentMessage::EnterNestedNetwork { node_id });
 				}
 			}
-			NodeGraphMessage::ExposeInput {
-				input_connector,
-				set_to_exposed,
-				start_transaction,
-			} => {
-				let InputConnector::Node { node_id, input_index } = input_connector else {
-					log::error!("Cannot expose/hide export");
-					return;
-				};
+			NodeGraphMessage::ExposeInput { node_id, input_index, exposed } => {
 				let Some(node) = network_interface.document_node(&node_id, selection_network_path) else {
 					log::error!("Could not find node {node_id} in NodeGraphMessage::ExposeInput");
 					return;
@@ -421,38 +413,19 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
 					return;
 				};
 
-				// If we're un-exposing an input that is not a value, then disconnect it. This will convert it to a value input,
-				// so we can come back to handle this message again to set the exposed value in the second run-through.
-				if !set_to_exposed && node_input.as_value().is_none() {
-					// Reversed order because we are pushing front
-					responses.add_front(NodeGraphMessage::ExposeInput {
-						input_connector,
-						set_to_exposed,
-						start_transaction: false,
-					});
-					responses.add_front(NodeGraphMessage::DisconnectInput { input_connector });
-					responses.add_front(DocumentMessage::StartTransaction);
-					return;
-				}
-
-				// Add a history step, but only do so if we didn't already start a transaction in the first run-through of this message in the above code
-				if start_transaction {
-					responses.add_front(DocumentMessage::StartTransaction);
-				}
+				responses.add(DocumentMessage::AddTransaction);
 
-				// If this node's input is a value type, we set its chosen exposed state
+				let new_exposed = exposed;
 				if let NodeInput::Value { exposed, .. } = &mut node_input {
-					*exposed = set_to_exposed;
+					*exposed = new_exposed;
 				}
+
 				responses.add(NodeGraphMessage::SetInput {
 					input_connector: InputConnector::node(node_id, input_index),
 					input: node_input,
 				});
 
-				// Finish the history step
-				responses.add(DocumentMessage::CommitTransaction);
-
-				// Update the graph UI and re-render
+				// Update the graph UI and re-render if the graph is open, if the graph is closed then open the graph and zoom in on the input
 				if graph_view_overlay_open {
 					responses.add(PropertiesPanelMessage::Refresh);
 					responses.add(NodeGraphMessage::SendGraph);

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

@@ -47,7 +47,7 @@ pub fn commit_value<T>(_: &T) -> Message {
 	DocumentMessage::AddTransaction.into()
 }
 
-pub fn expose_widget(node_id: NodeId, index: usize, data_type: FrontendGraphDataType, exposed: bool) -> WidgetInstance {
+pub fn expose_widget(node_id: NodeId, input_index: usize, data_type: FrontendGraphDataType, exposed: bool) -> WidgetInstance {
 	ParameterExposeButton::new()
 		.exposed(exposed)
 		.data_type(data_type)
@@ -58,9 +58,9 @@ pub fn expose_widget(node_id: NodeId, index: usize, data_type: FrontendGraphData
 		})
 		.on_update(move |_parameter| Message::Batched {
 			messages: Box::new([NodeGraphMessage::ExposeInput {
-				input_connector: InputConnector::node(node_id, index),
-				set_to_exposed: !exposed,
-				start_transaction: true,
+				node_id,
+				input_index,
+				exposed: !exposed,
 			}
 			.into()]),
 		})

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

@@ -3800,8 +3800,8 @@ impl NodeNetworkInterface {
 			return;
 		};
 
-		// When changing a NodeInput::Node to a NodeInput::Node, the input should first be disconnected to ensure proper side effects
-		if (matches!(previous_input, NodeInput::Node { .. }) && matches!(new_input, NodeInput::Node { .. })) {
+		// When changing a NodeInput::Node to another input, the input should first be disconnected to ensure proper side effects
+		if matches!(previous_input, NodeInput::Node { .. }) {
 			self.disconnect_input(input_connector, network_path);
 			self.set_input(input_connector, new_input, network_path);
 			return;
@@ -3856,7 +3856,7 @@ impl NodeNetworkInterface {
 			return;
 		}
 
-		// It is necessary to ensure the grpah is acyclic before calling `self.position` as it sometimes crashes with cyclic graphs #3227
+		// It is necessary to ensure the graph is acyclic before calling `self.position` as it sometimes crashes with cyclic graphs #3227
 		let previous_metadata = match &previous_input {
 			NodeInput::Node { node_id, .. } => self.position(node_id, network_path).map(|position| (*node_id, position)),
 			_ => None,

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

@@ -214,7 +214,6 @@ impl ToolTransition for FreehandTool {
 #[derive(Clone, Debug, Default)]
 struct FreehandToolData {
 	end_point: Option<(DVec2, PointId)>,
-	dragged: bool,
 	weight: f64,
 	layer: Option<LayerNodeIdentifier>,
 }
@@ -250,7 +249,6 @@ impl Fsm for FreehandToolFsmState {
 			(FreehandToolFsmState::Ready, FreehandToolMessage::DragStart { append_to_selected }) => {
 				responses.add(DocumentMessage::StartTransaction);
 
-				tool_data.dragged = false;
 				tool_data.end_point = None;
 				tool_data.weight = tool_options.line_weight;
 
@@ -307,11 +305,7 @@ impl Fsm for FreehandToolFsmState {
 				FreehandToolFsmState::Drawing
 			}
 			(FreehandToolFsmState::Drawing, FreehandToolMessage::DragStop) => {
-				if tool_data.dragged {
-					responses.add(DocumentMessage::CommitTransaction);
-				} else {
-					responses.add(DocumentMessage::EndTransaction);
-				}
+				responses.add(DocumentMessage::EndTransaction);
 
 				tool_data.end_point = None;
 				tool_data.layer = None;
@@ -380,7 +374,6 @@ fn extend_path_with_next_segment(tool_data: &mut FreehandToolData, position: DVe
 		});
 	}
 
-	tool_data.dragged = true;
 	tool_data.end_point = Some((position, id));
 }