make Layer.addOptions call initResolutions if necessary

[elemoine]

this tickets suggests modifying the Layer.addOptions method in such a way that initResolutions is called (again) if at least one of the new options is a "resolution option" (one of 'projection', 'units', 'scales', 'resolutions', 'maxScale', 'minScale', 'maxResolution', 'minResolution', 'minExtent', 'maxExtent', 'numZoomLevels', 'maxZoomLevel').

[elemoine]

apply patch from #2427 before this one

[bartvde]

I assume this is ready for review elemoine?

[elemoine]

Replying to bartvde:

I assume this is ready for review elemoine?

Hi Bart. I'd like that #2427 gets reviewed and committed first. I'll mark #2427 as Review once 2.9 is out.

[bartvde]

Hi Eric, since #2427 has been committed, is this ready for review?

[elemoine]

Replying to bartvde:

Hi Eric, since #2427 has been committed, is this ready for review?

Hi Bart, thanks for bringing this back up. I'd like to give it another look before changing the ticket state to review. I'll do it in the coming days.

[elemoine]

Bart, I made a few adjustments to the patch and added tests (patch-2360-A2.diff ). Tests pass in FF3. This is now ready for review.

[bartvde]

Hi Eric, looks good, but I have a few questions:

• shouldn't you check for the existence of this.projection:

if(typeof this.projection === "string") { 

if (this.projection && typeof this.projection == "string") {

• shouldn't this.units be taken from projection in addOptions as well? Like is done in setMap?

            // Check the projection to see if we can get units -- if not, refer
            // to properties.
            this.units = this.projection.getUnits() ||
                         this.units || this.map.units;

• I did not really get your last change in the test ({chicken: chicken}), can you elaborate?

[elemoine]

Replying to bartvde:

Hi Eric looks good,

Thanks for reviewing Bart.

but I have a few questions: * shouldn't you check for the existence of this.projection:

if(typeof this.projection === "string") { 

if (this.projection && typeof this.projection == "string") {

If this.projection is undefined then typeof this.projection == "string" will evaluate to false, so I think I'm ok.

* shouldn't this.units be taken from projection in addOptions as well? Like is done in setMap?

            // Check the projection to see if we can get units -- if not, refer
            // to properties.
            this.units = this.projection.getUnits() ||
                         this.units || this.map.units;

You're right.

* I did not really get your last change in the test ({chicken: chicken}), can you elaborate?

With the new code in addOptions, if the projection option is a string then an OpenLayers.Projection object is created, so the test

t.eq(layer.projection, projection, "projection set correctly");

fails without this change.

I'll work on a new patch to address you second point.

[bartvde]

Eric, can you also remove the this.projection check then from setMap since it is unnecessary as you correctly point out?

if (this.projection && typeof this.projection == "string") {

if (typeof this.projection == "string") {

TIA.

[elemoine]

addresses Bart's comments

[elemoine]

Bart, see patch-2360-A3.diff . I'd appreciate another look from you before I commit. Thanks.

[elemoine]

Replying to elemoine:

Bart, see patch-2360-A3.diff . I'd appreciate another look from you before I commit. Thanks.

I forgot to mention that tests pass in FF3, IE7 and Chrome5.

[bartvde]

Looks good now Eric, please commit.

[elemoine]

(In [10680]) make Layer.addOptions call initResolutions if necessary, r=bartvde (closes #2360)

[bartvde]

Layer/Image test is failing because of this change I presume:

fail default layer projection correctly set. eq: types differ: got Object, but expected String

[bartvde]

Test case fixed in r10704 just like the other test was fixed up by elemoine. The fix for the test case should go into 2.10., so changing state to pullup.

[crschmidt]

(In [10715]) Commit fixes for:

• (Closes #2360) make Layer.addOptions call initResolutions if necessary
• (Closes #2656) no way to pass read options from protocol to format
• (Closes #2751) Changes in languages: "es" and "ca".
• (Closes #2814) SLDSelect control tests failing
• (Closes #2815) Cluster Strategy should not destroy all features

Also:

• Change examples to use OSGeo, rather than MetaCarta, servers.
• Change copyright statements to correctly state joint copyright in the project, rather than MetaCarta copyright.