Bug: improperly cloned expandable object

[ETFVoyageur]

An expandable property uses an expandable object. At times NinjaTrader needs to clone that object. It does so by directly creating a new instance -- calling the object's constructor and then setting the new object's public properties. THAT IS WRONG!!!

There is a good reason for copy constructors and Clone() methods. It is just plain wrong to attempt to clone an object by constructing a new one and then setting its public properties that you know about. You have absolutely no way to guarantee that you know enough to clone an object that way -- because in the general case you don't know enough. You MUST use a copy constructor or a Clone() method.

Neither of those methods exist for an expandable object being used as an expandable property. There is, however, a functional equivalent -- create a new instance of the indicator and call the expandable property's get() method. That is guaranteed to return a valid instance of the expandable object. At that point you can set the new object's public properties to anything you like. The important point is that you MUST use get(), not operator new, to clone such an object.

The following code shows the bug. The comments at the top contain detailed instructions for reproducing the problem. If they are not clear enough, let me know and I'll improve whatever is not clear enough. I do not think the file requires anything from my environment. If I overlooked some dependency, let me know and I'll fix it.

The general idea is:

• Create a chart template that includes the test indicator
• Create a new chart using that template
• As part of creating that chart, NT will both make a bogus clone of the object, and then set the bogus clone into an instance of the indicator

[NinjaTrader_Brett]

I did not look fully into your test case and can revisit if needed but I believed we needed to get on the same page from your first paragraph before going further.

By default when we clone an object we simple clone it using the objects Clone() method. Please review this clone() method: http://ninjatrader.com/support/helpG...-us/?clone.htm

Our default clone method is setup and works as you have found which is to set public properties on clone which works in the majority of use cases and removes the complexity of cloning from normal strategy/indicator developers. However that wont work in all cases as you pointed out (only the simple basic properties) and if you need fine control over how an object gets cloned and the default behavior doesn't work in your case you are free to override the clone() method and set your own custom clone code to change the cloning behavior to suit your needs. Sounds like you need a custom Clone() method to me and that will solve your problem.

If I'm missing something please let me know.

-Brett

[ETFVoyageur]

Brett,

Thanks for the comment. I do still have some questions.

First of all, I agree that the current default behavior is suitable in most cases. It is good that NT makes it so simple to have. I am also glad to see the recognition that the simple behavior is not always enough.

Reading the documentation page your link points leaves me with questions. Is there a requirement that an expandable object be a subclass of some NinjaScript class? I was unaware of that, but I could see it making good sense. The documentation page talks about overriding Clone(), which presumes a known base class, but does not tell what that base class is.

I would agree that if NT were to call a Clone() method that my class provides it would address my current situation. I'm just not aware of what is required for me to do that.

--EV

[NinjaTrader_Brett]

Sure, the Clone method is on the NinjaScript class which is the base class of all NinjaScript including indicators/strategies. So you'll want to override it in your indicator class.

Something like public override object Clone() {

};

Where there you will create the new instance, copy over / clone as you see fit to the new instance and then return that instance.

-Brett

[ETFVoyageur]

I'm not with you. We are not talking about cloning an indicator. We are talking about cloning the expandable object that is the value of a property in an indicator. It is not currently a subclass of anything. If it should be, please let me know and/or point me to the relevant documentation.

Thanks,
--EV

[NinjaTrader_Brett]

NT only clones NinjaScript objects. So thats as far as we reach.

If I was in your shoes in the Indicator clone I would take the action needed to clone that 'custom' object used as part of the indicator you've created. If you wanted to keep on going with OOP then you needed to add IClonable to your object and implement clone method on it to 'clone itself' then call Object.Clone() inside the clone method of NinjaScript that is being cloned.

[ETFVoyageur]

NT only clones NinjaScript objects. So thats as far as we reach.

If I was in your shoes in the Indicator clone I would take the action needed to clone that 'custom' object used as part of the indicator you've created. If you wanted to keep on going with OOP then you needed to add IClonable to your object and implement clone method on it to 'clone itself' then call Object.Clone() inside the clone method of NinjaScript that is being cloned.

I guess I am still not making myself clear.

"in the Indicator clone" -- stop right there. NT is calling the expandable object constructor directly. The indicator is not in the picture. The indicator is not being cloned at that time. Cloning my indicator works just fine today. The problem comes when NT tries to clone the expandable object directly, totally out of context of any indicator.

--EV

[bltdavid]

An expandable property uses an expandable object.
...

Am I right in seeing that your example code shows a class with nested expandable properties?

I see that BarNumbering is the class that you want expanded, but is the SimpleFont property of BarNumbering also a class that is supposed to be expanded, too?

I'm probably way off-base. But all the properties in my classes that I've expanded have always been very simple types, ie, ints, doubles, strings, bools, enums, etc.

I wonder if SimpleFont (being a more complex type) is somehow related to the behavior you're seeing.

Maybe the BarNumbering class should override Clone() for itself and call 'new SimpleFont' to establish a different reference for the Font property before returning a new BarNumbering object?

Perhaps that would resolve the issue.

[ETFVoyageur]

Am I right in seeing that your example code shows a class with nested expandable properties?

Yes. It displays and works fine, modulo some issues I have reported such as not activating the "Apply" button when the value is changed.

I see that BarNumbering is the class that you want expanded, but is the SimpleFont property of BarNumbering also a class that is supposed to be expanded, too?

Yes. In real life I include my expandable font picker class. Works per my preceding remarks. (I used SimpleFont in the demo to avoid dependencies on my other code.)

I'm probably way off-base. But all the properties in my classes that I've expanded have always been very simple types, ie, ints, doubles, strings, enums, etc.

I would expect that is more common, and you notice that I have those as well. I see no reason to be limited to them, however. The included expandable things work well per my preceding remarks.

I wonder if SimpleFont (being a more complex type) is somehow related to the issue.

Perhaps, but the connection is not obvious to me -- the problem is that the logic for how to do a clone of the expandable object class is too simplistic. Note that for indicators they have recognized that the simplistic way is often good enough, so they default to that. But they have also recognized that the simplistic way will not be adequate in some cases, so they provide Clone(). The expandable object class needs to be thought of the same way but, as far as I know, there is no such capability.

Maybe the BarNumbering class should override Clone() for itself and call 'new SimpleFont' to establish a different reference for the Font property before returning a new BarNumbering object?

Perhaps that would resolve the issue.

"overriding Clone()" implies there is a base class with a Clone() to override. BarNumbering has no base class. If it should have one, then please inform me. I have not seen that documented anywhere.

--EV

[bltdavid]

Maybe the BarNumbering class should override Clone() for itself and call 'new SimpleFont' to establish a different reference for the Font property before returning a new BarNumbering object?

What I mean is, perhaps BarNumbering should implement ICloneable ...

https://msdn.microsoft.com/en-us/library/system.icloneable.clone(v=vs.110).aspx

[ETFVoyageur]

If BarNumbering does implement ICloneable, who is going to call Clone()? Are you saying that NT would notice that and call Clone()?

[bltdavid]

If BarNumbering does implement ICloneable, who is going to call Clone()? Are you saying that NT would notice that and call Clone()?

I'm presuming the .NET framework would see it, yes.

[ETFVoyageur]

NT (Brett)? Is he correct? If my expandable object implements ICloneable and overrides Clone() will NT then call Clone()? That would indeed solve the problem.

As I see it, what is needed is one of two things:

1) Some way for the expandable object class to provide Clone() that NT would be aware of and would call

2) For NT to get its clone instance by calling get() on the indicator's property and then proceed as now. This is a bit of a hack, and not as good as Clone(), but would work as things are today.

--EV

[ETFVoyageur]

The object browser does say that SimpleFont implements ICloneable, so I'm going to give that a try. If anyone already knows that will not work, then please speak up.

--EV

[ETFVoyageur]

ICloneable is a failed experiment. It needs to be there anyway, because the indicator needs to call BarNumbers.Clone() when the indicator is cloned.

But the basic problem remains -- NT calls the BarNumbering constructor, not its Clone() method..

OK -- no progress so far. Until someone shows me what I am doing wrong, it appears that NT has a serious bug -- it, as the original subject suggests, clones the expandable object improperly. It needs to adopt the same logic as for indicators -- simple is good when you can get away with that, but Clone() support is required for the remaining cases.

I have attached the updated demo program with ICloneable implemented for both the indicator (AAExpandablePropertyDemo) and the expandable object (BarNumbering)

--EV