DYN-10081: Preserve warnings/info stack format and Batch node info updates#16955
DYN-10081: Preserve warnings/info stack format and Batch node info updates#16955zeusongit wants to merge 5 commits intoDynamoDS:masterfrom
Conversation
Prevent transient warnings/errors from being cleared when restoring node State during Info() by adding a suppressTransientClear flag and scoping State assignments. Adjust ClearInfoMessages so only info-related states are promoted back to Active and other states (Warning/Error) keep their state while still removing info entries. Add batching to InfoBubble/NodeViewModel updates (BeginBatchUpdate/EndBatchUpdate and dispatcher invocation) to avoid repeated RefreshNodeInformationalStateDisplay calls and duplicate subscriptions/Workspace error entries. Minor fixes: tighten GetHashCode parentheses and add Clear() to ObservableHashSet. Overall: correctness (preserve warnings) and performance (batch updates, avoid double-subscribe).
![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-10081
There was a problem hiding this comment.
Pull request overview
Fixes and improves the node “bubble” messaging system so Info and Warning/Error bubbles can coexist (stacked) on non-chart nodes, while also reducing redundant UI refresh work during message updates.
Changes:
- Prevents
NodeModel.Info()state restoration from clearing transient warnings/errors, allowing Warning+Info to coexist inInfos. - Adds batching to suppress per-item
RefreshNodeInformationalStateDisplay()rebuilds duringNodeMessagesupdates. - Improves bubble lifecycle/subscription handling and fixes
Info.GetHashCode()operator precedence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/DynamoUtilities/ObservableHashSet.cs |
Adds Clear() with collection reset notification support. |
src/DynamoCoreWpf/ViewModels/Preview/InfoBubbleViewModel.cs |
Adds nested batch update tracking to suppress repeated refresh during bulk NodeMessages updates; disposes more cleanly. |
src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs |
Batches bubble message updates, prevents duplicate subscriptions, avoids duplicate Errors insertion, and nulls ErrorBubble on dispose. |
src/DynamoCore/Graph/Nodes/NodeModel.cs |
Fixes GetHashCode(), adds suppression flag for state restoration side-effects, and updates ClearInfoMessages() / Info() behavior. |
You can also share your feedback on Copilot code review. Take the survey.
| /// Info() state restoration. Access is single-threaded (NodeModel state mutations | ||
| /// are not thread-safe by design). | ||
| /// </summary> | ||
| private bool suppressTransientClear; |
There was a problem hiding this comment.
I think this should be named suppressClearTransient or suppressClearTransientMessages - something like that, since you're suppressing the clearing.
| // For any other state (Warning, Error, Active, Dead, etc.), clear all info types. | ||
| // This fixes the case where State was restored (e.g., by Info() method) and no | ||
| // longer matches Info/PersistentInfo, but info entries still exist in Infos. | ||
| infos.RemoveWhere(x => x.State == ElementState.PersistentInfo || x.State == ElementState.Info); |
There was a problem hiding this comment.
This confusing naming has been there for a while. We have a notion of Info vs Warning messages, but we still maintain a type with the same conflicting name, "Info", that actually can be any type of message, even warning or error.
There was a problem hiding this comment.
Is this a good time to rename some of these fields/properties to something more sensible and have a clear separation of meaning?
There was a problem hiding this comment.
will look into this, this has bothered me for a while too
| CollectionChanged?.Invoke(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, range)); | ||
| } | ||
|
|
||
| public void Clear() |
There was a problem hiding this comment.
Where is this new method called?
aparajit-pratap
left a comment
aparajit-pratap
left a comment
There was a problem hiding this comment.
Some minor comments and questions, then LGTM!
Rename the private boolean suppressTransientClear to suppressClearTransientMessages in NodeModel and update all references accordingly. The flag is used to suppress clearing transient warnings/errors during state/Info restoration. Also remove the public Clear() method from ObservableHashSet<T> (and its Reset event invocation), so the collection no longer exposes a Clear API.
|
Purpose
TL;DR
Fixes a bug where Info and Warning bubbles couldn't coexist on non-chart nodes (info would silently wipe the warning from the Infos collection), plus performance and lifecycle improvements to the bubble system.
Problem
Changes
What this enables
All three bubble sections (Error, Warning, Info) can now coexist on any node — not just chart nodes. The node's bottom bar reflects the highest-priority state, and the bubble stacks sections vertically: Errors (red) → Warnings (orange) → Info (blue).
Before:
After:
Declarations
Check these if you believe they are true
Release Notes
Fixed a bug where Info and Warning bubbles couldn't coexist on non-chart nodes (info would silently wipe the warning from the Infos collection), plus performance and lifecycle improvements to the bubble system.
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of