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

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

Merged
merged 3 commits into from
Jan 23, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e21a95a
fix for crash upon unresolved node undo
aparajit-pratap Jan 23, 2020
4ebc70f
override Equals in PortModel
aparajit-pratap Jan 23, 2020
4ae9814
add recorded test
aparajit-pratap Jan 23, 2020
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
28 changes: 20 additions & 8 deletions src/DynamoCore/Graph/Nodes/DummyNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Xml;

namespace Dynamo.Graph.Nodes
Expand Down Expand Up @@ -250,11 +251,10 @@ private void LoadNode(XmlNode nodeElement)
OutputCount = Int32.Parse(outputCount.Value);
LegacyNodeName = legacyName.Value;

if (nodeElement.ChildNodes != null)
foreach (XmlNode childNode in nodeElement.ChildNodes)
Copy link
Member

Choose a reason for hiding this comment

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

if childNodes is null - this will throw an exception - I think you should keep the if

Copy link
Member

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/3088147/why-does-net-foreach-loop-throw-nullrefexception-when-collection-is-null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resharper tells me that expression will always be true.

{
foreach (XmlNode childNode in nodeElement.ChildNodes)
if (childNode.Name.Equals("OriginalNodeContent"))
OriginalNodeContent = (XmlElement)nodeElement.FirstChild.FirstChild;
if (childNode.Name.Equals("OriginalNodeContent"))
OriginalNodeContent = (XmlElement) nodeElement.FirstChild.FirstChild;
}

if (originalElement != null)
Expand Down Expand Up @@ -292,18 +292,30 @@ private void LoadNode(XmlNode nodeElement)

private void UpdatePorts()
{
var guids = InPorts.Select(x => x.GUID).ToArray();
InPorts.Clear();
for (int input = 0; input < InputCount; input++)
{
var name = string.Format("Port {0}", input + 1);
InPorts.Add(new PortModel(PortType.Input, this, new PortData(name, "")));
var name = $"Port {input + 1}";
var pm = new PortModel(PortType.Input, this, new PortData(name, ""));
InPorts.Add(pm);
if (guids.Length == InputCount)
{
pm.GUID = guids[input];
}
}

guids = OutPorts.Select(x => x.GUID).ToArray();
OutPorts.Clear();
for (int output = 0; output < OutputCount; output++)
{
var name = string.Format("Port {0}", output + 1);
OutPorts.Add(new PortModel(PortType.Output, this, new PortData(name, "")));
var name = $"Port {output + 1}";
var pm = new PortModel(PortType.Output, this, new PortData(name, ""));
OutPorts.Add(pm);
if (guids.Length == OutputCount)
{
pm.GUID = guids[output];
}
}

RegisterAllPorts();
Expand Down
19 changes: 18 additions & 1 deletion src/DynamoCore/Graph/Nodes/PortModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public enum PortType { Input, Output };
/// <summary>
/// PortModel represents Dynamo ports.
/// </summary>
public class PortModel : ModelBase
public class PortModel : ModelBase, IEquatable<PortModel>
{
#region private fields
ObservableCollection<ConnectorModel> connectors = new ObservableCollection<ConnectorModel>();
Expand Down Expand Up @@ -369,6 +369,23 @@ protected override void DeserializeCore(XmlElement nodeElement, SaveContext cont
}

#endregion

public bool Equals(PortModel other)
Copy link
Member

Choose a reason for hiding this comment

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

last time I tried overriding equals there were a bunch of unexpected failures where reference equality was expected throughout the codebase instead of equality by guid - (I can't remember what type this was on) - just a consideration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I'm checking for guid equality only if reference equality fails. From what I see in other nodemodel types, it looks like the equality check will either be true or false in both cases (reference equality + guid equality) since if the same port model is being reused, both conditions will be true, and if a new port model is created, still both conditions will be false, unless the new ports are created using the same guids as the old ones, which is what I'm enforcing here explicitly for dummy nodes.

{
if (other == null) return false;

if (this == other) return true;

if (GUID == other.GUID) return true;

return false;
}

public override int GetHashCode()
{
return GUID.GetHashCode();
}

}

/// <summary>
Expand Down