OpenLayers OpenLayers

Ticket #967 (closed task: fixed)

Opened 1 year ago

Last modified 7 months ago

map.setCenter() dragging interpretation

Reported by: elemoine Assigned to:
Priority: minor Milestone: 2.6 Release
Component: general Version: SVN
Keywords: Cc:
State: Complete

Description

map.setCenter() dragging argument is documented as follows:

"Specifies whether or not to trigger movestart/end events"

This is misleading. dragging is also passed to layer.moveTo(), meaning it has other implications than just specifying whether or not to trigger movestart/end events.

We could just change the ND comment for dragging.

In addition, I'd like to see a new argument to setCenter(), namely noEvent. This argument will actually specify whether or not to trigger movestart/end events. Adding it should not break anything as noEvent would evaluate to false if not specified.

There are cases where it's convenient to have setCenter() not triggering moveend/start event. For example, I recently happened to register a callback on moveend. This callback calls map.setCenter()... See the problem? ;-)

Attachments

patch-967-A0.diff (1.7 kB) - added by elemoine on 09/12/07 16:37:49.
setCenter noEvent patch
patch-967-A1.diff (1.7 kB) - added by elemoine on 09/12/07 16:42:57.
fix typo in previous patch (thanks crschmidt!)
moveTo.patch (0.9 kB) - added by crschmidt on 01/16/08 11:57:38.
demonstrate what i mean
patch-967-r5944-B0.diff (2.9 kB) - added by elemoine on 01/30/08 18:12:47.
patch-967-r5950-B1.diff (3.0 kB) - added by pgiraud on 01/31/08 12:21:28.
patch-967-r5978-C0.diff (2.7 kB) - added by elemoine on 02/03/08 14:21:36.
967.patch (2.8 kB) - added by crschmidt on 02/08/08 08:10:33.
use caller instead of source

Change History

09/12/07 16:37:49 changed by elemoine

  • attachment patch-967-A0.diff added.

setCenter noEvent patch

09/12/07 16:42:57 changed by elemoine

  • attachment patch-967-A1.diff added.

fix typo in previous patch (thanks crschmidt!)

09/13/07 14:56:55 changed by elemoine

  • keywords deleted.

Removing review keyword. We'll resurect this patch later, after 2.5.

10/10/07 00:55:11 changed by crschmidt

  • milestone set to 2.6 Release.

01/16/08 11:06:57 changed by crschmidt

  • state set to Needs Discussion.

Can you explain how the 'dragging' parameter is different than the 'noEvent' parameter? If you pretend that the dragging param is actually called 'noEvent', how is the behavior different with the two params?

01/16/08 11:16:40 changed by pgiraud

'dragging' argument is also passed to layers moveTo calls. There it has other implications than just triggering events or not.

01/16/08 11:19:20 changed by crschmidt

I see. Okay.

I really don't like the proliferation of booleans: I think perhaps going to the moveTo style thing I suggested instead would be better. This would allow setCenter to just be an old-style wrapper around the new 'moveTo' on the map, which takes, center, zoom, and options.

Does that make any sense? I just don't like this long list of parameters that may continue to grow...

01/16/08 11:57:38 changed by crschmidt

  • attachment moveTo.patch added.

demonstrate what i mean

01/22/08 17:09:19 changed by crschmidt

  • state changed from Needs Discussion to Needs More Work.

General agreement in IRC meeting: needs mor ework though

01/22/08 17:16:42 changed by crschmidt

  • state changed from Needs More Work to Needs Discussion.

01/30/08 18:12:47 changed by elemoine

  • attachment patch-967-r5944-B0.diff added.

01/30/08 18:17:40 changed by elemoine

patch-967-r5944-B0.diff implements crschmidt's idea and introduces two new options to setCenter (moveTo really): noEvent and cancelTween. The cancelTween option is added to help pgiraud with #1308. At this point, I haven't run the unit tests with this patch. It's more a 'see-if-that-can-help-pgiraud' patch for now...

01/31/08 12:21:05 changed by pgiraud

Hey, it seems like it matches my needs and Chris' proposal. I just tested it with the panTo method and this works well. If this is to be accepted as is, I would just prefer to rename cancelTween to stopPanTween. New patch attached. Some of the code in this patch goes with the code for #110.

01/31/08 12:21:28 changed by pgiraud

  • attachment patch-967-r5950-B1.diff added.

01/31/08 12:22:30 changed by pgiraud

Note : I launched tests with the last patch and they passed under FF and IE6.

02/03/08 14:21:36 changed by elemoine

  • attachment patch-967-r5978-C0.diff added.

02/03/08 14:29:27 changed by elemoine

  • state changed from Needs Discussion to Review.

I've just attached a new patch. In this new patch I've taken out the animation-related code - this code must be part of the animation patch of #110.

So I guess this patch is ready for review.

pgiraud ran the tests on FF and IE6. And I just double-checked that they pass on FF. They do ;-)

(follow-up: ↓ 13 ) 02/04/08 04:50:29 changed by pgiraud

This looks good to me. Do we need another reviewer ?

(in reply to: ↑ 12 ; follow-up: ↓ 14 ) 02/04/08 07:50:32 changed by elemoine

Replying to pgiraud:

This looks good to me. Do we need another reviewer ?

I guess we do. IIRC, during our first IRC meeting for 2.6 Erik expressed his disfavor for introducing this moveTo method. Erik, are you still concerned with this patch? Or others maybe...

(in reply to: ↑ 13 ) 02/05/08 00:10:15 changed by euzuro

Replying to elemoine:

Replying to pgiraud:

This looks good to me. Do we need another reviewer ?

I guess we do. IIRC, during our first IRC meeting for 2.6 Erik expressed his disfavor for introducing this moveTo method. Erik, are you still concerned with this patch? Or others maybe...

Well, I'm not *incredibly* against it. I just think it seems sort of funny. But I haven't been able to follow the development here for the last 3 months, so unless a truck comes by and unloads a week or two of extra time on my lap... I'm perfectly comfortable defering to the people who have been working on this.

02/07/08 13:56:44 changed by crschmidt

This is fine. Note that there is some discussion on possibly changing moveTo in the future to not have the required center and zoom parameters -- however, since moveTo is an internal method, I think we can safely change this.

One other comment: one of the reasons to do this is that it will allow us to tell if a movement is tirggered by setCenter or not. If we could add something to add a property like 'setCenter': true, that way, we can know the case where that's the source of the moveTo init (and do things like cancel tweening, for example).

(follow-up: ↓ 18 ) 02/07/08 16:41:42 changed by crschmidt

Actually, I've got one other Comment: rather than having options && options.foo in all cases, I think if we don't have an options object, we should just create an empty object to use for the rest of the function; feels cleaner to me. I've made this change and an addition of a 'source': 'SetCenter' option in a patch I'm about to commit to a sandbox; i"ll turn into a patch in a bit.

02/07/08 16:47:57 changed by crschmidt

r6035

(in reply to: ↑ 16 ) 02/08/08 01:28:13 changed by pgiraud

Replying to crschmidt: I totaly agree with the options empty object set if not options is given as argument. About the 'source' option you added. Don't you feel like 'caller', you previously proposed, a better choice (for readability without doc) ?

02/08/08 08:10:33 changed by crschmidt

  • attachment 967.patch added.

use caller instead of source

02/08/08 08:10:57 changed by crschmidt

Yes! This is what I get for not listening to myself :) New patch attached. Thx

02/08/08 08:22:02 changed by pgiraud

  • state changed from Review to Commit.

This patch looks ready to go into trunk now.

02/08/08 08:24:36 changed by crschmidt

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

Refactor setCenter to do most of its work through a smarter function called moveTo. In moveTo, we also have knowledge of whether the event was fired through setCenter, allowing us to know the difference between an 'internal' move and an external one. r=pgiraud (r6099)