Fix editor can write invalid config options (e.g. D3D9 gfx driver for Linux)

[ericoporto]

Fix #2206

My approach to cloning the RuntimeSetup may be a bit unusual but it was the only idea I had as I believe assignment in C# will be only a reference. The other approach would be to explicitly copy each field.

[ivan-mogilko]

I think cloning may be done using reflection, about same way as automatic serialization is done in SerializeUtils: run through all properties that have setters, optionally skip properties with certain attributes (or not?), and copy property values. That might result in a similar effect, but skipping writing and parsing XML doc in memory.

If you add Clone method, probably it makes sense to add ICloneable to the list of interfaces this class implements.

[ericoporto]

it makes sense to add ICloneable to the list of interfaces this class implements.

There's a weird note in .net docs to not put ICloneable in public APIs (at the last sentence here).

[ivan-mogilko]

There's a weird note in .net docs to not put ICloneable in public APIs (at the last sentence here).

Hmm, I see...

This sais:

Because callers of Clone() cannot depend on the method performing a predictable cloning operation, we recommend that ICloneable not be implemented in public APIs.

But to think about this, Is not the same true for this Clone method? It does not seem to guarantee that all fields are cloned, does not let choose whether to do shallow or deep copy. Actually, since the current implementation secretly uses xml serialization, the result would probably be a sort of a deep copy, since any referenced sub-objects will be recreated (although this will probably not make a difference for RuntimeSetup class).

EDIT: what I mean, maybe it will make more sense to write a generic separate method for cloning like this, which could be called on any similar type, but it will be more dependable on what it does.

[ericoporto]

@ivan-mogilko , here is what I did

• I added a test for RuntimeSetup, the real focus is I wanted to test Cloning it, so a lot of other things there (like serialization) is not tested.
• for rewriting Clone()
  • I understood that I had to create a generic way for cloning, but I could not quite figure how (RuntimeSetup doesn't have an empty constructor for instance), so I instead created a method for copying properties from one object to the other in the Utilities unit. This method lacks some safety checks - objects of different types will result in things going wrong...
  • I then rewrote RuntimeSetup Clone method to use it.

I think better understanding of what cloning mean will have to be exercised at a later time when solving #601.

[ivan-mogilko]

I just want to clarify: since we are adding a method to the public interface, then we must clearly specify what is expected from this method, and this expectation has to be kept in the future (and likely repeated for each similar class, since this is a universal kind of function).
If you're certain about this now then fine, but there has to be well defined rule for this method, like, which properties does it copy, does it create new instances of sub-objects (like for properties that are containers, and others) or copies a reference, and so forth.
If not, then it's better to not add anything in the class itself, and use external helper method(s) for this for the time being.

I understood that I had to create a generic way for cloning, but I could not quite figure how (RuntimeSetup doesn't have an empty constructor for instance)

That is true, constructing a new object depends on a use-case then. But property-copying function that you wrote seems fine, and it accepting an object as an argument in line with DeserializeFromXML.

[ericoporto]

If you're certain about this now then fine,

Oh, I am not certain, no, as I mentioned I think a thorough investigation of Clone is necessary for #601, but I don't want to commit to it at this point, I just want to solve #2206.

I will try to hide this inside BuildTargetBase, but keep it public (in Editor project) just for testing. I will look into this later tonight.

In general for #601 I expect we will figure that clone can't always make everything the same since some fields are expected to be unique (scriptName adds a sequential number at end or IDs that will get the next available) - or maybe we will have to do this in two steps to keep identifiers unique, since it depends on objects other than itself, so it needs the Editor for the information. But currently this is not something I know enough for all objects, only that for RuntimeSetup specifically a shallow copy is indeed the best approach.

[ivan-mogilko]

I will try to hide this inside BuildTargetBase, but keep it public (in Editor project) just for testing

I recall that there's a "internal" access modifier which makes things be seen only within an assembly, idk if this will help here (are tests located in a separate one?)

[ericoporto]

Did as I mentioned and hidden the cloning in BuildTargetBase.

Went ahead and squashed around the commits. Previous commits are in fix-editor-config-invalid-bkp branch.