Opened 7 years ago

Last modified 6 years ago

#396 new defect

Errors in components with a layer property when the layer is part of the MapPanel.layers configuration property

Reported by: mpriour Owned by:
Priority: minor Milestone: 1.2
Component: GeoExt Version: 1.0
Keywords: Cc:
State: Needs more work

Description (last modified by ahocevar)

As of GeoExt 1.0, layers configured as a property of the GeoExt.MapPanel.layers property are not actually added to the map until the MapPanel is rendered.

This behaviour causes problems in several components which can be configured with a layer that is added to the map as a property of the MapPanel. The components depend on a valid 'this.layer.map' or 'config.layer.map' property to determine where to add controls, attach event listeners, etc...

These errors could be eliminated by allowing them to be configured with an optional 'map' property which could be a GeoExt.MapPanel or OpenLayers.Map. This map property would be used when 'layer.map' was null and could fallback to GeoExt.MapPanel.guess() when no 'map' property is provided.

Attachments (3)

LazyLayerFixExamples.patch (4.2 KB) - added by mpriour 7 years ago.
An example of how to fix the layer.map property dependance which is broken when layers are lazy loaded via the MapPanel. Contains a fix for a class that adds a control to the map and a class that adds and removes event handlers to/from the map.
geoext-396.patch (1.6 KB) - added by ahocevar 7 years ago.
untested, no tests
geoext-396-with-tests.patch (5.1 KB) - added by mpriour 7 years ago.
Added tests to @ahocevar's geoext-396 patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by mpriour

I've attached patches for GeoExt.grid.FeatureSelectionModel and GeoExt.LayerOpacitySlider as an example of how this could be fixed.

Changed 7 years ago by mpriour

An example of how to fix the layer.map property dependance which is broken when layers are lazy loaded via the MapPanel. Contains a fix for a class that adds a control to the map and a class that adds and removes event handlers to/from the map.

Changed 7 years ago by ahocevar

untested, no tests

comment:2 Changed 7 years ago by ahocevar

  • State changed from None to Needs more work

The attached patch shows how this could be done now that http://trac.osgeo.org/openlayers/ticket/2983 is resolved. @mpriour: maybe you want to see if this works and add unit tests?

Changed 7 years ago by mpriour

Added tests to @ahocevar's geoext-396 patch

comment:3 Changed 7 years ago by mpriour

Patch worked well for test applications. Added unit tests for layers that had not been added to a map yet. I'm not sure if these test are really the best way to test but they seemed like the most reasonable way to test without registering listeners with a specific priority or using ExtJS's createSequence method.

If these test look good then I can expand your method for handling lazy loaded layers to the other classes that need it and expand the tests as well.

comment:4 Changed 7 years ago by ahocevar

Thanks for the tests @mpriour. I think it would be simpler to just di something like this:

var mapPanel =  new GeoExt.MapPanel({
    layers: [layer]
});
// create a FeatureSelectionModel
// test for control.map, undefined
mapPanel.render("map");
// test again, control.map == mapPanel.map

Also please make sure to use 4 spaces for indentation.

comment:5 Changed 7 years ago by elemoine

For LayerOpacitySlider event unregistration should occur on "unbind". In my opinion registration should be done in bind, and unregistration in unbind.

comment:6 Changed 7 years ago by ahocevar

  • Description modified (diff)

@elemoine: what event are you referring to? A removed layer can be re-added later, and we cannot call bind before the layer is added. Can you provide an alternative patch to show what you mean?

comment:7 Changed 6 years ago by ahocevar

  • Milestone changed from 1.1 to 1.2

Batch move of tickets to finish the 1.1 milestone.

Note: See TracTickets for help on using tickets.