Remove InPortData and OutPortData #7301

ikeough · 2016-10-26T14:27:30Z

Purpose

This PR represents a substantial refactoring of ports and connectors.

This PR removes the InPortData and OutPortData collections on NodeModel. These collections pre-date the InPorts and OutPorts collections, and were previously deprecated. They acted as a way to store initialization data for ports during the construction of the node, with the assumption that at some point after they were filled, NodeModel.RegisterAllPorts would be called, subsequently creating/updating ports on the nodes. This indirection was not obvious or useful and led to the InportData and OutportData collections getting out of sync with the InPorts and OutPorts collections.

In the beginning, collections in Dynamo were not observable, so we created a lot of events like NodeModel.PortConnected and NodeModel.ConnectorConnected when ports and connectors were added. As we have moved to using observable collections, it's much easier to deal with the creation and deletion of ports, connectors, nodes, etc., by simply handling the collection changed events for those collections and doing the right thing. This PR gets us part of the way towards a fully observable methodology by adjusting NodeModel to watch the collection changed events for the InPorts and OutPorts collections, and consolidating logic previously spread among numerous event handlers into those collection change handlers.

Additionally, this PR removes a number of methods which seemed like a good idea three years ago, but now only add confusion. For example. PortModel.Connect was a very thin wrapper around PortModel.Connectors.Add(someNewConnector). These thin wrappers have been removed except in cases where the wrapper contained some assertion logic.

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. (In progress)
Snapshot of UI changes, if any.
Follows Semantic Versioning standards (No change of public methods or types etc..)

Reviewers

@mjkkirschner

…construction.

ramramps · 2016-10-28T15:20:24Z

@ikeough DynamoStorage is using node.inportdata.

ikeough · 2016-10-28T15:43:20Z

@ramramps Thanks! I'll fix that.

mjkkirschner · 2016-10-31T18:58:16Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

@@ -895,6 +876,15 @@ protected NodeModel()
            ArgumentLacing = LacingStrategy.Disabled;

            RaisesModificationEvents = true;
+
+            InPorts.CollectionChanged += PortsCollectionChanged;


these handlers should be removed in the dispose method right?

Ah yes. Nice catch @mjkkirschner.

mjkkirschner · 2016-10-31T18:59:40Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

+        private void PortsCollectionChanged(object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e)
+        {
+            ValidateConnections();
+            ConfigureSnapEdges(sender == InPorts?InPorts : OutPorts);


sender will be a collection? not the modified element?

Yes. The CollectionChanged event's sender is the collection. You can get at the elements that have been changed by using the NotifyCollectionChangedEventArgs.

mjkkirschner · 2016-10-31T19:01:54Z

src/DynamoCoreWpf/ViewModels/Core/PortViewModel.cs

@@ -268,8 +268,8 @@ void _port_PropertyChanged(object sender, System.ComponentModel.PropertyChangedE
                case "Center":
                    RaisePropertyChanged("Center");
                    break;
-                case "DefaultValueEnabled":


what was the reason to get rid of this?

DefaultValueEnabled was removed on the NodeModel, so the change event can be removed here. The property was removed from NodeModel in favor of using the more precise DefaultValue == null.

mjkkirschner · 2016-10-31T19:03:56Z

src/Libraries/CoreNodeModels/HigherOrder/MapCombineLacing.cs

-            InPortData.Add(new PortData("reductor", Resources.ReducePortDataReductorToolTip));
-            InPortData.Add(new PortData("seed", Resources.ReducePortDataSeedToolTip));
-            InPortData.Add(new PortData("list1", Resources.PortDataListToolTip + " #1"));
+            InPorts.Add(new PortModel(PortType.Input, this, new PortData("reductor", Resources.ReducePortDataReductorToolTip)));


just curious... what happens when a zero touch node author adds a PortModel of type Output to the InPortscollection... do we crash?

Nothing. This design is not great and needs to be cleaned up, but I didn't want to do it as part of this PR. We should remove PortType because it doesn't have any real meaning. Whether a port is an "input" or an "output" can be determined by whether it's in the InPorts or the OutPorts collections respectively.

mjkkirschner · 2016-10-31T19:04:46Z

src/Libraries/DynamoArduino/dynArduino.cs

@@ -116,35 +116,6 @@ protected override void DeserializeCore(XmlElement nodeElement, SaveContext cont
                    .Select(subNode => subNode.Attributes[0].Value)
                    .Last();
        }
-
-        //public override Value Evaluate(FSharpList<Value> args)


mjkkirschner · 2016-10-31T19:07:10Z

@ikeough this looks good to me. I have a few questions though, see some of the comments, also, does this effectively break all nodeModelnodes made by third parties, or any other others up until now. For example, the Dynamo Samples repo will need to be updated and Revit UI nodes?

Is there any work necessary for zero touch node authors?

ikeough · 2016-11-01T12:35:36Z

@mjkkirschner Yes. This will break third party nodes, the samples, and Revit. That is why this is a 2.0 change. Fortunately, the InportData and OutportData collections have been marked as deprecated for several versions.

…ort collection change in view model.

…on sync.

- Remove Connect and Disconnect methods in favor of allowing connectors to be added to the Connectors collection. - Consolidate port connect and disconnect handling logic into collection changed handlers. - Remove methods on NodeModel which allow finding whether a port is connected by index, in favor of one method on PortModel, IsConnected. - Fix collapse logic on custom node manager.

ikeough · 2016-11-03T20:10:00Z

src/DynamoCore/Core/CustomNodeDefinition.cs

@@ -50,7 +50,7 @@ public class CustomNodeDefinition : IFunctionDescriptor
            if (outputs.Any())
            {
                topMost.AddRange(
-                    outputs.Where(x => x.HasInput(0)).Select(x => Tuple.Create(0, x as NodeModel)));
+                    outputs.Where(x => x.InPorts[0].IsConnected).Select(x => Tuple.Create(0, x as NodeModel)));


API Change: NodeModel.HasInput(...) has been removed. Use PortModel.IsConnected instead.

ikeough · 2016-11-03T20:11:23Z

src/DynamoCore/Core/CustomNodeManager.cs

@@ -767,7 +771,7 @@ internal bool TryGetNodeInfo(string name, out CustomNodeInfo info)
                        selectedNodeSet.SelectMany(
                            node =>
                                Enumerable.Range(0, node.InPorts.Count)
-                                .Where(node.HasConnectedInput)
+                                .Where(index=>node.InPorts[index].Connectors.Any())


We can't use the PortModel.IsConnected method here because that assumes default values as part of connection. Here we really want to know if the port has any connectors, so we use the more idiomatic Connectors.Any()

ikeough · 2016-11-03T20:12:29Z

src/DynamoCore/Graph/Connectors/ConnectorModel.cs

        /// without raising port disconnection events.</param>
-        internal void Delete(bool silent = false)


The silent parameter was never used.

ikeough · 2016-11-03T20:13:58Z

src/DynamoCore/Graph/Nodes/CustomNodes/CustomNodeController.cs

-            var inputs = Definition.DisplayParameters.Zip(Definition.Parameters, (dp, p) => Tuple.Create(dp, p.Description, p.DefaultValue));
-            foreach (var p in inputs)
-                model.InPortData.Add(new PortData(p.Item1, p.Item2, p.Item3));
+            var inputs = Definition.DisplayParameters.Zip(Definition.Parameters, (dp, p) => Tuple.Create(dp, p.Description, p.DefaultValue)).ToList();


The following logic originally lived on NodeModel in RegisterOutputs(). We move it here for custom nodes so that their ports are not deleted when the underlying function definition changes, but the ports are updated.

ikeough · 2016-11-03T20:16:44Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

@@ -1281,28 +1284,28 @@ public void SelectNeighbors()
        /// Sets the <seealso cref="ElementState"/> for the node based on
        /// the port's default value status and connectivity.
        /// </summary>
-        public void ValidateConnections()


API Change: NodeModel.ValidateConnections() has been removed.

ikeough · 2016-11-03T20:17:51Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

@@ -1387,35 +1390,12 @@ internal double GetPortVerticalOffset(PortModel portModel)
        ///     Reads inputs list and adds ports for each input.
        /// </summary>
        [Obsolete("RegisterInputPorts is deprecated, please use the InPortNamesAttribute, InPortDescriptionsAttribute, and InPortTypesAttribute instead.")]
-        public void RegisterInputPorts()
+        public void RegisterInputPorts(IEnumerable<PortData> portDatas)


API Change: NodeModel.RegisterInputPorts() is now NodeModel.RegisterInputPorts(IEnumerable<PortData> portDatas)

ikeough · 2016-11-03T20:19:23Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

        }

        /// <summary>
        ///     Reads outputs list and adds ports for each output
        /// </summary>
        [Obsolete("RegisterOutputPorts is deprecated, please use the OutPortNamesAttribute, OutPortDescriptionsAttribute, and OutPortTypesAttribute instead.")]
-        public void RegisterOutputPorts()
+        public void RegisterOutputPorts(IEnumerable<PortData> portDatas)


API Change: NodeModel.RegisterOutputPorts() is now NodeModel.RegisterOutputPorts(IEnumerable<PortData> portDatas)

ikeough · 2016-11-03T20:22:01Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

@@ -1603,7 +1559,7 @@ public void RegisterAllPorts()
        /// <param name="data"></param>
        /// <param name="index"></param>
        /// <returns></returns>
-        public PortModel AddPort(PortType portType, PortData data, int index)
+        private PortModel AddPort(PortType portType, PortData data, int index)


API Change: NodeModel.AddPort(...) has been removed. Ports can now be added using NodeModel.InPorts.Add(...)

ikeough · 2016-11-03T20:22:51Z

src/DynamoCore/Graph/Nodes/PortModel.cs

-        /// A <see cref="PortData"/> object used for the construction of the node.
-        /// </summary>
-        [JsonIgnore]
-        public PortData Data { get { return portData; } }


API Change: The PortModel.Data property has been removed.

ikeough · 2016-11-03T20:23:15Z

src/DynamoCore/Graph/Nodes/PortModel.cs

        }

        /// <summary>
        /// Tooltip of the port.
        /// </summary>
        [JsonProperty("Description")]
-        public string ToolTipContent
+        public string ToolTip


API Change: The PortModel.ToolTipContent property is now PortModel.ToolTip.

ikeough · 2016-11-03T20:24:39Z

src/DynamoCore/Graph/Nodes/PortModel.cs

-        /// Controls whether the Use Default Value option is available.
-        /// </summary>
-        [JsonIgnore]
-        public bool DefaultValueEnabled


API Change: PortModel.DefaultValueEnabled has been removed. Please use PortModel.DefaultValue == null to check whether a port has a default value.

I've got one concern here and this is a lot of zt node authors desire to actually use null as a default value... maybe that design decision is out of scope for this PR and this won't actually change the behavior (I dont think it will) just wanted to mention it.

ikeough · 2016-11-03T20:25:44Z

src/DynamoCore/Graph/Nodes/PortModel.cs

+        /// Returns true if the port has connectors or if the 
+        /// default value is enabled and not null. Otherwise, returns false.
+        /// </summary>
+        internal bool IsConnected


API Change: PortModel.IsConnected property is added. This property replaces functionality previously contained in the NodeModel.HasInput and NodeModel.HasConnectedInput properties.

ikeough · 2016-11-03T20:27:42Z

src/DynamoCore/Graph/Nodes/PortModel.cs

-        /// </summary>
-        [Obsolete("Please use NodeModel.HasConnectedInput instead.")]
-        [JsonIgnore]
-        public bool IsConnected


API Change: Obsolete PortModel.IsConnected property has been removed.

ikeough · 2016-11-03T20:28:39Z

src/DynamoCoreWpf/Commands/NodeCommands.cs

@@ -106,18 +106,6 @@ public DelegateCommand ViewCustomNodeWorkspaceCommand
            }
        }

-        public DelegateCommand ValidateConnectionsCommand


API Change: NodeCommands.ValidateConnectionsCommand has been removed.

ikeough · 2016-11-03T20:44:17Z

@mjkkirschner ping. Ready for another round. Call me to chat if you want to know more about some of these refactors.

mjkkirschner · 2016-11-05T14:48:50Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs

-        {
-            foreach (UIElement e in inputGrid.Children)
-            {
-                if (enabledDict.ContainsKey(e))


did anything use this dictionary?

mjkkirschner · 2016-11-05T15:09:53Z

@ikeough this looks pretty good to me.

mjkkirschner · 2016-11-05T15:10:04Z

mjkkirschner · 2016-11-05T19:09:20Z

because this change will effect our internal libraries and many external ones, should we create an issue to track public documentation / update notes on this change and what internal repos still need to be updated?

ikeough · 2016-11-05T19:17:36Z

@mjkkirschner I'll make a project for it :)

ikeough · 2016-11-05T19:21:15Z

Project added: https://github.com/DynamoDS/Dynamo/projects/3. I'll tackle these in short order.

Ian Keough added 3 commits October 19, 2016 20:13

Remove InPortData and OutPortData.

d4e3b0a

Put back Register... methods... for now.

74b5fc7

Remove DefaultValueEnabled property and set UsingDefaultValue during …

b254b15

…construction.

ikeough changed the title ~~Remove InPortData and OutPortData~~ (WIP) Remove InPortData and OutPortData Oct 26, 2016

ikeough mentioned this pull request Oct 27, 2016

Updated version to 2.0.0 #7300

Merged

Merge branch 'master' into PortDataRemoval

50863a4

ikeough assigned mjkkirschner Oct 27, 2016

mjkkirschner reviewed Oct 31, 2016

View reviewed changes

ikeough changed the title ~~(WIP) Remove InPortData and OutPortData~~ Remove InPortData and OutPortData Nov 1, 2016

Ian Keough added 6 commits November 1, 2016 06:15

Merge branch 'master' into PortDataRemoval

12cf222

Remove event handlers on node dispose.

c385034

Simplify connection validation and make internal.

c6b3657

Cleanup.

b294c08

Fix serialization tests and one other.

7bbd405

Don't set State if it's been set to persistent.

dabf2d9

ikeough referenced this pull request Nov 2, 2016

Updated Labels and Descriptions

1160547

Ian Keough added 6 commits November 2, 2016 10:20

Remove attributes which create N/A ports.

a09f528

Fix mistakes in inport vs. outport creation. Handler clear case for p…

acb3963

…ort collection change in view model.

Debug statements.

f41f013

Resize port collections without disconnecting on Custom node definiti…

2bf010d

…on sync.

Put connector destruction in one place.

82a0af0

Fix state setting for list.create.

8c46e76

ikeough commented Nov 3, 2016

View reviewed changes

mjkkirschner reviewed Nov 5, 2016

View reviewed changes

ikeough merged commit afe5b10 into DynamoDS:master Nov 5, 2016

ikeough deleted the PortDataRemoval branch November 5, 2016 19:07

Randy-Ma mentioned this pull request Dec 2, 2016

Fix the issue that warning msg persists after it should be cleared up #7387

Merged

7 tasks

mjkkirschner mentioned this pull request Jan 19, 2018

Undo redo connection failure #8476

Merged

7 tasks

aparajit-pratap mentioned this pull request Jan 23, 2020

DYN-2286: fix for crash upon unresolved node undo #10315

Merged

7 tasks

		/// without raising port disconnection events.</param>
		internal void Delete(bool silent = false)

Remove InPortData and OutPortData #7301

Remove InPortData and OutPortData #7301

Conversation

ikeough commented Oct 26, 2016 • edited Loading

Purpose

Declarations

Reviewers

ramramps commented Oct 28, 2016

ikeough commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Oct 31, 2016 • edited Loading

ikeough commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikeough Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

ikeough Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikeough commented Nov 3, 2016

Choose a reason for hiding this comment

mjkkirschner commented Nov 5, 2016

mjkkirschner commented Nov 5, 2016

mjkkirschner commented Nov 5, 2016

ikeough commented Nov 5, 2016

ikeough commented Nov 5, 2016

ikeough commented Oct 26, 2016 •

edited

Loading

mjkkirschner commented Oct 31, 2016 •

edited

Loading

ikeough Nov 3, 2016 •

edited

Loading

ikeough Nov 3, 2016 •

edited

Loading