Ticket #60 (closed enhancement: fixed)

Opened 1 year ago

Last modified 1 year ago

incorrect map initialization sequence in MapPanel within a layout

Reported by: pgiraud Assigned to: elemoine
Priority: blocker Milestone: 0.5
Component: GeoExt.MapPanel Version:
Keywords: Cc:
State: Review

Description

In some use cases (ie. when the MapPanel is part of a layout), the rendering sequence seems to cause problems with map initialization, particularly dealing with extent. When set as an item of a border layout for example, the MapPanel's onRender method is called to early (before the layout is done). This results in a map rendered first in an element with no dimensions.

The attached patch introduce a new initMap method called either when the element is rendered or after its parent's layout is done (if any). It also adds the layers only after the map is rendered to prevent Map.updateSize to be called twice.

State set to "Needs more work" since patch is missing tests.

Attachments

patch-60-r34.diff (2.4 kB) - added by pgiraud on 05/05/09 14:58:52.
patch-60-r830-A0.diff (4.1 kB) - added by elemoine on 05/20/09 18:12:08.
60.patch (3.4 kB) - added by tschaub on 05/22/09 20:15:36.
render map and set center after layout
60-also-fixes-39.patch (5.8 kB) - added by ahocevar on 05/23/09 11:04:17.
60-also-fixes-39.1.patch (8.2 kB) - added by elemoine on 05/25/09 08:21:16.
mappanel-with-google.diff (3.6 kB) - added by elemoine on 05/28/09 06:36:29.
60-also-fixes-39.2.patch (8.2 kB) - added by pgiraud on 05/28/09 10:23:16.
patch with workaround for OL#2105

Change History

05/05/09 14:58:52 changed by pgiraud

  • attachment patch-60-r34.diff added.

05/05/09 15:01:43 changed by elemoine

  • priority changed from minor to blocker.
  • milestone set to 0.1.

05/20/09 18:12:08 changed by elemoine

  • attachment patch-60-r830-A0.diff added.

05/20/09 18:16:26 changed by elemoine

  • state changed from Needs more work to Commit.

Patch looks good. I tested it using a map panel inside a panel with layout "border" and it does fix things. I also tested the existing mappanel examples, they still work. patch-60-r830-A0.diff adds a test. Please commit if the final patch works for you. Thanks.

05/22/09 10:24:41 changed by pgiraud

  • state changed from Commit to Needs more work.

Sometimes, the unit tests are really useful. I can't commit this patch because it breaks the LayerNode widget. Both LayerNode.html test and tree example are broken.

05/22/09 20:15:36 changed by tschaub

  • attachment 60.patch added.

render map and set center after layout

05/22/09 20:17:30 changed by tschaub

  • state changed from Needs more work to Commit.

I think the initial patches went a bit too far. The panel can be given a reference to the layers before the map has been rendered (just as you can add layers to a map before setting the center).

The latest patch just moves the map rendering and center setting to the point after the layout has settled.

Eric, if you agree this looks good, please commit.

05/23/09 09:29:03 changed by elemoine

I'll have a look on monday, thanks for picking that up Tim.

05/23/09 11:04:17 changed by ahocevar

  • attachment 60-also-fixes-39.patch added.

05/23/09 11:04:59 changed by ahocevar

My latest patch fixes the testcase of MapPanel.html to always set a size on the mappanel. Now the added vector layer does not cause any trouble. This testcase modification is the same I had made for #39, which is definitely a duplicate of this issue.

(follow-up: ↓ 8 ) 05/23/09 18:08:53 changed by elemoine

One preliminary question: why does renderMap call map.updateSize()? map.render() already does it, doesn't it?

I'd like that we add a unit test showing that we the map panel calls setCenter exactly once on render. When working on GeoExt.Action, I noticed that setCenter was called multiple times, making the history control have previous activated right after the map is initialized.

(in reply to: ↑ 7 ) 05/23/09 22:42:06 changed by elemoine

  • status changed from new to closed.
  • resolution set to fixed.

Replying to elemoine:

I'd like that we add a unit test showing that we the map panel calls setCenter exactly once on render. When working on GeoExt.Action, I noticed that setCenter was called multiple times, making the history control have previous activated right after the map is initialized.

Looking at the code I think we call map.setCenter twice today: we first zoom to max extent when we do map.render(), and then zoom to the user-specified extent. So the initial zoom to max extent isn't desirable, and it seems to me that it results from a bug in OpenLayers' Map:updateSize() method (which is called by Map:render()). updateSize() unconditionally calls setCenter(), that is even if the map does not have a center (because its moveTo() method has never been called), and in that case setCenter() moves the map to its max extent. Instead I think updateSize() should call setCenter() only the map has a center. And I guess this would solve the problem we're having with the map panel.

Since a fix to updateSize is unlikely to go in OpenLayers 2.8 I guess we could work around the issue in GeoExt 0.1 by updating map.center and map.zoom (based on the extent, center and zoom values provided in the map panel config) prior to calling map.render(). In this way map.updateSize() will take care of zooming the map to the extent specified by the user.

If people agree with that I'll write unit tests and update that ticket's latest patch.

05/23/09 22:43:23 changed by elemoine

  • status changed from closed to reopened.
  • resolution deleted.

I again closed the ticket my mistake. Sorry about that.

05/25/09 08:21:16 changed by elemoine

  • attachment 60-also-fixes-39.1.patch added.

05/25/09 08:27:38 changed by elemoine

60-also-fixes-39.1.patch complements ahocevar's 60-also-fixes-39.patch with the following:

  • add a unit test checking that map.moveTo is called exactly once when the map is rendered
  • add a workaround to <http://trac.openlayers.org/ticket/2105> to avoid calling map.moveTo twice when the map is rendered
  • remove the unnecessary call to updateMapSize from renderMap (map.updateSize is already called by map.render)

Tests pass in FF3.

Andreas, please commit if you agree with these changes.

(follow-up: ↓ 12 ) 05/27/09 17:10:01 changed by pgiraud

I'm +1 to commit this one. If no one complains or does the commit, I'll do it myself tomorrow.

(in reply to: ↑ 11 ) 05/27/09 17:12:50 changed by tschaub

Replying to pgiraud:

I'm +1 to commit this one. If no one complains or does the commit, I'll do it myself tomorrow.

Yeah, I agree. Pierre, if you've got it ready, I'd say commit now.

(follow-up: ↓ 14 ) 05/27/09 17:59:25 changed by elemoine

I've got other issues with Google layers, but this will go in a separate ticket. Please commit now! ;-)

(in reply to: ↑ 13 ) 05/28/09 06:36:11 changed by elemoine

Replying to elemoine:

I've got other issues with Google layers, but this will go in a separate ticket. Please commit now! ;-)

Yes, the map panel does not work with Google layers (at least I can't get it to work). And the patch attached to this ticket does not improve the situation. See the example mappanel-with-google.diff.

05/28/09 06:36:29 changed by elemoine

  • attachment mappanel-with-google.diff added.

05/28/09 10:22:35 changed by pgiraud

  • state changed from Commit to Review.

The OpenLayers #2105 ticket is really worth. However, the temporary workaround for the current ticket proposed by Eric is not fixing it. Vector layers won't show up in any example.

With Eric's help, here's a new patch to fix that.

05/28/09 10:23:16 changed by pgiraud

  • attachment 60-also-fixes-39.2.patch added.

patch with workaround for OL#2105

05/28/09 13:57:49 changed by pgiraud

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [877]) if the panel is an item of a container, don't render the map till the container layout is complete, this prevents issues with border layouts (Closes #60), thanks to ahocevar, tschaub and elemoine for their contribution on that ticket