fix(Model): remove valid check from add method

[FreyLuis]

I would like to remove the HasValidData check from the Add(IGeoObject... method on the model.

Add(GeoObjectList... & Add(IEnumerable<IGeoObject>... both don't check for this. The Model can contain invalid geometries anyway, so this check just leads to unexpected behavior.

[dsn27]

Thanks for opening this and explaining the motivation behind removing the HasValidData check on Add(IGeoObject…). I agree that something about this should be addressed, especially since the other overloads (Add(GeoObjectList…), Add(IEnumerable…)) don’t behave the same way.

That said, I’m not really in favor of removing the check entirely. It was likely added for a reason, and removing it outright might reintroduce subtle issues unless we’re sure it’s safe. Consistency between overloads is important, but so is maintaining the intended safeguards.

I think a better approach could be to align the behavior between the different Add methods while still trying to keep the model clear of invalid data. That way we can make the behavior predictable without losing the integrity checks that are there for a reason.

[FreyLuis]

@dsn27 That would still allow invalid geometries in the Model, since i could always add a valid geometry and then change it to an invalid state. The idea, behind removing this check is, to allow importing invalid data into cadability which the user then can fix afterwards.

If you would still like to keep the check, could we add a AddUnsafe method, or some other way to add invalid geometries?

[SOFAgh]

This "HasValidData()" has been introduced to avoid interactive construction of "invalid" GeoObjects. You can for example draw a line with identical start- and endpoint. And this line will not be added to the Model because of this HasValidData() check. It should be the responsibility of the interactive ConstructActions (and there are many) not to call Model.Add when the constructed object is kind of invalid (circle with radius 0, path where the segments are not connected). And I don't know, whether such an object could cause an exception somewhere else. Of course using the Model.Add from your code, it is your responsibility to add well behaving objects only. I tested this by drawing such a line and it would have been added if it had not been rejected by HasValidData(). Not all GeoObjects implement HasValidData, so this is not consistent anyhow.

[stefan-tb]

This "HasValidData()" has been introduced to avoid interactive construction of "invalid" GeoObjects.

Hello. While i get this, silently dropping the .Add operation is probably the most fragile way of dealing with invalid data. I would be fine with keeping the check but at least throw an exception. The application should NOT call .Add if the geo is invalid (because it has to know currently that the geo is not ending up in the List because its invalid in the first place).

[FreyLuis]

@dsn27 @SOFAgh I can see that a construction shouldn't add invalid geometries, but that doesn't prevent the model from containing invalid geometries, since the user can always make a line invalid by editing the data in the property editor.

So any piece of code that expects valid geometries can't rely on the model to ensure that.

The check in the .Add method also means that an importer can't add any invalid geometries, not giving the user a chance to correct them after importing.

[SOFAgh]

In my view, this is a minor problem.
As I mentioned above, "HasValidData()" has been introduced to avoid interactive construction of "invalid" GeoObjects.
Most construct action work in this way: there is an active object, which is created and modified by the action. When all necessary inputs have been set, Action.OnDone() ist called and the default implementation adds the active object to the model. So we could check, whether the object is valid, before adding it to the model. In
public virtual void OnDone()
there is the condition
if (activeObject != null && showActiveObject)
where we could add && activeObject.HasValidData(). In all standard construct actions, this would prevent adding invalid objects. All other actions would have to make sure, that only valid objects are being added.
Stopping the user to later make objects invalid by setting properties like startpoint==endpoint is a lot of work. I looked at ShowPropertyLine. Here in the lines

 endPointProperty = new GeoPointProperty(Frame, "Line.EndPoint");
            endPointProperty.OnGetValue = () => line.EndPoint;
            endPointProperty.OnSetValue = (value) => line.EndPoint = value;

you would have to check in OnSetValue, whether the value can be used as an endpoint. There are many such unchecked property changes.

[stefan-tb]

I agree, it is a minor problem. We just think it is strange internal / blackbox behavior that users of the library don't expect. If I call .Add(...), I either expect the add to happen or an exception if the library has any complaints about the item about to be added. In no case I expect a silent drop of the .Add(...) operation.

If this PR doesn't go thru, we will simply use Add(IEnumerable...) which doesn't show this behavior (and I like that irony ;).