OpenLayers OpenLayers

Ticket #823 (closed feature: fixed)

Opened 1 year ago

Last modified 1 year ago

simplify class stuff

Reported by: tschaub Assigned to: tschaub
Priority: minor Milestone: 2.5 Release
Component: general Version: 2.4
Keywords: Cc:
State:

Description

All the extra typing to define classes deal with inheritance is a bother.

I like:

var MyClass = new OpenLayers.Class(Parent1, Parent2, {
    // class def here
});

Instead of:

var MyClass = new OpenLayers.Class.create();
MyClass.prototype = 
  OpenLayers.Class.inherit(Parent1, Parent2, {
    // class def here
});

Attachments

class.patch (10.2 kB) - added by tschaub on 07/09/07 19:32:21.
simple class syntax
class.2.patch (71.3 kB) - added by tschaub on 07/16/07 12:58:58.
new class syntax applied everywhere and tested
Format.XML.class.patch (503 bytes) - added by fredj on 09/13/07 03:19:59.
fix OpenLayers.Format.XML class declaration

Change History

07/09/07 18:52:22 changed by tschaub

Most of the patch is documentation, tests, and deprecated stuff. The meat of the new class syntax is in these lines:

OpenLayers.Class = function() {
    var Class = function() {
        this.initialize.apply(this, arguments);
    }
    var extended = {};
    var parent;
    for(var i=0; i<arguments.length; ++i) {
        if(typeof arguments[i] == "function") {
            // get the prototype of the superclass
            parent = arguments[i].prototype;
        } else {
            // in this case we're extending with the prototype
            parent = arguments[i];
        }
        OpenLayers.Util.extend(extended, parent);
    }
    Class.prototype = extended;
    return Class;
}

07/09/07 19:32:21 changed by tschaub

  • attachment class.patch added.

simple class syntax

07/09/07 19:33:12 changed by tschaub

  • keywords set to review.

Don't know what is up with no HTML preview on that patch.

Apply it if you dare. Approve it if you like.

07/12/07 19:17:55 changed by crschmidt

This patch doesn't include the toString hacks we had in place before. I don't have IE handy, but I'm thinking this might fail tests in IE for that reason. If that is fixed (or proven to be unneccesary) then this can probably go in, but I would need to run the tests in IE (or have someone else assure me of same) before I'd mark it commit.

07/12/07 23:32:30 changed by euzuro

I like this patch. A few things:

  • Foo = new OpenLayers.Class() or Foo = OpenLayers.Class() ?? seems to me that the "new" there is not necessary. Usage of it in tests, however, is inconsistent.
  • What cr5 said above is a good point. The code should be copied over. Not only that, but we should design a test for that bug, since none now exists.
  • Application! I think that the patch is good and cool and so if we're going to accept it, the patch should go through each class definition and replace it with the new style. This will be the best way of really testing the patch thoroughly.
  • this.superclass - Even though it becomes complicated in the case of multiple-inheritance, I still think it is worth it to add this feature. SDE and I started in on a sandbox to do this but never finished it. Just off the top of my head it seems like it would be something like this:
        for(var i=0; i<arguments.length; ++i) {
            if(typeof arguments[i] == "function") {
                // get the prototype of the superclass
                parent = arguments[i].prototype;
    
    
                if (i==0) {
                    Class.prototype.superclass = arguments[i];
                }
    
    
            } else {
    
    

... though not sure if that is quite right.

07/13/07 14:29:10 changed by tschaub

  • keywords deleted.

pulling from the review queue until I can demonstrate that this really works

07/13/07 18:45:59 changed by tschaub

I've got a nice big patch ready for review. After someone reviews #837.

07/16/07 12:58:58 changed by tschaub

  • attachment class.2.patch added.

new class syntax applied everywhere and tested

07/16/07 13:00:23 changed by tschaub

  • keywords set to review.

Ok, please review class.2.patch. This adds the new style Class constructor, modifies all instances of Class.create and Class.inherit, allows for backwards compatibility, and tests.

07/16/07 13:11:43 changed by tschaub

Patch trivia:

svn diff lib | diffstat
 ...
 98 files changed, 278 insertions(+), 416 deletions(-)
svn diff tests | diffstat
 BaseTypes/test_Class.html |  164 ++++++++++++++++++++++++++++++++++++++++++++++
 Control/test_Panel.html   |    6 -
 Marker/test_Box.html      |    2 
 3 files changed, 166 insertions(+), 6 deletions(-)

Tests pass in IE (6/7) and FF

07/16/07 15:29:06 changed by euzuro

All looks good. Few comments:

  • Index: tests/Marker/test_Box.html -- the changes to this file are insignificant and I think can be removed.
  • According to my editor, the following files' changes cause the OpenLayers.Class line to go over the defined 79 character limit:
    • lib/OpenLayers/Control/EditingToolbar.js
    • lib/OpenLayers/Geometry/Polygon.js
  • Need to update Layer/TileCache.js to use the new style.
  • I would change this comment:
    /**
     * Constructor: OpenLayers.Class
     * Base class used to construct all other classes with multiple inheritance.
     * This constructor is new in OpenLayers 2.5.  At OpenLayers 3.0, the old
     * syntax for creating classes and dealing with inheritance will be removed.
     * 
     * To create a new OpenLayers style class, use the following syntax:
     * > MyClass = new OpenLayers.Class(prototype);
     *
     * To create a new OpenLayers Style class with multiple inheritance, use the
     *     following syntax:
     * > MyClass = new OpenLayers.Class(Class1, Class2, prototype);
     *
     */
    

to read:

/**
 * Constructor: OpenLayers.Class
 * Base class used to construct all other classes. Includes support for 
 *     multiple inheritance. 
 *     
 * This constructor is new in OpenLayers 2.5.  At OpenLayers 3.0, the old 
 *     syntax for creating classes and dealing with inheritance 
 *     will be removed.
 * 
 * To create a new OpenLayers-style class, use the following syntax:
 * > MyClass = new OpenLayers.Class(prototype);
 *
 * To create a new OpenLayers-style class with multiple inheritance, use the
 *     following syntax:
 * > MyClass = new OpenLayers.Class(Class1, Class2, prototype);
 *
 */

07/16/07 16:12:53 changed by euzuro

  • keywords changed from review to commit.

assuming you are ok with the above comments, then im ok with committing... :-)

07/16/07 16:26:20 changed by tschaub

  • keywords deleted.
  • status changed from new to closed.
  • resolution set to fixed.

Thanks for the review Dr. Uz. All comments incorporated.

This in with r3767.

Henceforth:

var MyClass = OpenLayers.Class(SomeParent, prototype);

09/13/07 03:18:44 changed by fredj

  • status changed from closed to reopened.
  • resolution deleted.

OpenLayers.Format.XML still use the old syntax

09/13/07 03:19:59 changed by fredj

  • attachment Format.XML.class.patch added.

fix OpenLayers.Format.XML class declaration

09/13/07 06:26:54 changed by crschmidt

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [4255]) Update class creation on Format.XML. Thanks, fredj. (Closes #823)