diff --git a/editor/src/messages/portfolio/document/document_message.rs b/editor/src/messages/portfolio/document/document_message.rs index b6bc9b63ae..8a94907d2c 100644 --- a/editor/src/messages/portfolio/document/document_message.rs +++ b/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, }, - Undo, UngroupSelectedLayers, UngroupLayer { layer: LayerNodeIdentifier, diff --git a/editor/src/messages/portfolio/document/document_message_handler.rs b/editor/src/messages/portfolio/document/document_message_handler.rs index 6402f609fd..30e8cc1a76 100644 --- a/editor/src/messages/portfolio/document/document_message_handler.rs +++ b/editor/src/messages/portfolio/document/document_message_handler.rs @@ -372,8 +372,6 @@ impl MessageHandler> 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> 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> 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> 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) { + 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) { + 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 { let document_message_handler = serde_json::from_str::(serialized_content) .or_else(|e| { diff --git a/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs b/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs index 330d123efe..c68a68cc7c 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs +++ b/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, diff --git a/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs b/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs index c491b3cba0..88c114b8b5 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs @@ -403,15 +403,7 @@ impl<'a> MessageHandler> 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> 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); diff --git a/editor/src/messages/portfolio/document/node_graph/node_properties.rs b/editor/src/messages/portfolio/document/node_graph/node_properties.rs index 2ea963345b..c77f218b6c 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_properties.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_properties.rs @@ -47,7 +47,7 @@ pub fn commit_value(_: &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()]), }) diff --git a/editor/src/messages/portfolio/document/utility_types/network_interface.rs b/editor/src/messages/portfolio/document/utility_types/network_interface.rs index a44c790207..e731e1a872 100644 --- a/editor/src/messages/portfolio/document/utility_types/network_interface.rs +++ b/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, diff --git a/editor/src/messages/tool/tool_messages/freehand_tool.rs b/editor/src/messages/tool/tool_messages/freehand_tool.rs index 9412577b48..88c2511735 100644 --- a/editor/src/messages/tool/tool_messages/freehand_tool.rs +++ b/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, } @@ -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)); }