Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions src/DynamoCore/Graph/Nodes/NodeModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public override int GetHashCode()
unchecked
{
int hash = 17;
hash = hash * 23 + Message == null ? 0 : Message.GetHashCode();
hash = hash * 23 + (Message == null ? 0 : Message.GetHashCode());
hash = hash * 23 + State.GetHashCode();
return hash;
}
Expand All @@ -81,6 +81,12 @@ public abstract class NodeModel : ModelBase, IRenderPackageSource<NodeModel>, ID
private bool canUpdatePeriodically;
private string name;
private ElementState state;
/// <summary>
/// Suppresses ClearTransientWarningsAndErrors() in the State setter during
/// Info() state restoration. Access is single-threaded (NodeModel state mutations
/// are not thread-safe by design).
/// </summary>
private bool suppressClearTransientMessages;
private readonly ObservableHashSet<Info> infos = new ObservableHashSet<Info>();
private string description;

Expand Down Expand Up @@ -390,7 +396,8 @@ public ElementState State
get { return state; }
set
{
if (value != ElementState.Error && value != ElementState.Info && value != ElementState.PersistentInfo && value != ElementState.AstBuildBroken)
if (!suppressClearTransientMessages &&
value != ElementState.Error && value != ElementState.Info && value != ElementState.PersistentInfo && value != ElementState.AstBuildBroken)
ClearTransientWarningsAndErrors();

// Check before settings and raising
Expand Down Expand Up @@ -1797,13 +1804,12 @@ public virtual void ClearErrorsAndWarnings()
}

/// <summary>
/// Clears the info messages that are generated when running the graph,
/// the State will be set to ElementState.Active.
/// Clears the info messages that are generated when running the graph.
/// If the current State is Info or PersistentInfo, it will be set to Active.
/// If the current State is Warning, Error, or another non-info state, it is preserved.
/// </summary>
public virtual void ClearInfoMessages()
{
// It is very unlikely a node could be in both info state and persistent info state from the current design
// If that exception happens, we should redesign this function or have particular node override the behavior
if (State == ElementState.Info)
Comment thread
aparajit-pratap marked this conversation as resolved.
{
infos.RemoveWhere(x => x.State == ElementState.Info);
Expand All @@ -1812,12 +1818,21 @@ public virtual void ClearInfoMessages()
{
infos.RemoveWhere(x => x.State == ElementState.PersistentInfo);
}
// If there are still warnings/errors, keep the state unchanged.
else if (State == ElementState.Warning || State == ElementState.Error)
else
{
// 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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a good time to rename some of these fields/properties to something more sensible and have a clear separation of meaning?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will look into this, this has bothered me for a while too

}
State = ElementState.Active;

// Only reset State to Active if it was info-related.
// Don't demote Warning/Error to Active — that would trigger
// ClearTransientWarningsAndErrors() via the setter side effect.
if (State == ElementState.Info || State == ElementState.PersistentInfo)
{
State = ElementState.Active;
}
OnNodeInfoMessagesClearing();
}

Expand Down Expand Up @@ -1985,7 +2000,13 @@ public void Info(string p, bool isPersistent = false)
initialState == ElementState.PersistentWarning ||
initialState == ElementState.Error)
{
State = initialState;
// Suppress the ClearTransientWarningsAndErrors() side effect in the State setter
// during restoration. Without this, restoring State to Warning triggers the setter
// which calls ClearTransientWarningsAndErrors(), wiping the warning entry from Infos
// that was just added before Info() was called.
suppressClearTransientMessages = true;
try { State = initialState; }
finally { suppressClearTransientMessages = false; }
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
}
}

protected override void HandleTaskCompletionCore()

Check failure on line 113 in src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=DynamoDS_Dynamo&issues=AZzeg4QiWod_BZQXVmU0&open=AZzeg4QiWod_BZQXVmU0&pullRequest=16955
{
if (engineController.IsDisposed)
{
Expand Down Expand Up @@ -158,7 +158,8 @@
node.ClearErrorsAndWarnings();
}
}
if (node.State == ElementState.Info)
if (node.State == ElementState.Info ||
node.Infos.Any(x => x.State == ElementState.Info))
{
using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.Infos), nameof(NodeModel.State)))
{
Expand Down
59 changes: 35 additions & 24 deletions src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,7 @@ public override void Dispose()
NodeModel.NodeWarningMessagesClearing -= Logic_NodeWarningMessagesClearing;

if (ErrorBubble != null) DisposeErrorBubble();
ErrorBubble = null;

DynamoSelection.Instance.Selection.CollectionChanged -= SelectionOnCollectionChanged;
delayDocumentBrowserRefresh?.Dispose();
Expand Down Expand Up @@ -1383,23 +1384,24 @@ private void BuildErrorBubble()
// ErrorBubble's ZIndex should be the node's ZIndex + 2.
ZIndex = ZIndex + 2
};

// All subscriptions inside the null guard to prevent double-subscription
// if BuildErrorBubble is ever called when ErrorBubble already exists.
ErrorBubble.NodeInfoToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.NodeWarningsToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.NodeErrorsToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.PropertyChanged += ErrorBubble_PropertyChanged;
ErrorBubble.DismissedMessages.CollectionChanged += DismissedNodeMessages_CollectionChanged;
}

ErrorBubble.NodeInfoToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.NodeWarningsToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.NodeErrorsToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.PropertyChanged += ErrorBubble_PropertyChanged;

if (DynamoViewModel.UIDispatcher != null)
{
DynamoViewModel.UIDispatcher.Invoke(() =>
{
WorkspaceViewModel.Errors.Add(ErrorBubble);
if (!WorkspaceViewModel.Errors.Contains(ErrorBubble))
WorkspaceViewModel.Errors.Add(ErrorBubble);
});
}

// The Node displays a count of dismissed messages, listening to that collection in the node's ErrorBubble
ErrorBubble.DismissedMessages.CollectionChanged += DismissedNodeMessages_CollectionChanged;
}

// These colors are duplicated from the DynamoColorsAndBrushesDictionary as it is not assumed that the xaml will be loaded before setting the color
Expand Down Expand Up @@ -1668,10 +1670,17 @@ public void UpdateBubbleContent()

ErrorBubble.InfoBubbleStyle = style;

// If running Dynamo with UI, use dispatcher, otherwise not
if (DynamoViewModel.UIDispatcher != null)
{
DynamoViewModel.UIDispatcher.Invoke(() =>
// Batch updates to avoid O(n²) cascade: each NodeMessages.Add() triggers
// RefreshNodeInformationalStateDisplay() which clears/rebuilds 3 display collections.
// By batching, we do one refresh at the end instead of N refreshes.
// NOTE: We use incremental Contains+Add (not Clear+rebuild) because messages
// accumulate across multiple UpdateBubbleContent() calls (e.g., build errors
// followed by runtime warnings). ClearTransientWarningsAndErrors() may remove
// earlier entries from Infos between calls, but they must persist in NodeMessages.
void UpdateMessages()
{
ErrorBubble.BeginBatchUpdate();
try
{
foreach (var data in packets)
{
Expand All @@ -1680,21 +1689,23 @@ public void UpdateBubbleContent()
ErrorBubble.NodeMessages.Add(data);
}
}
HandleColorOverlayChange();
});
}
else
{
foreach (var data in packets)
}
finally
{
if (!ErrorBubble.NodeMessages.Contains(data))
{
ErrorBubble.NodeMessages.Add(data);
}
ErrorBubble.EndBatchUpdate();
}
HandleColorOverlayChange();
}
ErrorBubble.ChangeInfoBubbleStateCommand.Execute(InfoBubbleViewModel.State.Pinned);

if (DynamoViewModel.UIDispatcher != null)
{
DynamoViewModel.UIDispatcher.Invoke(UpdateMessages);
}
else
{
UpdateMessages();
}
ErrorBubble.ChangeInfoBubbleStateCommand.Execute(InfoBubbleViewModel.State.Pinned);
}

private void UpdateErrorBubblePosition()
Expand Down
33 changes: 32 additions & 1 deletion src/DynamoCoreWpf/ViewModels/Preview/InfoBubbleViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,12 @@ public override bool IsCollapsed
/// </summary>
public ObservableCollection<InfoBubbleDataPacket> DismissedMessages { get; } = new ObservableCollection<InfoBubbleDataPacket>();

/// <summary>
/// Tracks nested batch update depth. When > 0, suppresses
/// RefreshNodeInformationalStateDisplay() calls during batch updates to NodeMessages.
/// </summary>
private int batchUpdateDepth;

/// <summary>
/// Used to determine whether the UI container for node Info is visible
/// </summary>
Expand Down Expand Up @@ -551,7 +557,31 @@ private bool CanCopyTextToClipboard(object parameter)
/// <param name="e"></param>
private void NodeInformation_CollectionChanged(object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e)
{
RefreshNodeInformationalStateDisplay();
if (batchUpdateDepth == 0)
RefreshNodeInformationalStateDisplay();
}

/// <summary>
/// Begins a batch update. Suppresses per-item RefreshNodeInformationalStateDisplay()
/// calls until the matching EndBatchUpdate() is called. Supports nesting.
/// </summary>
internal void BeginBatchUpdate()
{
batchUpdateDepth++;
}

/// <summary>
/// Ends a batch update. When the outermost batch completes (depth returns to 0),
/// performs a single RefreshNodeInformationalStateDisplay().
/// </summary>
internal void EndBatchUpdate()
{
if (batchUpdateDepth <= 0)
return;

batchUpdateDepth--;
if (batchUpdateDepth == 0)
RefreshNodeInformationalStateDisplay();
Comment thread
zeusongit marked this conversation as resolved.
}

#region Command Methods
Expand Down Expand Up @@ -1068,6 +1098,7 @@ internal void ValidateWorkspaceRunStatusMsg()
public override void Dispose()
{
NodeMessages.CollectionChanged -= NodeInformation_CollectionChanged;
base.Dispose();
}
}

Expand Down
106 changes: 106 additions & 0 deletions test/DynamoCoreTests/Models/NodeModelWarnings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,112 @@ public void KeepTransitionBetweenWarningTypes()
Assert.AreEqual(0, cbn.Infos.Count);
}

/// <summary>
/// This test verifies that Warning and Info messages can coexist on a node.
/// After Info() is called on a node in Warning state, both entries should remain
/// in Infos and the State should be restored to Warning (not downgraded to Info).
/// ClearInfoMessages should remove info entries without affecting warnings.
/// ClearErrorsAndWarnings should remove warning entries without affecting persistent info.
/// </summary>
[Test]
[Category("UnitTests")]
public void WarningAndInfoCoexistOnNode()
{
var cbn = new CodeBlockNodeModel(CurrentDynamoModel.LibraryServices);
var command = new DynamoModel.CreateNodeCommand(cbn, 0, 0, true, false);
CurrentDynamoModel.ExecuteCommand(command);
Assert.IsNotNull(cbn);

// Add a transient warning, then a transient info
cbn.Warning("TestWarning0", false);
Assert.AreEqual(ElementState.Warning, cbn.State);
Assert.AreEqual(1, cbn.Infos.Count);

cbn.Info("TestInfo0", false);

// Both should coexist; State should be restored to Warning (higher priority)
Assert.AreEqual(ElementState.Warning, cbn.State);
Assert.AreEqual(2, cbn.Infos.Count);
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestWarning0") && x.State == ElementState.Warning));
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestInfo0") && x.State == ElementState.Info));

// ClearInfoMessages should remove info but preserve warning
cbn.ClearInfoMessages();
Assert.AreEqual(ElementState.Warning, cbn.State);
Assert.AreEqual(1, cbn.Infos.Count);
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestWarning0") && x.State == ElementState.Warning));

// Clean up
cbn.ClearErrorsAndWarnings();
Assert.AreEqual(ElementState.Active, cbn.State);
Assert.AreEqual(0, cbn.Infos.Count);
}

/// <summary>
/// This test verifies that persistent warning and persistent info can coexist,
/// and that Error state takes priority over both.
/// </summary>
[Test]
[Category("UnitTests")]
public void PersistentWarningAndInfoCoexistOnNode()
{
var cbn = new CodeBlockNodeModel(CurrentDynamoModel.LibraryServices);
var command = new DynamoModel.CreateNodeCommand(cbn, 0, 0, true, false);
CurrentDynamoModel.ExecuteCommand(command);
Assert.IsNotNull(cbn);

// Add persistent warning, then persistent info
cbn.Warning("TestPersistentWarning", true);
Assert.AreEqual(ElementState.PersistentWarning, cbn.State);

cbn.Info("TestPersistentInfo", true);

// Both should coexist; State restored to PersistentWarning
Assert.AreEqual(ElementState.PersistentWarning, cbn.State);
Assert.AreEqual(2, cbn.Infos.Count);
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestPersistentWarning") && x.State == ElementState.PersistentWarning));
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestPersistentInfo") && x.State == ElementState.PersistentInfo));

// Error takes priority over everything
cbn.Error("TestError");
Assert.AreEqual(ElementState.Error, cbn.State);
Assert.AreEqual(3, cbn.Infos.Count);

// ClearErrorsAndWarnings clears warnings and errors but preserves persistent info
cbn.ClearErrorsAndWarnings();
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestPersistentInfo") && x.State == ElementState.PersistentInfo));
Assert.IsFalse(cbn.Infos.Any(x => x.State == ElementState.Error));
Assert.IsFalse(cbn.Infos.Any(x => x.State == ElementState.PersistentWarning));
}

/// <summary>
/// This test verifies that Error state restoration in Info() preserves error entries.
/// </summary>
[Test]
[Category("UnitTests")]
public void ErrorAndInfoCoexistOnNode()
{
var cbn = new CodeBlockNodeModel(CurrentDynamoModel.LibraryServices);
var command = new DynamoModel.CreateNodeCommand(cbn, 0, 0, true, false);
CurrentDynamoModel.ExecuteCommand(command);
Assert.IsNotNull(cbn);

// Add error, then transient info
cbn.Error("TestError0");
Assert.AreEqual(ElementState.Error, cbn.State);

cbn.Info("TestInfo0", false);

// Both should coexist; State restored to Error (highest priority)
Assert.AreEqual(ElementState.Error, cbn.State);
Assert.AreEqual(2, cbn.Infos.Count);
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestError0") && x.State == ElementState.Error));
Assert.IsTrue(cbn.Infos.Any(x => x.Message.Equals("TestInfo0") && x.State == ElementState.Info));

cbn.ClearErrorsAndWarnings();
Assert.AreEqual(ElementState.Active, cbn.State);
}

[Test]
[Category("UnitTests")]
public void CombinedBuildAndRuntimeWarnings()
Expand Down
Loading