OpenLayers OpenLayers

Ticket #712 (closed feature: fixed)

Opened 1 year ago

Last modified 1 year ago

Get everything in the OpenLayers namespace

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

Description

Function, Number, and String methods would be nice to have in the OpenLayers namespace.

Attachments

BaseTypes_only.patch (10.5 kB) - added by tschaub on 09/14/07 12:52:10.
Only to be reviewed by the dubious - full patch to follow.
namespace.patch (46.8 kB) - added by tschaub on 09/14/07 13:03:19.
Remove and deprecate the use of prototype extensions

Change History

09/11/07 13:53:57 changed by tschaub

Ok, this obviously needs all sorts of tests and changes in the code, but this is an example of what I'd like to see. That limitSigDigs function was a bit wordy (unless I'm nuts), and the prototype bind stuff can be simplified a bit.

09/11/07 16:19:05 changed by tschaub

  • keywords set to review.
  • milestone changed from 3.0 Release to 2.6 Release.

The patch is now complete. All prototype extensions replaced with OL functions.

09/13/07 21:35:47 changed by euzuro

are we sure we don't want to put this into 2.5???

09/13/07 21:37:14 changed by crschmidt

  • milestone changed from 2.6 Release to 2.5 Release.

I'm willing to have this go into 2.5. I think Tim's made a good point of what we want to do here, and I think the patch is good. Erik, you got any qualms about putting this in before rc1?

09/13/07 23:23:06 changed by euzuro

assuming the patch is all golden and a test-passin' then yeah i think we should definitely put it in before we make the RC.

i haven't had the mind to review this patch entirely, but i am definitely in accord with what it's doing. let us finally free ourselves of our meddling ways.

if you have reviewed it and like it, i'm good to have it in.

09/14/07 10:38:34 changed by crschmidt

  • keywords deleted.

Okay, reviewed this more carefully, and I have only one complaint:

We're duplicating code.

String.camelize is still defined, with a warning -- but instead of just being a wrapper, it reimplements the same code.

Since we just fixed string.trim(), it seems possible we'll fix others before 3.0 -- so we should make them all just wrappers around the new functions we're defining.

Tim, could you clean that aspect of it up? I know it's time consuming and annoying, but as always, better to get it right than pay the price later.

09/14/07 10:43:24 changed by euzuro

good catch, chris. you're right that should definitely be fixed before this patch gets in. if no one else has time to do it, I can pick this up over the weekend.

09/14/07 12:52:10 changed by tschaub

  • attachment BaseTypes_only.patch added.

Only to be reviewed by the dubious - full patch to follow.

09/14/07 12:59:12 changed by tschaub

Ok, I'm down with the suggestions here. I've made the changes to BaseTypes and attached the diff here. I'll next put up the patch for the whole lib - but because this includes changes to the tests (and removes use of the old prototype extensions), I thought it was nice to be able to review changes and run tests with these wrappers only.

The careful reviewer will notice that these changes cause 1 test to fail. I've taken the liberty of changing the functionality of limitSigDig so that it *does* work with floats. If this is considered a heinous addition to the API, I'll (begrudgingly) change it back. Now, I know this is just the sort of sneaky change that raises the hackles of some reviewers. That said, I don't see reason to impose unnecessary limitations in functionality.

This same shrewd reviewer may also notice that a number of the other methods have changed in structure (though not in functionality.) I'll describe these changes in a later comment.

09/14/07 13:03:19 changed by tschaub

  • attachment namespace.patch added.

Remove and deprecate the use of prototype extensions

09/14/07 13:24:04 changed by tschaub

As promised, here's what else I changed:

String.prototype.startsWith used to look like this

String.prototype.startsWith = function(sStart) {
    return (this.substr(0,sStart.length) == sStart);
};

I've made OpenLayers.String.startsWith look like this

    startsWith: function(str, sub) {
        return (str.indexOf(sub) == 0);
    }

Justification: no reason to do substring extraction and string comparison.

String.prototype.trim used to look like this

String.prototype.trim = function() {
    return this.replace(/^\s+/, '').replace(/\s+$/, '');    
};

I've made OpenLayers.String.trim look like this

    trim: function(str) {
        return str.replace(/^\s*(.*?)\s*$/, "$1");    
    }

Justification: no reason to do two regex replacements when one will suffice (this becomes significant when parsing strings representing many many geometries).

Number.prototype.limitSigDigs used to look like this

Number.prototype.limitSigDigs = function(sig) {
    var numStr = (sig > 0) ? this.toString() : "0";
    if (numStr.contains(".")) {
        var msg = "limitSigDig can not be called on a floating point number";
        OpenLayers.Console.error(msg);
        return null;
    }
    if ( (sig > 0) && (sig < numStr.length) ) {
        var exp = numStr.length - sig;
        numStr = Math.round( this / Math.pow(10, exp)) * Math.pow(10, exp);
    }
    return parseInt(numStr);
};

I've made OpenLayers.Number.limitSigDigs look like this

    limitSigDigs: function(num, sig) {
        var fig;
        if(sig > 0) {
            fig = parseFloat(num.toPrecision(sig));
        } else {
            fig = 0;
        }
        return fig;
    }

Justification: it's easier on the eyes. Note that this method could be replaced entirely by the use of parseFloat(num.toPrecision(sig)) - using built in methods instead of our own - if you don't care about getting the null & negative argument handling.

Function.prototype.bind used to look like this

Function.prototype.bind = function() {
    var __method = this;
    var args = [];
    var object = arguments[0];
    
    for (var i = 1; i < arguments.length; i++) {
        args.push(arguments[i]);
    }
    
    return function(moreargs) {
        var i;
        var newArgs = [];
        for (i = 0; i < args.length; i++) {
            newArgs.push(args[i]);
        }
        for (i = 0; i < arguments.length; i++) {
            newArgs.push(arguments[i]);
        }
        return __method.apply(object, newArgs);
    };
};

I've made OpenLayers.Function.bind look like this

    bind: function(func, object) {
        // create a reference to all arguments past the second one
        var args = Array.prototype.slice.apply(arguments, [2]);
        return function() {
            // Push on any additional arguments from the actual function call.
            // These will come after those sent to the bind call.
            var newArgs = args.concat(
                Array.prototype.slice.apply(arguments, [0])
            );
            return func.apply(object, newArgs);
        };
    }

Justification: all that args looping and pushing can be replaced by the use of the array prototype's slice and concat methods. (Were arguments a real array, we could slice directly. Because it is not, we apply the array prototype's slice.)

The only other oddity is the new Function.prototype.bind wrapper. This can not directly call the new function because of the argument handling. As above, we can use methods on the array prototype to unshift the function on the the arguments before applying the new function.

09/14/07 13:24:28 changed by tschaub

  • keywords set to review.
  • status changed from new to assigned.

All tests pass in IE & FF. Please review.

09/14/07 16:03:25 changed by crschmidt

  • keywords changed from review to commit.

All tests pass. Testing of examples + acceptance tests succeeds: in general, this seems to match the old behavior without problems.

Go for it.

09/14/07 16:08:49 changed by tschaub

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

(In [4302]) Deprecating all prototype extensions. This puts all OpenLayers functionality in the OpenLayers namespace. If you are using any of the Function, String, or Number prototype extensions, start using the functional equivalents in the OpenLayers namespace - the prototype extensions will be gone in 3.0 (closes #712).