OpenLayers OpenLayers

Ticket #1392 (closed feature: fixed)

Opened 6 months ago

Last modified 6 months ago

panTween needs to be nullified

Reported by: pgiraud Assigned to:
Priority: minor Milestone: 2.6 Release
Component: Map Version: 2.5
Keywords: Cc:
State: Complete

Description

Eric previously proposed we don't recreate a new Tween object each time we call panTo. But, this means that, with the current if conditions in moveTo(), panTween.stop() will be called even if not required. Attached patch fixes this by nullifying the panTween property.

One other solution would be to have a 'isPlaying' boolean property in the tween class and only call stop if isPlaying is true.

Attachments

1392-r6343-A0.diff (0.7 kB) - added by pgiraud on 02/26/08 04:14:57.
1392-r6382-B0.diff (2.5 kB) - added by pgiraud on 02/27/08 03:47:30.
1392-r6382-B1.diff (2.5 kB) - added by pgiraud on 02/27/08 07:58:35.

Change History

02/26/08 04:14:57 changed by pgiraud

  • attachment 1392-r6343-A0.diff added.

02/26/08 04:21:12 changed by pgiraud

I can easily provide a new patch if you guys prefer the second solution.

02/26/08 05:48:09 changed by crschmidt

  • milestone set to 2.6 Release.

I think that this is better, but still not perfect, because we don't actually set the panTween to 'null' when we're done panning, only the next time we setCenter, which means that there will always be one extra panTween.done called... Seems like we should *also* nullify it in panTween.done (in moveTo)?

02/27/08 03:47:30 changed by pgiraud

  • attachment 1392-r6382-B0.diff added.

02/27/08 03:50:07 changed by pgiraud

  • state set to Review.

This new patch adds a new 'playing' property to the Tween class. A call to stop doesn't do anything if tween isn't playing. No change needed in Map.js with this solution.

Patch also includes completed tests and the missing 'panTween' property declare statement in Map.

Please review.

02/27/08 03:58:45 changed by pgiraud

Forgot to mention that tests pass on FF2 and IE6 (if we don't consider errors in Layer_MapGuide).

02/27/08 05:54:51 changed by ahocevar

  • state changed from Review to Commit.

Pierre: I'm totally in favour of the second patch. Thanks for addressing this.

The patch looks perfect, please commit!

02/27/08 05:57:40 changed by pgiraud

I would like Christopher's opinion on that before I commit.

(follow-up: ↓ 8 ) 02/27/08 07:36:31 changed by crschmidt

Why don't we just nullify the panTween in the function bound to done? Wouldn't that take care of it?

I'm happy with either approach, really. Just curious.

02/27/08 07:58:35 changed by pgiraud

  • attachment 1392-r6382-B1.diff added.

(in reply to: ↑ 7 ) 02/27/08 08:05:58 changed by pgiraud

Replying to crschmidt:

Why don't we just nullify the panTween in the function bound to done? Wouldn't that take care of it?

The new 'playing' approach doesn't require any change in the Map.js code. I have set the 'playing' to Property (non API) because the code in Map.js doesn't have to know anything about it.

If I choose to nullify the panTween I would have to do it in several location in the code. I'm not sure I won't forget one.

02/27/08 08:47:46 changed by crschmidt

Okay, that's fair enough. Essentially, this prevents us from shooting ourselves in the foot slightly more than the other way around.

With that in mind, and Andreas's review, I'm happy if you are.

02/27/08 08:57:13 changed by pgiraud

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

(In [6387]) add a new 'playing' property to the Tween class so that a call to stop doesn't do anything if animation is already finished, r=ahocevar,crschmidt (Closes #1392)

02/27/08 14:05:28 changed by tschaub

(In [6388]) Invoking the trivial change clause, I'm adding a semicolon. (see #1392)