[DYN-1454] Unresolved nodes should attempt to be resolved when a new package is loaded #10011

reddyashish · 2019-09-26T05:30:48Z

Purpose

This PR is to resolve the dummy nodes once the package is downloaded.
Task: https://jira.autodesk.com/browse/DYN-1454

Once the package is downloaded, the VM is reset and it reloads all the packages. When this is completed in here: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Engine/EngineController.cs#L505, we attempt to resolve the dummy node's that are present in the current workspace. The ReloadDummyNodes event is triggered and the model will deserialize each of the dummy nodes. If any of them is resolved by the package that was downloaded, then that dummy node will be replaced in the workspace and all the properties that were present before will be set. If they are not resolved, we just skip them. We disable the executions temporarily when the dummy node is replaced by the new resolved node. Once all the dummy nodes are checked, a final run is triggered here: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs#L326.

Once all the dummy nodes are reloaded, a TriggerDependencyRegen is triggered on the Workspace Model from the Dynamo Model. This will regenerate the Dependency table and update the package dependency states on the Workspace Dependency View extension.

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @aparajit-pratap @QilongTang @scottmitchell

This check was needed if any custom path has been added to node path settings in the dynamo session. When dynamo is reopened, it throws an exception while loading the package from the custom path as the workspace object is not null but the filename is null.

…space are reloaded.

aparajit-pratap · 2019-09-26T20:36:55Z

Once all the dummy nodes are reloaded, a TriggerDependencyRegen is triggered on the Workspace Model from the Dynamo Model. This will regenerate the Dependency table and update the package dependency states on the Workspace Dependency View extension.

Do you need to this? Did that not work previously?

reddyashish · 2019-09-26T20:39:45Z

The DependencyRegen was being called as soon as the package is loaded. Then the VM is reset and reloads all the libraries. We have added the Reloading of dummy nodes after the VM is reset so the dependency table was not being updated. So this is needed after we attempt to reload all the dummy nodes.

mjkkirschner · 2019-09-27T01:01:16Z

src/DynamoCore/Engine/EngineController.cs

+        /// <summary>
+        /// This event is fired to reload the dummy nodes.
+        /// </summary>
+        public static event Action ReloadDummyNodes;


think about this when naming events

Do name events with a verb or a verb phrase. Do give event names a concept of before and after, using the present and past tense. For example, a close event that is raised before a window is closed would be called Closing and one that is raised after the window is closed would be called Closed. Do not use Before or After prefixes or suffixes to indicate pre and post events. Do name event handlers (delegates used as types of events) with the EventHandler suffix. Do use two parameters named sender and e in event handler signatures. The sender parameter should be of type Object, and the e parameter should be an instance of or inherit from EventArgs. Do name event argument classes with the EventArgs suffix.

why is this static? Is there a good reason?

thinking more, I think this should be named to represent the state that is happening, not one single method you are using to the event to call...

so something like VMLibrariesReset

Makes sense. Changed the event name accordingly

mjkkirschner · 2019-09-27T01:03:05Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

+        /// <summary>
+        /// This event is to trigger the DependencyRegen on the WorkspaceDependencyview extension.
+        /// </summary>
+        public static event Action TriggerDependencyRegen;


I think the event should not be way to signal just that we want to trigger some other method, but more about the state of the workspace -

like

DummyNodesResolved

why is this static- is there a good reason?

Renamed the event name based on the state and made it non-static.

Actually, this has to be static. Because when this event is attached to the current workspace in here (https://github.com/DynamoDS/Dynamo/pull/10011/files#diff-14bfe53b9085e8f1b47772149559d0a7R135), the workspace object is different. So had to keep it static for now.

@reddyashish couldn't you just attach this handler when the workspace changes?

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

src/DynamoCore/Models/DynamoModel.cs

mjkkirschner · 2019-09-27T01:05:01Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

+        /// <summary>
+        /// Calls the DependencyRegen function when the event is triggered from the dynamo model.
+        /// </summary>
+        internal void TriggerDepenRegen()


whats going on here? Why does this need to live in the view?

This is just a helper method to call the DependencyRegen function.

mjkkirschner · 2019-09-27T01:05:47Z

test/DynamoCoreTests/PackageDependencyTests.cs

+            LoadPackage(packageDirectory);
+
+            dummyNodes = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<DummyNode>();
+            Assert.AreEqual(0, dummyNodes.Count());


Can you also assert the value of some node in the graph which depends on the dummy node being resolved?

Yes, asserting the value of the node in the test.

mjkkirschner · 2019-09-27T01:06:29Z

src/DynamoCore/Models/DynamoModel.cs

+            JObject dummyNodeJSON = null;
+            WorkspaceModel currentWorkspace = this.CurrentWorkspace;
+
+            if (currentWorkspace == null || string.IsNullOrEmpty(currentWorkspace.FileName))


what happens if this is a customNode workspace? Have you tested that? I think we need a test for that case.

src/DynamoCore/Models/DynamoModel.cs

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

src/DynamoCore/Models/DynamoModel.cs

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

test/DynamoCoreTests/PackageDependencyTests.cs

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

src/DynamoCore/Models/DynamoModel.cs

src/DynamoCore/Engine/EngineController.cs

src/DynamoCore/Models/DynamoModel.cs

reddyashish · 2019-09-30T17:32:38Z

I have a bunch of failures on Self CI but they are passing locally. Looking into it more.

reddyashish · 2019-10-02T19:10:46Z

@aparajit-pratap @mjkkirschner The reason for those failures was due to the event in here: https://github.com/DynamoDS/Dynamo/pull/10011/files#diff-14bfe53b9085e8f1b47772149559d0a7R135

I have removed the invocation of DependencyRegen event for now and modified the test to use the dynamo samples and not HowickMaker. Waiting on the self serve results to confirm there are no failures. I will then remove the commented part of the code. Looking into alternatives in the meantime.

mjkkirschner · 2019-10-02T19:40:17Z

@reddyashish this looks like it could have been a memory leak - can you try adding it back and unsubscribing it when the extension is shutdown?

…he extension shuts down.

reddyashish · 2019-10-02T22:30:08Z

Self-serve has passed now. https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/424/

Please take a look and this is ready to be merged.

mjkkirschner · 2019-10-03T04:36:51Z

test/DynamoCoreTests/DummyNodeTests.cs

+            var output = GetPreviewValue("a1aba50a873443f2bfb88480a89e3f36");
+            Assert.IsNull(output);
+
+            // Load the HowickMaker dll and check that the dummy nodes have been resolved.


comment is out of date.

Changed it. Thanks Mike.

mjkkirschner · 2019-10-03T19:25:54Z

src/DynamoCore/Engine/EngineController.cs

+        /// <summary>
+        /// The event notifies client that the VMLibraries have been reset and the VM is now ready to run the new code. 
+        /// </summary>
+        public static event Action VMLibrariesReset;


what do you think about making this internal? Is it useful to anyone but us?

do we want to continue supporting it if we refactor the vm library load logic - ie, could we still raise this event in a way that makes sense if we refactor that flow?

I made this internal for now as it is being using only inside the DynamoCore package. Even after we refactor that workflow, we still might need this event.

mjkkirschner · 2019-10-03T19:28:11Z

src/DynamoCore/Graph/Nodes/DummyNode.cs

@@ -398,5 +404,75 @@ private XmlElement OriginalXmlNodeContent
                return originalXmlElement;
            }
        }
+
+        /// <summary>
+        /// This will return the NodeModel for a dummy node. 


this comment is a bit vague - what do you think about something like:

Deserializes and returns the nodeModel that is represented by the original content of this DummyNode. If this node cannot be resolved, returns a new DummyNode

mjkkirschner · 2019-10-03T19:29:53Z

src/DynamoCore/Graph/Nodes/DummyNode.cs

+                return;
+            }
+
+            resolvedNode.X = dummyNode.X;


one thought I have about this is - is there a way to ensure we are transferring all view properties of the dummyNode - so we don't forget to update this if new view properties are added? Something like checking the ExtraNodeViewInfo object for all properties or something? This is not required - but something to think about.

aparajit-pratap · 2019-10-03T19:30:39Z

test/DynamoCoreTests/DummyNodeTests.cs

@@ -24,6 +25,14 @@ protected override void GetLibrariesToPreload(List<string> libraries)
            base.GetLibrariesToPreload(libraries);
        }

+        private void LoadPackage(string packageDirectory)


I think we can move this function to the base class as it is used in more than on derived class, this one and PackageDependencyTests.

Yes, moved it DynamoModelTestBase class.

mjkkirschner · 2019-10-03T19:31:40Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

@@ -188,6 +188,19 @@ internal int CurrentPasteOffset
        /// </summary>
        private bool workspaceLoaded;

+        /// <summary>
+        /// This event is to notify that the dummy nodes have been resolved. 


what about: This event is raised after the workspace tries to resolve existing dummyNodes - for example after a new package or library is loaded.

mjkkirschner · 2019-10-03T19:37:44Z

test/DynamoCoreTests/DummyNodeTests.cs

@@ -88,5 +97,29 @@ public void WorkspaceIsReadonly_IfXmlDummyNodePresent()
            this.CurrentDynamoModel.CurrentWorkspace.RemoveAndDisposeNode(dummyNode);
            Assert.IsFalse(this.CurrentDynamoModel.CurrentWorkspace.IsReadOnly);
        }
+
+        [Test]
+        public void ResolveDummyNodesOnDownloadingPackage()


please add a test that does this same workflow inside a custom node - IE - the current workspace should be a custom node with dummy nodes in it.

Are there more complex tests you can think of that we might be missing?

dummy nodes connecting to dummy nodes?

a test that asserts the view properties are transferred correctly? I don't see that in your test - ie check the name, position etc.

Added the test for dummy nodes inside a custom node workspace

mjkkirschner · 2019-10-03T19:41:45Z

@reddyashish - looks pretty solid - a few thoughts

please fix up the comments a bit to make them more useful to other devs who are unfamiliar with this work
I think this PR needs more tests.

…omments.

reddyashish · 2019-10-04T15:01:49Z

@mjkkirschner merging this now and will create a followup PR for the view test.

mjkkirschner · 2019-10-04T15:02:41Z

@reddyashish go for it.

reddyashish added 6 commits September 25, 2019 13:10

First commit

336f9b8

Second commit

514c1e0

Adding test

9b884d7

Triggering DependencyRegen event once all the dummy nodes in the work…

9e87dbb

…space are reloaded.

Remove extra line

be104b4