DYN-6427 & DYN-6828 Graph Properties UI Fixes (#15200)

Co-authored-by: Trygve Wastvedt <[email protected]>
Co-authored-by: pinzart90 <[email protected]>

src/GraphMetadataViewExtension/GraphMetadataViewModel.cs

@@ -1,13 +1,13 @@
 using System;
 using System.Collections.ObjectModel;
 using System.IO;
+using System.Linq;
 using System.Windows.Media.Imaging;
 using Dynamo.Core;
 using Dynamo.Graph.Workspaces;
 using Dynamo.GraphMetadata.Controls;
 using Dynamo.Linting;
 using Dynamo.UI.Commands;
-using Dynamo.ViewModels;
 using Dynamo.Wpf.Extensions;
 
 namespace Dynamo.GraphMetadata
@@ -114,10 +114,9 @@ public GraphMetadataViewModel(ViewLoadedParams viewLoadedParams, GraphMetadataVi
             this.linterManager = viewLoadedParams.StartupParams.LinterManager;
 
             this.viewLoadedParams.CurrentWorkspaceChanged += OnCurrentWorkspaceChanged;
-            // using this as CurrentWorkspaceChanged wont trigger if you:
-            // Close a saved workspace and open a New homeworkspace..
-            // This means that properties defined in the previous opened workspace will still be showed in the extension.
-            // CurrentWorkspaceCleared will trigger everytime a graph is closed which allows us to reset the properties. 
+            // Using this as CurrentWorkspaceChanged won't trigger if you close a saved workspace and open a new homeworkspace.
+            // This means that properties defined in the previous opened workspace will still be shown in the extension.
+            // CurrentWorkspaceCleared will trigger every time a graph is closed which allows us to reset the properties. 
             this.viewLoadedParams.CurrentWorkspaceCleared += OnCurrentWorkspaceChanged;
             if (linterManager != null)
             {
@@ -128,21 +127,41 @@ public GraphMetadataViewModel(ViewLoadedParams viewLoadedParams, GraphMetadataVi
             InitializeCommands();
         }
 
+        /// <summary>
+        /// This event is triggered when a new workspace is opened or when the current workspace is cleared.
+        /// This event manages state of the workspace properties (ie GraphDescription, GraphAuthor, HelpLink, Thumbnail)
+        /// as well as the custom properties in the extension which do not live in the workspace model.
+        /// </summary>
         private void OnCurrentWorkspaceChanged(Graph.Workspaces.IWorkspaceModel obj)
         {
-            if (!(obj is HomeWorkspaceModel hwm))
+            //Todo review if the workspace properties should be managed in the Workspace model.
+            //Due to the fact that Dynamo often leaves the workspace objects in memory and resets their properties when you open a new workspace,
+            //the management of state is not straightforward. However it may make more sense to update those properties with the clearing logic.
+
+            //Handle the case of a custom workspace model opening
+            if (obj is not HomeWorkspaceModel hwm)
             {
                 extension.Closed();
                 return;
             }
 
+            //Handle workspace change cases in UI.  First is a new workspace or template opening
+            //In this case the properties should be cleared
             if (!hwm.IsTemplate && string.IsNullOrEmpty(hwm.FileName) )
             {
                 GraphDescription = string.Empty;
                 GraphAuthor = string.Empty;
                 HelpLink = null;
                 Thumbnail = null;
             }
+            //Second is switching between an open workspace and open custom node and no state changes are required.
+            //This case can also be true if you close an open workspace while focused on a custom node.
+            //However in that scenario the first case will be triggered first due to empty filename.
+            else if(hwm == currentWorkspace)
+            {
+                return;
+            }
+            //Third is a new workspace opening from a saved file
             else
             {
                 currentWorkspace = hwm;
@@ -152,6 +171,7 @@ private void OnCurrentWorkspaceChanged(Graph.Workspaces.IWorkspaceModel obj)
                 RaisePropertyChanged(nameof(Thumbnail));
             }
 
+            //Clear custom properties for cases one and two.
             CustomProperties.Clear();
         }
 
@@ -216,7 +236,15 @@ private void OpenGraphStatusExecute(object obj)
 
         private void AddCustomPropertyExecute(object obj)
         {
-            var propName = Properties.Resources.CustomPropertyControl_CustomPropertyDefault + " " + (CustomProperties.Count + 1);
+            int increment = CustomProperties.Count + 1;
+            string propName;
+            do
+            {
+                propName = Properties.Resources.CustomPropertyControl_CustomPropertyDefault + " " + increment;
+                increment++;
+            }
+            while (CustomProperties.Any(x => x.PropertyName == propName));
+
             AddCustomProperty(propName, string.Empty);
         }
 

test/DynamoCoreWpfTests/ViewExtensions/GraphMetadataViewExtensionTests.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
@@ -8,6 +8,7 @@
 using Dynamo.Graph.Workspaces;
 using Dynamo.GraphMetadata;
 using Dynamo.GraphMetadata.Controls;
+using Dynamo.Models;
 using NUnit.Framework;
 
 namespace DynamoCoreWpfTests.ViewExtensions
@@ -29,15 +30,15 @@ public void SettingPropertiesInExtensionUpdatesWorkspace()
             var extensionManager = View.viewExtensionManager;
 
             var propertiesExt = extensionManager.ViewExtensions
-                .FirstOrDefault(x => x as GraphMetadataViewExtension != null )
+                .FirstOrDefault(x => x as GraphMetadataViewExtension != null)
                 as GraphMetadataViewExtension;
 
             var hwm = this.ViewModel.CurrentSpace as HomeWorkspaceModel;
 
             // Act
-            var graphDescriptionBefore = hwm.Description; 
-            var graphAuthorBefore = hwm.Author; 
-            var graphHelpLinkBefore = hwm.GraphDocumentationURL; 
+            var graphDescriptionBefore = hwm.Description;
+            var graphAuthorBefore = hwm.Author;
+            var graphHelpLinkBefore = hwm.GraphDocumentationURL;
             var graphThumbnailBefore = hwm.Thumbnail;
 
             propertiesExt.viewModel.GraphDescription = graphDescription;
@@ -107,7 +108,7 @@ public void ExistingGraphWithCustomPropertiesWillBeAddedToExtension()
             // Arrange
             var expectedCP1Key = "My prop 1";
             var expectedCP2Key = "My prop 2";
-            var expectedCP3Key = "Custom Property 3";
+            var expectedCP3Key = "Custom Property 4";
 
             var expectedCP1Value = "My value 1";
             var expectedCP2Value = "My Value 2";
@@ -134,6 +135,48 @@ public void ExistingGraphWithCustomPropertiesWillBeAddedToExtension()
             Assert.That(propertiesExt.viewModel.CustomProperties[2].PropertyValue == expectedCP3Value);
         }
 
+        [Test]
+        public void ExistingGraphWithCustomPropertiesKeepsPropertiesWhenCustomNodesAreOpened()
+        {
+            // Arrange
+            var expectedCP1Key = "My prop 1";
+            var expectedCP2Key = "My prop 2";
+            var expectedCP3Key = "Custom Property 4";
+
+            var expectedCP1Value = "My value 1";
+            var expectedCP2Value = "My Value 2";
+            var expectedCP3Value = "";
+
+            // Act
+            var extensionManager = View.viewExtensionManager;
+            var propertiesExt = extensionManager.ViewExtensions
+                .FirstOrDefault(x => x as GraphMetadataViewExtension != null)
+                as GraphMetadataViewExtension;
+
+            Open(@"core\CustompropertyTest.dyn");
+
+            Open(@"core\CustomNodes\add.dyf");
+
+            ViewModel.UIDispatcher.Invoke(new Action(() =>
+            {
+                DynamoModel.SwitchTabCommand switchCommand =
+                    new DynamoModel.SwitchTabCommand(0);
+
+                ViewModel.ExecuteCommand(switchCommand);
+            }));
+
+            Model.Logger.Log(ViewModel.CurrentSpace.Name);
+
+            // Assert
+            Assert.That(propertiesExt.viewModel.CustomProperties.Count == 3);
+            Assert.That(propertiesExt.viewModel.CustomProperties[0].PropertyName == expectedCP1Key);
+            Assert.That(propertiesExt.viewModel.CustomProperties[0].PropertyValue == expectedCP1Value);
+            Assert.That(propertiesExt.viewModel.CustomProperties[1].PropertyName == expectedCP2Key);
+            Assert.That(propertiesExt.viewModel.CustomProperties[1].PropertyValue == expectedCP2Value);
+            Assert.That(propertiesExt.viewModel.CustomProperties[2].PropertyName == expectedCP3Key);
+            Assert.That(propertiesExt.viewModel.CustomProperties[2].PropertyValue == expectedCP3Value);
+        }
+
         [Test]
         public void ExistingGraphOpenModifiedAndClosedWillSetAndClearModifiedFlag()
         {
@@ -157,5 +200,23 @@ public void ExistingGraphOpenModifiedAndClosedWillSetAndClearModifiedFlag()
 
             Assert.IsFalse(ViewModel.HomeSpace.HasUnsavedChanges);
         }
+
+        [Test]
+        public void AddingNewPropertiesHaveUniquePropertyNames()
+        {
+            var extensionManager = View.viewExtensionManager;
+            var propertiesExt = extensionManager.ViewExtensions
+                    .FirstOrDefault(x => x as GraphMetadataViewExtension != null)
+                as GraphMetadataViewExtension;
+
+            var customPropertiesBeforeOpen = propertiesExt.viewModel.CustomProperties.Count;
+            Open(@"core\CustompropertyTest.dyn");
+
+            propertiesExt.viewModel.AddCustomPropertyCommand.Execute(null);
+
+            Assert.That(propertiesExt.viewModel.CustomProperties.Count == 4);
+            Assert.That(propertiesExt.viewModel.CustomProperties[3].PropertyName == "Custom Property 5");
+            Assert.That(propertiesExt.viewModel.CustomProperties[3].PropertyValue == "");
+        }
     }
 }

test/DynamoCoreWpfTests/ViewExtensions/GraphNodeManagerViewExtensionTests.cs

@@ -214,10 +214,12 @@ public void ContainsEmptyListOrNullTest()
             int emptyListNodesCount = hwm.Nodes.Count(ContainsAnyEmptyLists);
 
             var view = viewExt.ManagerView;
-            var images = WpfUtilities.ChildrenOfType<Image>(view.NodesInfoDataGrid);
 
+            IEnumerable<Image> images = [];
             Utility.DispatcherUtil.DoEventsLoop(() =>
             {
+                images = WpfUtilities.ChildrenOfType<Image>(view.NodesInfoDataGrid);
+
                 int nullNodesImageCount = GetImageCount(images, "Null");
                 int emptyListNodesImageCount = GetImageCount(images, "EmptyList");