DYN-10130 Pinning a Dynamo Note can not be undone#16942
DYN-10130 Pinning a Dynamo Note can not be undone#16942jasonstratton merged 5 commits intoDynamoDS:masterfrom
Conversation
Pinning and unpinning a note from a node were not properly undoable. NoteModel now fires an UndoRequest event during deserialization so NoteViewModel can apply the correct pin/unpin state without injecting spurious entries onto the undo stack. Added regression tests for undo of pin, undo of unpin, and undo/redo stack integrity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dNode resolution The previous undo mechanism relied on detecting a field/property mismatch in NoteModel after deserialization and firing a custom UndoRequest event. This silently failed if NoteViewModel was not alive, or if state was unexpected. NoteModel.DeserializeCore now resolves PinnedNode directly from the workspace, removing the dependency on the event side-channel entirely. NoteViewModel reacts to PinnedNode property changes via note_PropertyChanged to manage subscriptions, using a subscribedPinnedNode field to safely unsubscribe even after Model.PinnedNode has been set to null. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a note is pinned to a node inside a group, the note is added to that group. Previously only NoteModel was recorded for undo, leaving AnnotationModel unrecorded. On undo, the pin was cleared but the note remained in the group. PinToNode and UnpinFromNode now use RecordModelsForModification with both NoteModel and AnnotationModel in the same action group, so a single Ctrl+Z atomically reverts both pin state and group membership. UnpinFromNode removes the note from its group via the Nodes setter (which handles event wiring and boundary update internally). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r-dispose Callbacks (RequestRenderPackages, ViewCameraChanged) were being detached after geometry was already disposed, creating a window where in-flight render callbacks could access disposed ProtoGeometry Points and cause a NullReferenceException in LibG. - NodeManipulator.Dispose: detach handlers and delete gizmos before disposing geometry - Gizmo.Dispose: unsubscribe ViewCameraChanged before disposing geometry - MousePointManipulator.UpdatePosition: dispose old origin before replacing it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-10130
There was a problem hiding this comment.
Pull request overview
Fixes undo/redo behavior for pinning/unpinning Dynamo notes to nodes (DYN-10130), and tightens disposal ordering in manipulation gizmo code to avoid event callbacks touching disposed geometry.
Changes:
- Adds regression tests covering undo of unpin, group membership restoration, and undo/redo stability for note pinning.
- Refactors note pin/unpin undo behavior to record related group (AnnotationModel) changes in the same undo action group and removes the prior undo event side-channel.
- Adjusts disposal ordering in
NodeManipulator/Gizmoand updates origin management inMousePointManipulatorto reduce use-after-dispose risk.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DynamoCoreWpf3Tests/NoteViewTests.cs | Adds regression tests for pin/unpin undo/redo and group membership restoration. |
| src/DynamoManipulation/NodeManipulator.cs | Changes dispose ordering to detach handlers and delete gizmos earlier. |
| src/DynamoManipulation/MousePointManipulator.cs | Disposes/recreates origin each update to avoid stale geometry. |
| src/DynamoManipulation/Gizmo.cs | Unsubscribes from camera-change events before disposing geometry. |
| src/DynamoCoreWpf/ViewModels/Core/NoteViewModel.cs | Reworks pin/unpin to record group + note in one undo group and updates subscription tracking. |
| src/DynamoCoreWpf/Commands/NoteCommands.cs | Normalizes file header/encoding (BOM removal). |
| src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs | Sets NoteModel.Workspace when adding notes. |
| src/DynamoCore/Graph/Notes/NoteModel.cs | Attempts to resolve pinned node during deserialization via workspace reference; retires old side-channel method. |
| private void AddNote(NoteModel note) | ||
| { | ||
| note.Workspace = this; | ||
|
|
There was a problem hiding this comment.
AddNote sets note.Workspace, but this happens after some call sites have already called NoteModel.Deserialize (e.g., NodeGraph.LoadNoteFromXml returns a deserialized NoteModel that is then passed into AddNote). If the goal is to let DeserializeCore resolve PinnedNode via Workspace, this assignment may be too late for those scenarios. Consider setting Workspace on the NoteModel before deserializing, or adding a post-add resolution step in AddNote when PinnedNode is still null but a serialized pinned-node GUID exists.
There was a problem hiding this comment.
Workspace is null in the CreateModel path (undo of a note deletion, and initial file load), but this code is executed on the ReloadModel path(undoing a modification to a note that already exists in the workspace). So Workspace is already set.
In the CreateModel/file load path (where LoadNoteFromXml deserializes a note before AddNote sets Workspace) is a pre-existing issue. TryToSubscribeUndoNote mechanism has its own failure modes doesn't reliably restore pin state in those paths
I will file a Jira issue to track this issue and assign it to myself.
There was a problem hiding this comment.
Created DYN-10234
| protected override void DeserializeCore(XmlElement nodeElement, SaveContext context) | ||
| { | ||
| var helper = new XmlElementHelper(nodeElement); | ||
| XmlElementHelper helper = new XmlElementHelper(nodeElement); | ||
| GUID = helper.ReadGuid("guid", GUID); | ||
| Text = helper.ReadString("text", "New Note"); | ||
| X = helper.ReadDouble("x", 0.0); | ||
| Y = helper.ReadDouble("y", 0.0); | ||
| PinnedNodeGuid = helper.ReadGuid("pinnedNode", Guid.Empty); | ||
|
|
||
| // Notify listeners that the position of the note has changed, | ||
| // then parent group will also redraw itself. | ||
| ReportPosition(); | ||
| TryToSubscribeUndoNote(); | ||
| } | ||
| Guid savedPinnedNodeGuid = helper.ReadGuid("pinnedNode", Guid.Empty); | ||
|
|
||
| /// <summary> | ||
| /// Verify if the current user action is to pin a node so the 'unpin' method can be called to undo the action | ||
| /// </summary> | ||
| internal void TryToSubscribeUndoNote() | ||
| { | ||
| if (pinnedNode != null && PinnedNodeGuid == Guid.Empty && UndoRequest != null) | ||
| // Resolve the pinned node directly from the workspace rather than relying | ||
| // on the fragile TryToSubscribeUndoNote side-channel event mechanism. | ||
| if (savedPinnedNodeGuid == Guid.Empty) | ||
| { | ||
| UndoRedoAction = UndoAction.Unpin; | ||
| UndoRequest(this); | ||
| return; | ||
| PinnedNode = null; | ||
| } | ||
| else if (pinnedNode == null && PinnedNodeGuid != Guid.Empty && UndoRequest != null) | ||
| else if (Workspace != null) | ||
| { | ||
| UndoRedoAction = UndoAction.Pin; | ||
| UndoRequest(this); | ||
| PinnedNode = Workspace.Nodes.FirstOrDefault(n => n.GUID == savedPinnedNodeGuid); | ||
| } |
There was a problem hiding this comment.
DeserializeCore only resolves the pinned node when Workspace is non-null, but NoteModel instances are often deserialized before being added to a WorkspaceModel (e.g., NodeGraph.LoadNoteFromXml used by initial file load and UndoRedo note reconstruction). In those cases Workspace is null during DeserializeCore, so the saved pinned node will never be restored. Consider persisting the GUID into PinnedNodeGuid and deferring resolution until Workspace is assigned (or ensure Workspace is set before Deserialize is called).
There was a problem hiding this comment.
This is similar to the previous comment.
AddNote setting Workspace after deserialization is problematic for the LoadNoteFromXml → AddNote sequence. However, undo of pin/unpin goes through ReloadModel, which calls Deserialize on the note already present in the workspace. Therefore Workspace is set before DeserializeCore runs in that path. The issue pointed out is a preexisting condition, unrelated to pin note undo.
I will create a Jira issue to track this and assign it to myself.
There was a problem hiding this comment.
Created DYN-10234
kalunkuo
left a comment
kalunkuo
left a comment
There was a problem hiding this comment.
LGTM, this approach fixes pin/unpin behavior reliably and also resolves the pin-to-group undo issue!
DYN-9599 (DynamoDS#16920) landed on master concurrently and added deferred pin restoration when a deleted node+note pair is restored via undo. It introduced SuppressUndoRecording on NoteModel and ResolvePinnedNodeReference on WorkspaceViewModel, both of which rely on TryToSubscribeUndoNote and the UndoRequest event that DYN-10130 had removed. Resolution: - Restore UndoRequest event, note_PinUnpinToNode handler, and TryToSubscribeUndoNote(recordForUndo) from DYN-9599 so that ResolvePinnedNodeReference continues to work for the undo-of-deletion and file-load deferred resolution paths. - Keep SuppressUndoRecording from DYN-9599 and guard PinToNode's RecordModelsForModification (note + group) behind it so that deferred restoration does not pollute the undo stack. - Keep DYN-10130's Workspace property and DeserializeCore direct resolution, which handles the undo-of-modification path and also sets PinnedNodeGuid so ResolvePinnedNodeReference detects pending pins in the Workspace-null paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@CustomBuildingConfigurators-Balaji, I had a merge conflict with the PR you merged for DYN-9599, PR #16920. I have determined that our changes do not impact each other, but I think you should give it a code review to confirm. Thank you. |
|
|
I did not hear back from @CustomBuildingConfigurators-Balaji. So I will proceed with the merge now. |
@jasonstratton, I have reviewed the code changes for this commit and tested DYN-9599 with the merged code. DYN-9599 is working as expected. |
Purpose
Fixes https://jira.autodesk.com/browse/DYN-10130 — pressing Ctrl+Z after pinning a Dynamo Note to a node had no effect, and repeated Ctrl+Z produced unpredictable behavior. The inverse action (unpin via context menu) was also not undoable.
Changes
Release Notes
Fixed a bug where pinning or unpinning a Dynamo Note to a node could not be undone with Ctrl+Z, and repeated undo attempts produced unpredictable behavior. The fix also ensures that undo correctly restores group membership when the pinned node belongs to an annotation group.
Reviewers
@DynamoDS/eidos
@kalunkuo (Because Karen has been working on the undo stack)
FYIs
Full documentation of the analysis and changes can be found in Jason's DynaNotes
Each commit is self-contained and can be reviewed or reverted independently.