Enable subclass of AllChildrenOfType set output Name #11195
Conversation
src/Libraries/CoreNodeModels/Enum.cs
Outdated
| [JsonConstructor] | ||
| protected AllChildrenOfType(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(outputName, inPorts, outPorts) | ||
| { | ||
| } | ||
|
|
||
| [JsonConstructor] | ||
| protected AllChildrenOfType(string value, IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(value, inPorts, outPorts) |
There was a problem hiding this comment.
hmm - I'm not sure how this constructor will ever be invoked by the deserializer - since the outputName field doesn't get serialized I don't think anything will ever be passed here - also naming it value is probably a good way to confuse json.net - since there might be some other property called value that is serialized.
The second issue I think needs to be tested, is since the output name is not serialized, what happens when you reopen a graph containing one of these nodes that was created with a different outputName, does it deserialize correctly with the right name?
There was a problem hiding this comment.
If what I said makes no sense, give this a read about how json.net uses these json constructors:
https://stackoverflow.com/questions/23017716/json-net-how-to-deserialize-without-using-the-default-constructor
There was a problem hiding this comment.
Thank you for your reply, Michael. Here is a comparison of the old and new nodes in the script, and does not affect the function.
I will update this variable name to avoid Json confusion.
There was a problem hiding this comment.
Hello @ZiyunShang - one comment: The term 'Class' is not familiar to the majority of our user base inside of Dynamo, where as "Type" is familiar coming from the Revit context. It may make it more difficult for users to understand.
There was a problem hiding this comment.
Hi @Amoursol , in RevitAPI, there is a type of Element called "ElementType", which is confusing with this "Type". And after discussing with my team, I think that 'Class' is more in line with the output of this node.
| [JsonConstructor] | ||
| protected AllChildrenOfType(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(outputName, inPorts, outPorts) | ||
| { | ||
| } | ||
|
|
||
| [JsonConstructor] | ||
| protected AllChildrenOfType(string outputName, IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(outputName, inPorts, outPorts) |
There was a problem hiding this comment.
Hi @ZiyunShang I still don't think this makes much sense because outputName is not a serialized property! But I don't think it hurts anything to add this.
I think your screenshot shows exactly my concern - the same node is being loaded one way, and constructed from the library another.
I think you need to change the [JsonConstructor] of the derived class in DynamoRevit to pass "Class" as the output name and call your new constructor base that you added here to get nodes to load correctly.
There was a problem hiding this comment.
@mjkkirschner Maybe I need to wait until DynamoCore Library has this change before updating.
|
@QilongTang any objections to merging this? |
not really go for it. |
Purpose
Refer to DynamoDS/DynamoRevit#2641 .
I want to rename the output name of "Element Types"(will be "Element Classes"). But the
AllChildrenOfTypecan't enable that change.This hard Code makes its inheritance very inflexible.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner @QilongTang
FYIs
@Amoursol