Simplify history transaction system messages

editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs

@@ -256,29 +256,6 @@ impl PreferencesDialogMessageHandler {
 					.widget_instance(),
 			];
 
-			let checkbox_id = CheckboxId::new();
-			let vector_mesh_description = "
-			Allow the Pen tool to produce branching geometry, where more than two segments may be connected to one anchor point.\n\
-			\n\
-			Currently, vector meshes do not properly render strokes (branching joins) and fills (multiple regions).
-			"
-			.trim();
-			let vector_meshes = vec![
-				Separator::new(SeparatorType::Unrelated).widget_instance(),
-				Separator::new(SeparatorType::Unrelated).widget_instance(),
-				CheckboxInput::new(preferences.vector_meshes)
-					.tooltip_label("Vector Meshes")
-					.tooltip_description(vector_mesh_description)
-					.on_update(|checkbox_input: &CheckboxInput| PreferencesMessage::VectorMeshes { enabled: checkbox_input.checked }.into())
-					.for_label(checkbox_id)
-					.widget_instance(),
-				TextLabel::new("Vector Meshes")
-					.tooltip_label("Vector Meshes")
-					.tooltip_description(vector_mesh_description)
-					.for_checkbox(checkbox_id)
-					.widget_instance(),
-			];
-
 			let checkbox_id = CheckboxId::new();
 			let brush_tool_description = "
 			Enable the Brush tool to support basic raster-based layer painting.\n\
@@ -304,7 +281,7 @@ impl PreferencesDialogMessageHandler {
 					.widget_instance(),
 			];
 
-			rows.extend_from_slice(&[header, node_graph_wires_label, graph_wire_style, use_vello, vector_meshes, brush_tool]);
+			rows.extend_from_slice(&[header, node_graph_wires_label, graph_wire_style, use_vello, brush_tool]);
 		}
 
 		Layout(rows.into_iter().map(|r| LayoutGroup::Row { widgets: r }).collect())

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

@@ -49,8 +49,6 @@ pub enum DocumentMessage {
 	},
 	DeleteSelectedLayers,
 	DeselectAllLayers,
-	DocumentHistoryBackward,
-	DocumentHistoryForward,
 	DocumentStructureChanged,
 	DrawArtboardOverlays {
 		context: OverlayContext,
@@ -110,7 +108,6 @@ pub enum DocumentMessage {
 		mouse: Option<(f64, f64)>,
 		parent_and_insert_index: Option<(LayerNodeIdentifier, usize)>,
 	},
-	Redo,
 	RenameDocument {
 		new_name: String,
 	},
@@ -179,12 +176,23 @@ pub enum DocumentMessage {
 	SetRenderMode {
 		render_mode: RenderMode,
 	},
+	Undo,
+	Redo,
+	DocumentHistoryBackward,
+	DocumentHistoryForward,
+	// TODO: Rename to HistoryStepPush
+	/// Create a snapshot of the document at this point in time, by immediately starting and committing a transaction.
 	AddTransaction,
+	// TODO: Rename to HistoryTransactionStart
+	/// Take a snapshot of the document to an intermediate state, and then depending on what we do next, we might either commit or abort it.
 	StartTransaction,
+	// TODO: Rename to HistoryTransactionEnd
+	/// Either commit (creating a new history step) or cancel (removing the last history step, as if it never happened) the last transaction started with `StartTransaction`.
 	EndTransaction,
-	CommitTransaction,
-	CancelTransaction,
+	/// Cause the document to revert back to the state when the transaction was started. For example, the user may be dragging
+	/// something around and hits Escape to abort the drag. This jumps the document back to the point before the drag began.
 	AbortTransaction,
+	/// The same as `AbortTransaction` with one step back, but it can also be called with multiple steps back in the history of undos.
 	RepeatedAbortTransaction {
 		undo_count: usize,
 	},
@@ -208,7 +216,6 @@ pub enum DocumentMessage {
 	UpdateClipTargets {
 		clip_targets: HashSet<NodeId>,
 	},
-	Undo,
 	UngroupSelectedLayers,
 	UngroupLayer {
 		layer: LayerNodeIdentifier,

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

@@ -372,8 +372,6 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				responses.add(NodeGraphMessage::SelectedNodesSet { nodes: vec![] });
 				self.layer_range_selection_reference = None;
 			}
-			DocumentMessage::DocumentHistoryBackward => self.undo_with_history(viewport, responses),
-			DocumentMessage::DocumentHistoryForward => self.redo_with_history(viewport, responses),
 			DocumentMessage::DocumentStructureChanged => {
 				if layers_panel_open {
 					self.network_interface.load_structure();
@@ -953,15 +951,6 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				responses.add(NodeGraphMessage::SelectedNodesSet { nodes: vec![layer.to_node()] });
 				responses.add(ToolMessage::ActivateTool { tool_type: ToolType::Select });
 			}
-			DocumentMessage::Redo => {
-				if self.network_interface.transaction_status() != TransactionStatus::Finished {
-					return;
-				}
-				responses.add(SelectToolMessage::Abort);
-				responses.add(DocumentMessage::DocumentHistoryForward);
-				responses.add(ToolMessage::Redo);
-				responses.add(OverlaysMessage::Draw);
-			}
 			DocumentMessage::RenameDocument { new_name } => {
 				self.name = new_name.clone();
 
@@ -1291,63 +1280,79 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				self.render_mode = render_mode;
 				responses.add_front(NodeGraphMessage::RunDocumentGraph);
 			}
+			DocumentMessage::Undo => {
+				if self.network_interface.transaction_status() != TransactionStatus::Finished {
+					return;
+				}
+				responses.add(ToolMessage::PreUndo);
+				responses.add(DocumentMessage::DocumentHistoryBackward);
+				responses.add(OverlaysMessage::Draw);
+				responses.add(ToolMessage::Undo);
+			}
+			DocumentMessage::Redo => {
+				if self.network_interface.transaction_status() != TransactionStatus::Finished {
+					return;
+				}
+				responses.add(SelectToolMessage::Abort);
+				responses.add(DocumentMessage::DocumentHistoryForward);
+				responses.add(ToolMessage::Redo);
+				responses.add(OverlaysMessage::Draw);
+			}
+			DocumentMessage::DocumentHistoryBackward => self.undo_with_history(viewport, responses),
+			DocumentMessage::DocumentHistoryForward => self.redo_with_history(viewport, responses),
+			// Create a snapshot of the document at this point in time, by immediately starting and committing a transaction.
 			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
+			// Either commit (creating a new history step) or cancel (removing the last history step, as if it never happened) the last transaction started with `StartTransaction`.
 			DocumentMessage::EndTransaction => match self.network_interface.transaction_status() {
+				// This is used if, between the start and end of the transaction, the changes were undone by the user.
+				// For example, dragging something around and then dropping it back at its exact original position.
+				// So we cancel the transaction to return to the point before the transaction was started.
 				TransactionStatus::Started => {
-					responses.add_front(DocumentMessage::CancelTransaction);
+					self.network_interface.finish_transaction();
+					self.document_undo_history.pop_back();
 				}
+				// This is used if, between the start and end of the transaction, actual changes did occur and we want to keep them as part of a history step that the user can undo/redo.
 				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() {
+				// If we abort a transaction without any changes having been made, we simply remove the transaction as if it never occurred.
 				TransactionStatus::Started => {
-					responses.add_front(DocumentMessage::CancelTransaction);
+					self.network_interface.finish_transaction();
+					self.document_undo_history.pop_back();
 				}
+				// If we abort a transaction after changes have been made, we need to undo those changes.
 				TransactionStatus::Modified => {
 					responses.add(DocumentMessage::RepeatedAbortTransaction { undo_count: 1 });
 				}
+				// This is an erroneous state indicating that a transaction is being aborted without having ever been started.
 				TransactionStatus::Finished => {}
 			},
+			// The same as `AbortTransaction` with one step back, but it can also be called with multiple steps back in the history of undos.
 			DocumentMessage::RepeatedAbortTransaction { undo_count } => {
+				// This prevents us from aborting a transaction multiple times in a row, which would be erroneous.
 				if self.network_interface.transaction_status() == TransactionStatus::Finished {
 					return;
 				}
 
+				// Sometimes (like successive G/R/S transformations) we may need to undo multiple steps to fully abort the transaction, before we finish.
 				for _ in 0..undo_count {
 					self.undo(viewport, responses);
 				}
 
+				// Finally finish the transaction, ensuring that any future operations are not erroneously redone as part of this aborted transaction.
 				self.network_interface.finish_transaction();
+
+				// Refresh state
 				responses.add(OverlaysMessage::Draw);
 				responses.add(PortfolioMessage::UpdateOpenDocumentsList);
 			}
@@ -1419,15 +1424,6 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 			DocumentMessage::UpdateClipTargets { clip_targets } => {
 				self.network_interface.update_clip_targets(clip_targets);
 			}
-			DocumentMessage::Undo => {
-				if self.network_interface.transaction_status() != TransactionStatus::Finished {
-					return;
-				}
-				responses.add(ToolMessage::PreUndo);
-				responses.add(DocumentMessage::DocumentHistoryBackward);
-				responses.add(OverlaysMessage::Draw);
-				responses.add(ToolMessage::Undo);
-			}
 			DocumentMessage::UngroupSelectedLayers => {
 				if !self.selection_network_path.is_empty() {
 					log::error!("Ungrouping selected layers is only supported for the Document Network");
@@ -1829,6 +1825,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/overlays/utility_functions.rs

@@ -3,7 +3,7 @@ use crate::consts::HIDE_HANDLE_DISTANCE;
 use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
 use crate::messages::portfolio::document::utility_types::network_interface::NodeNetworkInterface;
 use crate::messages::tool::common_functionality::shape_editor::{SelectedLayerState, ShapeState};
-use crate::messages::tool::tool_messages::tool_prelude::{DocumentMessageHandler, PreferencesMessageHandler};
+use crate::messages::tool::tool_messages::tool_prelude::DocumentMessageHandler;
 use glam::{DAffine2, DVec2};
 use graphene_std::subpath::{Bezier, BezierHandles};
 use graphene_std::text::{Font, FontCache, TextAlign, TextContext, TypesettingConfig};
@@ -200,7 +200,7 @@ pub fn path_overlays(document: &DocumentMessageHandler, draw_handles: DrawHandle
 	}
 }
 
-pub fn path_endpoint_overlays(document: &DocumentMessageHandler, shape_editor: &mut ShapeState, overlay_context: &mut OverlayContext, preferences: &PreferencesMessageHandler) {
+pub fn path_endpoint_overlays(document: &DocumentMessageHandler, shape_editor: &mut ShapeState, overlay_context: &mut OverlayContext) {
 	if !overlay_context.visibility_settings.anchors() {
 		return;
 	}
@@ -213,7 +213,7 @@ pub fn path_endpoint_overlays(document: &DocumentMessageHandler, shape_editor: &
 		let selected = shape_editor.selected_shape_state.get(&layer);
 		let is_selected = |selected: Option<&SelectedLayerState>, point: ManipulatorPointId| selected.is_some_and(|selected| selected.is_point_selected(point));
 
-		for point in vector.extendable_points(preferences.vector_meshes) {
+		for point in vector.extendable_points() {
 			let Some(position) = vector.point_domain.position_from_id(point) else { continue };
 			let position = transform.transform_point2(position);
 			overlay_context.manipulator_anchor(position, is_selected(selected, ManipulatorPointId::Anchor(point)), None);