Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables nestable undo/redo action groups in Dynamo by allowing BeginActionGroup() calls to be nested. The implementation flattens nested action groups into the outermost (root) group instead of throwing an exception, which simplifies code that wraps actions in undo groups without needing to check if an action group is already open.
Changes:
- Modified
BeginActionGroup()to detect nesting and only create a new action group at the root level - Updated
ActionGroupDisposableto track whether it represents a root action group and only callEndActionGroup()for root groups - Added
UpdateUndoRedoStack()method toIUndoRedoRecorderClientinterface to notify UI of undo/redo state changes - Connected property change notifications from workspace to view model to update UI when undo/redo state changes
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoCore/Core/UndoRedoRecorder.cs | Core logic changes: added nesting support to BeginActionGroup, modified ActionGroupDisposable to track root status, added UpdateUndoRedoStack to interface, updated comment in EnsureValidRecorderStates |
| src/DynamoCore/Graph/Workspaces/UndoRedo.cs | Implemented UpdateUndoRedoStack method to raise property change notification |
| src/DynamoCore/Models/DynamoModel.cs | Added property change forwarding for CanUndoRedoCommand |
| src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs | Added property change handling for CanUndoRedoCommand to update UI |
| test/DynamoCoreTests/UndoRedoRecorderTests.cs | Added empty UpdateUndoRedoStack implementation to test dummy workspace |
| public void UpdateUndoRedoStack() | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
There are no tests that verify the new nested action group behavior. Consider adding tests that verify: 1) nested action groups (using statements) correctly flatten all actions into the outermost group, 2) undo operations work correctly with nested action groups, and 3) multiple levels of nesting work as expected. This will ensure the feature works as intended and prevent future regressions.
| public void UpdateUndoRedoStack() | ||
| { | ||
|
|
There was a problem hiding this comment.
The test TestBeginActionGroup00 expects an InvalidOperationException when calling BeginActionGroup twice without using statements. However, with the new nestable behavior, this test will fail because nested action groups are now allowed and will not throw an exception. This test needs to be removed or updated to test the new expected behavior (i.e., that nested action groups are properly flattened into the parent group).
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-10184
| @@ -111,9 +113,14 @@ public UndoRedoRecorder(IUndoRedoRecorderClient undoClient) | |||
There was a problem hiding this comment.
"Failing to do so will result in subsequent calls to BeginActionGroup to throw an exception." I think this is no longer true, and should be updated
There was a problem hiding this comment.
Yes, updated to "...will result in the recorder being stuck in an invalid state."
|



Purpose
This allows the
ActionGroupDisposableto be nestable instead of throwing an exception on nesting. It works by merging innerActionGroupDisposableinto the parent one, unless its the root one. This allows to wrap any set of actions inside of anusing(UndoRecorder.BeginActionGroup())Declarations
Check these if you believe they are true
Release Notes
Make
ActionGroupDisposablenestable to allow for easily groupable undo-redo actions.Reviewers
@johnpierson
@BogdanZavu
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of