activate and deactivate methods of Controls and Handlers cause boilerplate code

[dhurlburtusa]

The activate and deactivate methods of OpenLayers.Control and OpenLayers.Handler cause boilerplate code in their subclasses.

For instance, the implementation of the activate method in OpenLayers.Handler.Box is as follows:

    /**
     * Method: activate
     */
    activate: function () {
        if (OpenLayers.Handler.prototype.activate.apply(this, arguments)) {
            this.dragHandler.activate();
            return true;
        } else {
            return false;
        }
    },

The problem is that you have to call the superclass's activate method and return its boolean value. Now if OpenLayers.Handler added a template method called onActivate implemented as such:

    /**
     * Method: onActivate
     * Method to be implemented by subclasses of OpenLayers.Handler to
     * perform any actions necessary during activation.
     * 
     * By default, this method does nothing.
     * 
     * @TemplateMethod
     */
    onActivate: function () {
    },

and implemented OpenLayers.Handler.activate as follows:

    /**
     * APIMethod: activate
     * Turn on the handler.  Returns false if the handler was already active.
     * 
     * Returns: 
     * {Boolean} The handler was activated.
     */
    activate: function() {
        if(this.active) {
            return false;
        }
        // register for event handlers defined on this class.
        var events = OpenLayers.Events.prototype.BROWSER_EVENTS;
        for (var i=0, len=events.length; i<len; i++) {
            if (this[events[i]]) {
                this.register(events[i], this[events[i]]); 
            }
        } 
        this.active = true;
        this.onActivate(); // The only difference from the original activate method.
        return true;
    },

then the OpenLayers.Handler.Box would implement onActivate as follows:

    /**
     * Method: onActivate
     */
    onActivate: function () {
        OpenLayers.Handler.prototype.onActivate.apply(this, arguments);
        this.dragHandler.activate();
    },

instead of implementing activate along with the boilerplate code.

Similarly for deactivate, there would be an onDeactivate template method.

Similar changes can be done for OpenLayers.Controls.

Motivation:

1. Reduce the amount of boilerplate code and thereby reducing the size of OpenLayers.js which is currently about 850KB (minimized).

2. Easier to implement subclasses of OpenLayers.Control and OpenLayers.Handler.

3. Easier to read and maintain subclasses.

[ahocevar]

One thing has to be considered: if a handler/control cannot be activated, the activate() method has to return false. If done as you propose, the onActivate() method could be the one to decide whether activation is successful or not. So you would also have the onActivate method return true or false, probably adding more boilerplate code than we currently have.

[dhurlburtusa]

The activate method will still return whatever it returns. If the control/handler is active it returns false. Else, in the handler's case, iterates over the events and registers them. Then it sets active to true. THEN it calls onActivate. This is where subclasses get to inject their own activation specific actions. Once onActivate returns, the activate method returns true.

Please look again at the code examples. onActivate does not need to return anything. Whether the control or handler was activated is still handled by the activate method, not by the onActivate template method.

The only time you would override activate is when this.active is not the only criteria determining whether to activate the control. But this should be a rare case.

[ahocevar]

@dhurlburtusa: why don't you just provide a patch? If you can prove that it works and really simplifies things, I'm willing to review and commit it.

[elemoine]

dhurlburtusa's proposed change would make sense to me. And we'd still be able to overload activate/deactivate if we need to control the return value. If we change the base class as suggested I think we also should change the existing controls so they actually use the new pattern.