| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-9107: Make Undo Redo Work As Expected #16366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8602
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Technically this is fine but I'm not sure if we really want to do this/might be seen as a bug/create confusion. Plus it will be tricky to roll it back later when we fix the undo behavior. Let's sleep over it and discuss tomorrow. |
|
Sounds good, I do think it has a certain amount of precedence as well. In revit when you are in an edit mode, you cannot do a lot of other actions. Eg. While editing a floor, you cannot place walls. So I think we can round this out with something that shows it is a process that is ongoing. Also, in the bug bash today, at least 3 people were trying to hookup and unhook DNA results while working with it, this also prevents that. |
johnpierson
changed the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9107
johnpierson
changed the title
johnpierson
changed the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the undo/redo handling around node autocomplete by removing the old lock-based approach and instead explicitly recording undo states, and it updates how transient nodes and connectors are cleaned up.
- Removed
ToggleUndoRedoLockedcalls andIsUndoRedoLockedflag, replacing them with targetedRecordUndoModelscalls - Updated
DeleteTransientNodesto directly remove and dispose connectors/nodes rather than using commands and undo-pop - Removed lock checks from
CanUndo/CanRedoinUndoRedo.cs
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml.cs | Removed obsolete ToggleUndoRedoLocked(false) call when closing the autocomplete view |
| src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs | Replaced lock toggles with DynamoModel.RecordUndoModels; updated transient connector/node cleanup logic |
| src/DynamoCore/Graph/Workspaces/UndoRedo.cs | Removed IsUndoRedoLocked property and related lock checks from CanUndo/CanRedo |
Comments suppressed due to low confidence (2)
src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml.cs:204
- With the old ToggleUndoRedoLocked call removed, ensure the new IsNodeOperationsBlocked flag (or equivalent) is reset when the autocomplete closes so that command execution, including undo/redo, is properly unblocked.
ViewModel?.DeleteTransientNodes();
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs:802
- Consider adding unit tests for DeleteTransientNodes that verify transient connectors are correctly found, deleted, disposed, and removed from the view model.
var transientConnectors = wsViewModel.Connectors.Where(c => c.IsTransient).ToList();
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs
Show resolved
Hide resolved
…arViewModel.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
LGTM! Working very nice ! Let's get this in before my pr. |
Purpose
This is a completion of a previous PR for consolidating Undos. The undos only get added to the stack when the user accepts the result now. We also don't modify the stack with
Popany longer.This PR introduces a mechanism to prevent node creation and deletion during DNA (autocomplete) operations by adding a block flag, wiring it into command execution, and toggling it around the autocomplete lifecycle.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
@DynamoDS/synapse
FYIs
@DynamoDS/eidos