BHoM / Grasshopper_UI

Tools for Grasshopper
GNU Lesser General Public License v3.0
16 stars 5 forks source link

Automatically generated Create() method does not assign default values for Reference type properties #528

Closed alelom closed 3 years ago

alelom commented 4 years ago

Steps to reproduce/description.

Select an auto-generated Create() method for an object that has some Reference-type properties whose default value is set like:

public List<string> SomeList { get; set; } = new List<string>() { "Some default value1", "Some default value 2" };
public bool SomeBoolean { get; set; } = true;

in the output object, the value will not be set for SomeList (an empty list is returned instead), but it will be true for SomeBoolean and any other Value type.

An example is the BH.oM.Diffing.DiffConfig: image

where this happens: image

This creates an inconsistency and some issues:

  1. I am forced to store the default values for Reference type properties in a "manual" Create() method. In my opinion, we should be consistent and store all default values in the same place. So I am tempted to remove all default values from the oM, but that would cause an even worse situation for point 2.

  2. Users of the Diffing need to be aware that instantiating a DiffConfig through the auto-generated Create() do not populate some properties with default values. If the auto-generated DiffConfig is used, it results in a behaviour of the diffing which is counter-intuitive for the users and leads them to think that "something is wrong" with the diffing.

  3. Personally, I've always liked to think of the auto-generated Create() method as the "parameterless constructor of the BHoM_UI". However, this inconsistency makes it not so. Happy to change my mind, but we still need to be consistent with where we decide to put default values for all types of properties, and I would like for the auto-generated Create() method to actually populate all default values.

@al-fisher @adecler

adecler commented 4 years ago

@alelom , Agreed this is a bug.

The auto-generated constructor should work the same way as new TypeToInstanciate { X = x, Y = y } (this is why we use that notation in the UI). So it should only replace the parts that have been assigned by the user.

I expect this is a bug linked to the ways default values are assigned to a component and how they are all used to modify the created object instead of only using the ones that the user provided. Notice on that level that the input PropertiesToIgnore is also an empty list on the component.

I'l have a look into it.

adecler commented 4 years ago

OK, the problem is in Grasshopper, not in the UI. All works well until this line:

https://github.com/BHoM/Grasshopper_Toolkit/blob/c00a861bd6aa7a145c3d803d1570c38a9295fb7b/Grasshopper_UI/Templates/CallerComponent.cs#L428

Two problems then occur:

image

To clarify, what you see there is a string representation of the list, not a list. I have tried to pass that as dynamic as well to SetPersistentData but it still comes out as the string representation of the list. It is like Grasshopper wants to treat it as an object and not as a list so it just applies ToString to the whole thing (as it is a GH_String` goo).

I am running out of time today so feel free to have a look at it yourself. In the mean-time I am transferring this to Grasshopper toolkit.