Missing dependencies for Map and DragPan

[jpulles]

The Map.js doesn't have @requires for the default controls: Navigation, PanZoom, ArgParser, and Attribution. The DragPan control is missing the @requires for kinetic dragging.

The accompanying patch adds comment lines for these @requires.

[bartvde]

John, this is by design, to keep builds small, especially for mobile. Application developers are responsible for adding these to their build if they use them. Closing as WONTFIX.

[jpulles]

Sorry to reopen this issue as 'needs discussion', but I would like to argue that the requires statements should reflect what is in code. Alternatively, to the design decision can be noted in the file header.

For DragPan the defaults are that Kinetic is not required, so this is ok. For Map.js however, the default code requires a Navigation control a.o. If the code would check for availability of these classes, the lack of @requires would be correct.

[erilem]

Replying to jpulles:

Sorry to reopen this issue as 'needs discussion', but I would like to argue that the requires statements should reflect what is in code. Alternatively, to the design decision can be noted in the file header. For DragPan the defaults are that Kinetic is not required, so this is ok. For Map.js however, the default code requires a Navigation control a.o. If the code would check for availability of these classes, the lack of @requires would be correct.

I'd be +1 on changing the code of OpenLayers.Map, to add Navigation, PanZoom, ArgParser et Attribution controls only if their constructors are available. With something like this:

if(this.controls == null) {
    this.controls = [];
    if(OpenLayers.Control) {
        var names = ["Navigation", "PanZoom", "ArgParser", "Attribution"];
        for(var i=0, len=names.length; i<len; i++) {
            var ctor = OpenLayers.Control[names[i]];
            if(ctor) {
                controls.push(new ctor());
            }
        }
    }
}

(Untested)

[ahocevar]

@erilem: Correct me if I'm wrong, but what you're suggesting above is exactly what we've been doing for years now.

[bartvde]

ahocevar: we currently only check if the OpenLayers.Control namespace is there:

        if (this.controls == null) {
            if (OpenLayers.Control != null) { // running full or lite?
                this.controls = [ new OpenLayers.Control.Navigation(),
                                  new OpenLayers.Control.PanZoom(),
                                  new OpenLayers.Control.ArgParser(),
                                  new OpenLayers.Control.Attribution()
                                ];
            } else {
                this.controls = [];
            }
        }

[erilem]

Replying to ahocevar:

@erilem: Correct me if I'm wrong, but what you're suggesting above is exactly what we've been doing for years now.

I'm not sure. We've been adding @requires all other the place, making it hard to create minimal custom builds of OpenLayers. We're now trying to have @requires only when absolutely necessary.