Add the possibility to check if an instance inherits from a given class

[pvalsecc]

It would be cool to be able to do something like that:

var drag = new OpenLayers.Control.DragFeature({});
...
if(OpenLayers.Class.isInstanceOf(drag, "OpenLayers.Control")) {
   ...
}

Please review the attached patch.

[ahocevar]

No need to do this. You can just use plain JavaScript:

if (drag instanceof OpenLayers.Control) {
    ...
}

I'm closing this ticket. Feel free to reopen If I missed something.

[crschmidt]

Yes, you missed that that doesn't work.

map.controls[1].CLASS_NAME instanceof OpenLayers.Control

false

map.controls[1].CLASS_NAME

"OpenLayers.Control.PanZoom"

[ahocevar]

You're doing the check wrong:

map.controls[1] instanceof OpenLayers.Control

is the way to do it, not

map.controls[1].CLASS_NAME instanceof OpenLayers.Control

[crschmidt]

And *also* doesn't work.

>>> map.controls[1] instanceof OpenLayers.Control
false

[crschmidt]

pvalsecc: It seems like storing this as a dictionary is kind of weird: it feels like storing them as a list would be more sensible, since the value of all the keys is just 'true'. Is this done for performance reasons?

[pvalsecc]

Yes, it's done for performance reason.

We could use that in a lot of sensible places like in renderers, formatters, ... So I think it's a good idea to make it as fast as possible by using the correct data structures.

But since OL doesn't have so deep inheritance structures, I could live with lists. If you don't like the hashes, I'll let you change the patch.

[crschmidt]

No, was just curious if you'd thought about it. All I'd really want is a comment mentioning that somewhere...

[ahocevar]

Since we always use foo.CLASS_NAME, why can't we just do this by a substring check on the CLASS_NAME?

instanceOf = function(instance, class) {
    var c = typeof class == "function" ? class.CLASS_NAME : class;
    var i = instance.CLASS_NAME;
    return(i.indexOf(c) == 0 && (i.length == c.length || i.substr(c.length) == "."));
}

Too slow?

[ahocevar]

Just an idea. To me, it still seems that using the javascript instanceof check is the right way to do it. The reason that this does not work is the way we create classes. The following works:

a = function(){};
a.prototype.foo = "bar";
b = new a();
b = function(){};
OpenLayers.Util.extend(b,a);
b.prototype.foo2 = "bar2";
c = new b();

c instanceof a will be true in this case.

[crschmidt]

ahocevar: Substring checks do not tell you that Layer.WFS is an instanceof Layer.Markers.

[ahocevar]

crschmidt: right, I missed that. Hence my new idea (btw, the 3rd line can be removed in that snippet).

[pvalsecc]

ahocevar: let me change a bit your code:

a = function(){};
a.prototype.foo = "bar";
b = new a();
b = function(){};
OpenLayers.Util.extend(b,a);
b.prototype.foo2 = "bar2";
c = new b();
d = function() {};
d.prototype.foo3="bar3";
OpenLayers.Util.extend(d,a);

c instanceof d should be false, no?

JavaScript has no real notion of inheritance for the moment. So I don't see how it could work with instanceof.

[elemoine]

I like this patch. It would be useful to people developing Controls, Strategies, etc. outside OpenLayers. E.g. I have a class defined with

MyNameSpace.MyControl = new OpenLayers.Class(OpenLayers.Control, ...);

then I still be able to test if MyControl objects are instance of OpenLayers.Control.

And given that we can't rely on the JavaScript instanceof construct and that pvalssec's patch is high quality and shouldn't hurt in any away, I'm in favor for this to go into trunk.

[pvalsecc]

New patch attached with the comments suggested by crschmidt.

[pvalsecc]

Where do we stand with this patch?

[elemoine]

i'm +1 on having this patch go in trunk. Andreas, do you still have objections? What about the other developers?

[elemoine]

I personally care about this stuff. OpenLayers, as a framework, offers its users to inherit from its base classes, like Control and Strategy, so it makes sense to me that OpenLayers provide an instanceOf function. Let's leave this ticket open and try to have the other developers' opinion.

[ahocevar]

I am not opposed in general, but I think this can be done with less impact. Patch to come.

[ahocevar]

alternative patch

[ahocevar]

The above patch is my first attempt to do this in a differnet way. The API is a bit different too. So if we e.g. have an object a and want to know if it is an instance of OpenLayers.Layer, then we just say

a.instanceOf["OpenLayers.Layer"]

Obviously this adds an instanceOf property to every prototype. Not sure if this is wise, and also not sure if it is better than pvalsecc's patch. Just an alternative. What do others think?

[ahocevar]

like the above, but instanceOf now also contains the class itself (not just parent classes), an with comments and tests

[ahocevar]

1802-r8441-A1.patch is an alternative to isInstanceOf-V2.patch. Tests pass.

Please note that I am really in favour of adding this functionality, and I am fine with both patches (although I like mine better because it is more convenient to use and also works with custom classes that do not define a CLASS_NAME, which happens frequently).

Please review, and may someone else decide which one to finally put into trunk.

[ahocevar]

Please ignore my above comment.

According to  http://www.3site.eu/doc/, inheritance hierarchies are possible in JavaScript:

function /* class */ MyFirstClass(){};
// add one or more prototype methods

function MyFirstSubClass(){};

// prototypal inheritance in action
MyFirstSubClass.prototype = new MyFirstClass;

var msc = new MyFirstSubClass;

// true
(msc instanceof MyFirstSubClass) && (msc instanceof MyFirstClass)

The problem is just that OpenLayers.Class does not work like this. And that we have multiple inheritance.

Anyway, I think we can address this later, and pvalsecc's patch has no impact on any API stuff, so I'm +1 to use his patch until we can come up with something better.

[ahocevar]

finally! instanceof testing with native javascript

[ahocevar]

The above patch changes OpenLayers.Class to make native JavaScript instanceof tests work throughout the class hierarchy. The only limitation is that a class can only have one superclass. This is solved by the assumption that in

NewClass = OpenLayers.Class(Class1, Class2, proto)

the "real" superclass is Class1. Note that this patch does not change anyting in the way class inheritance works (i.e. we can still do multiple inheritance), it just makes a class know its hierarchy to allow for native JavaScript instanceof checks:

myInstance = new OpenLayers.Layer.WMS();
myInstance instanceof OpenLayers.Layer.WMS; // true
myInstance instanceof OpenLayers.Layer.Grid; // true
myInstance instanceof OpenLayers.Layer.HTTPRequest; // true
// and so on

The patch also contains unit tests, all tests pass in FF2, FF3, IE6, IE7, Opera 9.5, Safari 3.1. Please review.

[elemoine]

Looks good Andreas!

I added a few tests:

t.ok(bad instanceof BadClass, "bad is a BadClass");

this one passes.

And:

var Cls = OpenLayers.Class(OpenLayers.Control);
var obj = new Cls();
t.ok(obj instanceof Cls, "obj is a Cls");
t.ok(obj instanceof OpenLayers.Control, "obj is a Control");

The last test fails, what do think? Do you think doing OpenLayers.Class(OpenLayers.Control) is so stupid that we should not expect that test to pass?

[pvalsecc]

IMHO, double inheritance should be supported. OL uses it...

[ahocevar]

elemoine: your last test fails because of the API of OpenLayers.Class: if you pass only one argument, this is considered the prototype of a new class. So yes, doing OpenLayers.Class(ExistingClass); is stupid, because it does not follow the API of OpenLayers.Class.

pvalsecc: Handling multiple inheritance using native JavaScript instanceof reflection is not possible. In JavaScript, every class can only have one superclass. Also, for the use cases mentioned in the comments for this ticket, what I have now should be sufficient. I see OpenLayers multiple inheritance more like "inherit from one superclass, add functions/properties from another class (or a couple of other classes), and create the prototype of the new class".

[pvalsecc]

I checked the few double parent class OL has and this limitation looks fine by me. It's true that the second classes are all mixins not really double inheritance.

Therefore the new patch fits my needs and looks good. I'd say: commit, but I don't have this power ;-)

Thanks.

[elemoine]

I agree with the mixin thing. Andreas, this is really good work, please commit.

[ahocevar]

(In [8445]) Made instanceof reflection work for class hierarchies. r=elemoine,pvalsecc (closes #1802)