Opened 8 years ago

Closed 8 years ago

#32 closed defect (fixed)

MapPanel should listen for ownerCt move

Reported by: tschaub Owned by: elemoine
Priority: critical Milestone:
Component: GeoExt.MapPanel Version:
Keywords: Cc:
State: Commit

Description

If you put a MapPanel in a window and move the window, the map erroneously uses cached offsets. An app developer could listen for move on the window and call updateSize on the MapPanel's map, but this requires special knowledge of OpenLayers. It also requires a reference to the MapPanel at least - which may not always be available when configuring the owner container.

So, a nice alternative is to check if the MapPanel has an ownerCt after it has been rendered. The MapPanel can listen for move and call updateSize on the map.

Attachments (2)

move.patch (1.9 KB) - added by tschaub 8 years ago.
update map size when owner container moves
move.2.patch (1.6 KB) - added by tschaub 8 years ago.
update map size when owner container moves (version 2)

Download all attachments as: .zip

Change History (13)

comment:1 follow-up: Changed 8 years ago by tschaub

  • State changed from None to Review

Drag the window in the mappanel-window.html example to see this issue in action. After dragging the map jumps on the first drag pan. Apply this patch to see better behavior.

Changed 8 years ago by tschaub

update map size when owner container moves

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by elemoine

Replying to tschaub:

Drag the window in the mappanel-window.html example to see this issue in action. After dragging the map jumps on the first drag pan. Apply this patch to see better behavior.

Tim, I don't understand what the issue is. Could you give the exact steps to reproduce the problem? Thanks.

comment:3 in reply to: ↑ 2 Changed 8 years ago by tschaub

Replying to elemoine:

Replying to tschaub:

Drag the window in the mappanel-window.html example to see this issue in action. After dragging the map jumps on the first drag pan. Apply this patch to see better behavior.

Tim, I don't understand what the issue is. Could you give the exact steps to reproduce the problem? Thanks.

Sure. Here's what I do:

1) browse to trunk/geoext/examples/mappanel-window.html 2) drag-pan map (not window) to confirm map dragging behaves as it should 3) drag window containing map panel (this ghosts the panel, moves it, then unghosts it) 4) drag-pan map (not window) to see map quickly jump before panning normally

The map is still using cached offsets after the move.

comment:4 Changed 8 years ago by bartvde

Hey Tim, looking at your patch I have one question: when should we use destroy and when should we use onDestroy instead? I would tend to use onDestroy in this case but I can't really provide a clear distinction when to use which one.

comment:5 Changed 8 years ago by tschaub

Yeah, I wondered the same thing. I decided to use destroy based on thinking like this:

  1. destroy is documented as part of the API, onDestroy is not - of course we wouldn't be overriding "constructor" and a whole lot of other stuff if we were strict about this, but it seems like a good thing to consider first.
  1. I generally like the idea of having constructor call constructor on the super first (or very early in the constructor function) and having destroy call destroy on the super last (or very late in the destroy function). The onDestroy method gets called somewhere in the middle. It is not important here, but it seems like it could cause trouble elsewhere.

That said, it seems like onRender, onDestroy, etc are the accepted ways of extending in ExtJS. I'd be happy adopting either convention. And we could also wait until we have enough of this code to settle on a pattern.

comment:6 follow-up: Changed 8 years ago by bartvde

I was looking at TreePanel, GridPanel and Window (all extending Ext.Panel) and they all use onDestroy. Also this post [1] by somebody from the Ext development team seems to suggest that things extending Component use onDestroy, and other objects destroy. At least that's what I can make of it all.

[1] http://extjs.com/forum/showthread.php?p=298178#post298178

Therefore, I would be in favour of using onDestroy.

comment:7 in reply to: ↑ 6 Changed 8 years ago by tschaub

Replying to bartvde:

[1] http://extjs.com/forum/showthread.php?p=298178#post298178

Therefore, I would be in favour of using onDestroy.

Interesting. My read of that thread is that it is a caution *against* using onDestroy - though not explicitly. It demonstrates how moving onDestroy around in a parent can wreak havoc on children. And we don't control the position of onDestroy. At least if we override destroy, we control the order.

That said, I am ok going either way. And I also see that onDestroy is used where others extend.

Changed 8 years ago by tschaub

update map size when owner container moves (version 2)

comment:8 Changed 8 years ago by tschaub

Ok, a patch for every preference. I'm happy to commit either. And, I don't think we need to labor the point right now. We can easily change from destroy to onDestroy (or the other way) at any point without harming our users.

comment:9 Changed 8 years ago by bartvde

  • State changed from Review to Commit

Tim, looks good to me, please commit.

comment:10 Changed 8 years ago by elemoine

I'm also +1 on using onDestroy, thanks Tim for the patch and taking the time to explain the problem to me.

comment:11 Changed 8 years ago by tschaub

  • Resolution set to fixed
  • Status changed from new to closed

(In [283]) If a map panel is rendered inside of a parent container, it will listen for move events on the parent. This allows us to clear the map's cached offsets. r=bartvde (closes #32)

Note: See TracTickets for help on using tickets.