DYN-2149 DummyNode unresolved node messages are confusing #10051

reddyashish · 2019-10-11T06:52:51Z

Purpose

This PR is to improve the warning message shown on the unresolved dummy nodes.
Task: https://jira.autodesk.com/browse/DYN-2149

The description of the node is shown when the user hovers on the node. When the node is of type 'unresolved', a warning message is shown that specifies the function name.

Before this change, the message was:

After this change,

The assembly name is not stored in the object so instead we show the whole function name(along with the namespace).

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

@DynamoDS/dynamo

QilongTang · 2019-10-11T17:42:15Z

src/DynamoCore/Graph/Nodes/DummyNode.cs

+            InputCount = inputCount;
+            OutputCount = outputCount;
+
+            string legacyName = "Unresolved";


@mjkkirschner @aparajit-pratap I cant remember, do we localize node names?

Just found out that we do not localize node names, so we should be fine here

QilongTang · 2019-10-11T17:49:48Z

src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

@@ -96,11 +96,22 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist
        {
            NodeModel node = null;

+            String typeName = null;
+            String functionName = null;


I would usually recommend to use String.Empty because it is less likely to cause crashes. But I saw you have wrapped string null check lots of places which is also good.

mjkkirschner · 2019-10-11T18:26:28Z

@reddyashish - I think something we neglected to consider or discuss yesterday was DummyNodes which represent explicit NodeModel nodes - for example, a dropdown node - here the type and assembly information can actually be gathered from the $type correctly

I think we need to handle both these cases, and this means we need different constructors for dummyNodes - one where we use the function name gathered from FunctionSignature - this will only exist for zero touch nodes - and another constructor and message where the node Type and assembly are actually known.

Have you considered that other case and ensured it is not broken now?

mjkkirschner

let's improve the zero touch dummy node messaging - as you have done here
let's keep the NodeModel dummy node messaging as it is - using type and assembly - as those will be known for NodeModel nodes.
tests for both cases.

mjkkirschner · 2019-10-15T01:34:40Z

src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

-               if (typeName.Contains("ZeroTouch"))
-               {
-                    // This assemblyName does not usually contain version information...
+                if (typeName.Contains("ZeroTouch"))


can we be specific about this- it's possible anyone could make a nodeModel and name it ZeroTouch lets use our entire namespace and type name - including the assembly name would be fine as well for precision.

mjkkirschner · 2019-10-15T01:36:24Z

src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

-                    // This assemblyName does not usually contain version information...
+                if (typeName.Contains("ZeroTouch"))
+                {
+                    // If it is a zero touch node, then get the whole function name including the namespace.
                    functionName = obj["FunctionSignature"].Value<string>().Split('@').FirstOrDefault().Trim();


do FunctionSignatures always have the @ token? What if it's not there, what happens?

reddyashish · 2019-10-15T04:10:51Z

Added the typename info for the node model node. Now the message for node model node looks like this:

@mjkkirschner If the function signature has not parameters(no '@'), it just takes the complete name. Have tested this case.

reddyashish · 2019-10-15T05:18:23Z

Also have checked for the complete namespace for xero-touch nodes. The assembly name is stored as DynamoCore in the type string. So was just checking for the namespace.

mjkkirschner · 2019-10-15T21:25:39Z

src/DynamoCore/Graph/Nodes/DummyNode.cs

+        /// <summary>
+        /// Type name of the node.
+        /// </summary>
+        public string TypeName { get; private set; }


any reason not to overload the LegacyNodeName - maybe note that this is property is only valid for NodeModel dummy nodes?

mjkkirschner · 2019-10-15T21:26:01Z

src/DynamoCore/Graph/Nodes/DummyNode.cs

+        /// <summary>
+        /// Returns the node's function name
+        /// </summary>
+        public string FunctionName { get; private set; }


any reason not to overload the LegacyNodeName - maybe note that this is property is only valid for ZeroTouch dummy nodes?

mjkkirschner · 2019-10-15T21:26:48Z

src/DynamoCore/Graph/Nodes/DummyNode.cs

                }
                else
                {
-                    const string format = "Node of type '{0}' ({1}) cannot be resolved";
-                    return string.Format(format, LegacyNodeName, LegacyAssembly);


is LegacyNodeName used anywhere anymore? If not maybe you want to obsolete it?

mjkkirschner · 2019-10-15T21:27:29Z

@reddyashish it looks good, thanks for the hard work, just a few last comments.

reddyashish · 2019-10-16T13:35:08Z

Added more comments. The LegacyNodeName and LegacyFullName are being used in some other places so I din't remove or obsolete it.

First commit

759041c

QilongTang reviewed Oct 11, 2019

View reviewed changes

mjkkirschner self-requested a review October 11, 2019 18:28

mjkkirschner requested changes Oct 11, 2019

View reviewed changes

Change the warning message for the node model nodes

d983d58

mjkkirschner reviewed Oct 15, 2019

View reviewed changes

Displaying the type name of the node model node.

a8c992f

Using the whole namespace to check for zerotouch nodes.

d14b311

Adding test

69d0cab

mjkkirschner approved these changes Oct 15, 2019

View reviewed changes

mjkkirschner reviewed Oct 15, 2019

View reviewed changes

some more comments

2de8a87

reddyashish merged commit 52557ec into DynamoDS:master Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-2149 DummyNode unresolved node messages are confusing #10051

DYN-2149 DummyNode unresolved node messages are confusing #10051

reddyashish commented Oct 11, 2019 •

edited

Loading

QilongTang Oct 11, 2019 •

edited

Loading

QilongTang Oct 11, 2019

QilongTang Oct 11, 2019

mjkkirschner commented Oct 11, 2019 •

edited

Loading

mjkkirschner left a comment

mjkkirschner Oct 15, 2019

mjkkirschner Oct 15, 2019

reddyashish commented Oct 15, 2019 •

edited

Loading

reddyashish commented Oct 15, 2019

mjkkirschner Oct 15, 2019 •

edited

Loading

mjkkirschner Oct 15, 2019

mjkkirschner Oct 15, 2019

mjkkirschner commented Oct 15, 2019

reddyashish commented Oct 16, 2019 •

edited

Loading

DYN-2149 DummyNode unresolved node messages are confusing #10051

DYN-2149 DummyNode unresolved node messages are confusing #10051

Conversation

reddyashish commented Oct 11, 2019 • edited Loading

Purpose

Declarations

Reviewers

QilongTang Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

QilongTang Oct 11, 2019

Choose a reason for hiding this comment

QilongTang Oct 11, 2019

Choose a reason for hiding this comment

mjkkirschner commented Oct 11, 2019 • edited Loading

mjkkirschner left a comment

Choose a reason for hiding this comment

mjkkirschner Oct 15, 2019

Choose a reason for hiding this comment

mjkkirschner Oct 15, 2019

Choose a reason for hiding this comment

reddyashish commented Oct 15, 2019 • edited Loading

reddyashish commented Oct 15, 2019

mjkkirschner Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Oct 15, 2019

Choose a reason for hiding this comment

mjkkirschner Oct 15, 2019

Choose a reason for hiding this comment

mjkkirschner commented Oct 15, 2019

reddyashish commented Oct 16, 2019 • edited Loading

reddyashish commented Oct 11, 2019 •

edited

Loading

QilongTang Oct 11, 2019 •

edited

Loading

mjkkirschner commented Oct 11, 2019 •

edited

Loading

reddyashish commented Oct 15, 2019 •

edited

Loading

mjkkirschner Oct 15, 2019 •

edited

Loading

reddyashish commented Oct 16, 2019 •

edited

Loading