Skip to content
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

improve performance of bubbleINfo #15129

Merged
merged 2 commits into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions src/DynamoCore/Graph/Nodes/NodeModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@ internal ObservableHashSet<Info> Infos
get { return infos; }
}

/// <summary>
/// BlockInfoBubbleUpdates is flag used to block InfoBubble updates during (or immediately after) graph execution.
/// </summary>
internal bool BlockInfoBubbleUpdates = false;

/// <summary>
/// A publicly accessible collector of all Info/Warning/Error data
Expand Down
8 changes: 8 additions & 0 deletions src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,11 @@ private void OnUpdateGraphCompleted(AsyncTask task)
var node = workspace.Nodes.FirstOrDefault(n => n.GUID == guid);
if (node == null)
continue;

// Block Infos updates during the many errors/warnings/notifications added here
// InfoBubbles will be updated on NodeViewModel's EvaluationCompleted handler.
using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.Infos), nameof(NodeModel.State)))
using (Disposable.Create(() => { node.BlockInfoBubbleUpdates = true; }, () => { node.BlockInfoBubbleUpdates = false; }))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably could have stopped collection notifications on nodeModel.Infos, but a dedicated flag for updates to bubbleInfo seemed easier to do and understand

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't refreshed my memory on the PropertyChangeManager and how it suppresses property-changed events, but my question is are both this and the blockInfoBubbleUpdates required? How are these different if both ensure the UI updates on the info bubbles happen at the end of the graph execution? In other words, aren't the updates batched for the end of the graph execution already (with the introduction of PropertyChangeManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PropertyChangeManager handles property changes. There are still CollectionChanged events that slip through the cracks. PropertyChangeManager was never adapted to suppress CollectionChanged events (Not sure it can easily)

BlockInfoBubbleUpdates is required for now to block UpdateBubbleContent calls during collection changed events.

There is an event in the NodeViewModel class, NodeModel.Infos.CollectionChanged += () => {UpdateBubbleContent()};

{
node.Warning(warning.Value); // Update node warning message.
}
Expand All @@ -711,7 +715,11 @@ private void OnUpdateGraphCompleted(AsyncTask task)
var node = workspace.Nodes.FirstOrDefault(n => n.GUID == guid);
if (node == null)
continue;

// Block Infos updates during the many errors/warnings/notifications added here
// InfoBubbles will be updated on NodeViewModel's EvaluationCompleted handler.
using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.Infos), nameof(NodeModel.State)))
using (Disposable.Create(() => { node.BlockInfoBubbleUpdates = true; }, () => { node.BlockInfoBubbleUpdates = false; }))
{
node.Info(string.Join(Environment.NewLine, info.Value.Select(w => w.Message)));
}
Expand Down
29 changes: 19 additions & 10 deletions src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1256,10 +1256,17 @@ private void UpdateErrorBubbleWidth()
/// </summary>
private void BuildErrorBubble()
{
if (ErrorBubble == null) ErrorBubble = new InfoBubbleViewModel(this)
if (ErrorBubble == null)
{
IsCollapsed = this.IsCollapsed
};
ErrorBubble = new InfoBubbleViewModel(this)
{
IsCollapsed = this.IsCollapsed,
// The Error bubble sits above the node in ZIndex. Since pinned notes sit above
// the node as well and the ErrorBubble needs to display on top of these, the
// ErrorBubble's ZIndex should be the node's ZIndex + 2.
ZIndex = ZIndex + 2
};
}

ErrorBubble.NodeInfoToDisplay.CollectionChanged += UpdateOverlays;
ErrorBubble.NodeWarningsToDisplay.CollectionChanged += UpdateOverlays;
Expand All @@ -1273,14 +1280,8 @@ private void BuildErrorBubble()
WorkspaceViewModel.Errors.Add(ErrorBubble);
});
}

// The Error bubble sits above the node in ZIndex. Since pinned notes sit above
// the node as well and the ErrorBubble needs to display on top of these, the
// ErrorBubble's ZIndex should be the node's ZIndex + 2.
ErrorBubble.ZIndex = ZIndex + 2;

// The Node displays a count of dismissed messages, listening to that collection in the node's ErrorBubble

ErrorBubble.DismissedMessages.CollectionChanged += DismissedNodeMessages_CollectionChanged;
}

Expand Down Expand Up @@ -1464,7 +1465,15 @@ private void DisposeErrorBubble()

public void UpdateBubbleContent()
{
if (DynamoViewModel == null) return;
if (NodeModel.BlockInfoBubbleUpdates)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify this with:
if (NodeModel.BlockInfoBubbleUpdates == true || DynamoViewModel == null) return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it easier to read this way.

{
return;
}

if (DynamoViewModel == null)
{
return;
}

bool hasErrorOrWarning = NodeModel.IsInErrorState || NodeModel.State == ElementState.Warning;
bool isNodeStateInfo = NodeModel.State == ElementState.Info || NodeModel.State == ElementState.PersistentInfo;
Expand Down
8 changes: 5 additions & 3 deletions src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,8 @@ void Model_NodeRemoved(NodeModel node)
lock (Nodes)
{
nodeViewModel = Nodes.First(x => x.NodeLogic == node);
Errors.Remove(nodeViewModel.ErrorBubble);
if (nodeViewModel.ErrorBubble != null)
Errors.Remove(nodeViewModel.ErrorBubble);
Nodes.Remove(nodeViewModel);
}
//unsub the events we attached below in NodeAdded.
Expand All @@ -905,8 +906,9 @@ void Model_NodeAdded(NodeModel node)
{
Nodes.Add(nodeViewModel);
}
Errors.Add(nodeViewModel.ErrorBubble);

if (nodeViewModel.ErrorBubble != null)
Errors.Add(nodeViewModel.ErrorBubble);

PostNodeChangeActions();
}

Expand Down
Loading
Loading