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.

Sign up for GitHub

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

fix watchTree silent crash #12975

Merged
merged 7 commits into from
Jun 13, 2022
Merged

fix watchTree silent crash #12975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ea8815
fix watchTree silent crash
pinzart Jun 7, 2022
d8d3402
Update WatchTree.xaml.cs
pinzart Jun 7, 2022
f49b66a
add test
pinzart Jun 8, 2022
56729bb
Update DynamoViewTests.cs
pinzart Jun 9, 2022
d9570b5
Update DynamoViewTests.cs
pinzart Jun 10, 2022
5d485d1
remove test
pinzart Jun 13, 2022
e7cff2a
Delete BiggestRectangle.dyn
mjkkirschner Jun 13, 2022
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
5 changes: 1 addition & 4 deletions src/DynamoCoreWpf/Views/Preview/PreviewControl.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,10 +393,7 @@ private void RefreshExpandedDisplay(Action refreshDisplay)

if (largeContentGrid.Children.Count == 0)
{
var tree = new WatchTree
{
DataContext = new WatchViewModel(nodeViewModel.DynamoViewModel.BackgroundPreviewViewModel.AddLabelForPath)
};
var tree = new WatchTree(new WatchViewModel(nodeViewModel.DynamoViewModel.BackgroundPreviewViewModel.AddLabelForPath));
tree.treeView1.ItemContainerGenerator.StatusChanged += WatchContainer_StatusChanged;
largeContentGrid.Children.Add(tree);
}
Expand Down
9 changes: 6 additions & 3 deletions src/DynamoCoreWpf/Views/Preview/WatchTree.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,13 @@ public partial class WatchTree : UserControl
private readonly int minWidthSize = 100;
private readonly int minHeightSize = 38;

public WatchTree()
public WatchTree(WatchViewModel vm)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be ok to break right ?

Copy link
Member

Choose a reason for hiding this comment

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

yes

{
_vm = vm;

InitializeComponent();

DataContext = vm;
this.Loaded += WatchTree_Loaded;
this.Unloaded += WatchTree_Unloaded;
}
Expand All @@ -42,8 +46,7 @@ private void WatchTree_Unloaded(object sender, RoutedEventArgs e)

void WatchTree_Loaded(object sender, RoutedEventArgs e)
{
_vm = this.DataContext as WatchViewModel;
_vm.PropertyChanged += _vm_PropertyChanged;
_vm.PropertyChanged += _vm_PropertyChanged;
}

private void _vm_PropertyChanged(object sender, System.ComponentModel.PropertyChangedEventArgs e)
Expand Down
11 changes: 4 additions & 7 deletions src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/Watch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ public void CustomizeView(Watch nodeModel, NodeView nodeView)
this.watch = nodeModel;
this.syncContext = new DispatcherSynchronizationContext(nodeView.Dispatcher);

var watchTree = new WatchTree();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is any of this code new to 2.15 that causes this to fail only in 2.15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hard to tell by looking at the code...multiple things can contribute to this bug
The critical area (IMO) is the Loaded event ...that assumes a DataContext exists. That part of the code has been there for a while.

The loaded event can be triggered multiple times with/without data context. Looks like at least in 2.15...a Loaded event is triggered without data context (when opening the .dyn attached to the jira task)
This seems to be acceptable (from what I have read) but we should not always assume the context exists.

// make empty watchViewModel
rootWatchViewModel = new WatchViewModel(dynamoViewModel.BackgroundPreviewViewModel.AddLabelForPath);

var watchTree = new WatchTree(rootWatchViewModel);
watchTree.BorderThickness = new Thickness(1, 1, 1, 1);
watchTree.BorderBrush = new SolidColorBrush(Color.FromRgb(220,220,220));

watchTree.SetWatchNodeProperties();

// make empty watchViewModel
rootWatchViewModel = new WatchViewModel(dynamoViewModel.BackgroundPreviewViewModel.AddLabelForPath);

nodeView.PresentationGrid.Children.Add(watchTree);
nodeView.PresentationGrid.Visibility = Visibility.Visible;
// disable preview control
Expand All @@ -61,9 +61,6 @@ public void CustomizeView(Watch nodeModel, NodeView nodeView)

private void Bind(WatchTree watchTree, NodeView nodeView)
{
// The WatchTree Control is bound to the WatchViewModel
watchTree.DataContext = rootWatchViewModel;

// Add binding for TreeView
var sourceBinding = new Binding("Children")
{
Expand Down
8 changes: 4 additions & 4 deletions test/DynamoCoreWpfTests/DynamoViewTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@


namespace DynamoCoreWpfTests
{
{
public class DynamoViewTests : DynamoTestUIBase
{
// adapted from: NodeViewTests.cs
Expand Down Expand Up @@ -58,11 +58,11 @@ public void FooterNotificationControlTest()

var workspace = ViewModel.Model.CurrentWorkspace as HomeWorkspaceModel;
Debug.Assert(workspace != null, nameof(workspace) + " != null");
workspace.Run();
workspace.Run();

List<NodeModel> errorNodes = ViewModel.Model.CurrentWorkspace.Nodes.ToList().FindAll(n => n.State == ElementState.Error);
List<NodeModel> warningNodes = ViewModel.Model.CurrentWorkspace.Nodes.ToList().FindAll(n => n.State == ElementState.Warning || n.State == ElementState.PersistentWarning);

// We should have 3 nodes in Error state and 3 nodes in Warning state
Assert.AreEqual(3, warningNodes.Count());
Assert.AreEqual(3, errorNodes.Count());
Expand Down Expand Up @@ -92,7 +92,7 @@ public void FooterNotificationControlTest()

// Check if the run message indicates warning
Assert.AreEqual(notificationsControl.runNotificationTextBlock.Text, "Run completed with warnings.");

// After deleting all Error nodes, the counter should get to 0
Assert.AreEqual((items[0] as FooterNotificationItem).NotificationCount, 0);

Expand Down