Fixing NodewView serializationn properties #13663
Conversation
|
|
|
@jesusalvino Would you set the order for Node Model as well? |
Done. |
|
@jesusalvino Would you share the reason why for NodeModel, you did not use the sequence number as identifier but chose a different implementation? |
Sure, because the NodeModel has serialized properties that are not directly mapped to its C# class, like the "ConcreteType" property and the NodeModel serialized properties could have extra properties based on the different kind of nodes like "Code" property or "InputValue", that's why the order of its serialized properties is handable after its json format, not in the tag order as part of its property in its C# Class like the NodeView, the order of serialization of the NodeModel property is centralized in the NodeModel.PropertiesSerializationOrder. |
| try | ||
| { | ||
| string metaNodesStartKey = "\"Nodes\": [\r\n "; | ||
| string metaNodesEndKey = "\r\n ]"; |
There was a problem hiding this comment.
you should use something cross platform using system environment new line or similar
| { | ||
| string metaNodesStartKey = "\"Nodes\": [\r\n "; | ||
| string metaNodesEndKey = "\r\n ]"; | ||
| int flatNodesStartIndex = metaContent.IndexOf(metaNodesStartKey) + metaNodesStartKey.Length; |
There was a problem hiding this comment.
I think this code has the potential to be very slow, have you profiled the serialization performance of a very large graph before and after this change?
| @@ -2265,6 +2265,11 @@ public virtual string PrintExpression() | |||
| return s; | |||
| } | |||
|
|
|||
| public static List<string> PropertiesSerializationOrder() | |||
| { | ||
| List<string> metaProperties = new List<string>(); | ||
|
|
||
| var rxKeyProperty = new Regex(keyProperty); |
|
There is a lot of complexity in this new code, I did not finish reviewing it. I am curious how much benefit there would be to simply using I'm also concerned about the potential performance impact - we should test it with various size graphs if we're going to proceed with this. |
|
@QilongTang @mjkkirschner unfortunatelly I couldn't test the custom approach with a large .dyn file enclosed here , it stucks my Visual Studio when I try to debug it, so in order to keeping simple I switched to the order approach. |
|
@jesusalvino Glad you were able to solve this using simple order definition. |
Purpose
Fix the bug https://jira.autodesk.com/browse/DYN-5446-serializing-dyn-file
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@QilongTang
FYIs
@RobertGlobant20 @filipeotero