Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#6060 closed enhancement (fixed)

dojox.fx.sizeTo caching leads to stale parameter state

Reported by: guest Owned by: dante
Priority: high Milestone: 1.4
Component: Dojox Version: 1.0
Keywords: Cc: ole.ersoy@…
Blocked By: Blocking:

Description (last modified by dante)

Hi,

I just did a little experimentation, and to make a long story short, it seems that dojox.fx.sizeTo is caching parameters it gets from the node argument during the construction of the animation.

Suppose the width of the node is 400px when the node is passed to the dojox.fx.sizeTo(args) factory method. The resulting animation thinks that when this animation is played, the width of the target node should always be 400px before the animation starts. From what I can see, it even sets the width to 400px (If it were changed to say 800px) before the animation starts, and then plays the animation.

To me it seems that since the starting width and height come from the node at first, the animation should always check the width and height before animating, and use the current width and height This would enable the animation to be cached and reused. Right now it seems that the only way to refresh the starting parameters is to create a brand new animation. I'm assuming that's fine in most cases. From a usability point of view though, I think it's more intuitive to have the animation start with the current width and height of the node whenever play is called. This way the animation can be cached, which means less garbage collection for the browser, etc.

Thoughts?

Thanks,

  • Ole

Attachments (4)

test_sizeTo2.html (2.2 KB) - added by guest 12 years ago.
Animation.js (1.7 KB) - added by guest 12 years ago.
SizeTo.js (2.6 KB) - added by guest 12 years ago.
Animation.html (1.4 KB) - added by guest 12 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 12 years ago by dante

Component: GeneralDojox
Milestone: 1.2
Owner: changed from anonymous to dante

i've seen this too. a test case would be awesome.

Changed 12 years ago by guest

Attachment: test_sizeTo2.html added

comment:2 Changed 12 years ago by guest

Dante,

I added a test case. It creates an animation, changes the width and height of the targeted node to 500px X 500px, and then plays the animation after 1250 milliseconds. When the animation starts, the width and height are reset to the width and height of the node at the time the animation was created.

Hope that helps. Please let me know if I can provide anything else.

BTW - I tried adding myself to the cc list, but apparently track did not take it. Could you please add me to the cc:

ole.ersoy@…

Thanks,

  • Ole

comment:3 Changed 12 years ago by dante

Cc: ole.ersoy@… added
Status: newassigned

comment:4 Changed 12 years ago by guest

Just a few more comments. In this ticket

http://trac.dojotoolkit.org/ticket/5213

I suggested the creation of a dojo.animation.Animation (Should probably be dojox.fx.Animation instead) base class for reasons particular to that ticket. I think that would also help in this case.

In this case I think dojox.fx.Animation should have two methods, initialize and play. Code that initializes the animation should be put in initialize and code that plays the animation goes in play, and Animation.play should call initialize. This way all subclasses of Animation get initialize called automatically just by implementing play like this:

play: function() { super.play(); Whatever else... }

So the base class could perform initialization that is common to all classes, which is what the sizeToHeight needed in ticket 5213. In this case additional initialization is needed before sizeTo is called, namely getting fresh parameters from the node being passed in the arguments, and deriving the other parameters the animation needs, before playing.

Also if the Animation subclass needs the initialization that takes place in the Animation base class, in can include it like this:

initialize: function() { super.initialize()

} Otherwise it could just leave it off, and run more even more efficiently.

If it were implemented like this dojox.fx.sizeTo would simply be a factory method that returns an instance of dojox.fx.SizeTo?, which subclasses dojox.fx.Animation.

Just my 2 cents.

Cheers,

  • Ole

comment:5 Changed 12 years ago by guest

One more thing I'm wondering about, but have yet to test, is what if a cached animation had it's node switched like this (Pseudo):

animation.node = dojo.byId("SomeOtherNode?");

or

animation.node = someOtherNode;

In this case I'm hoping that the animation will play the new node, but suspect that animation will just ignore it, and keep running with the old node.

I'm just leaving the thought here as a place holder, until I can investigate further.

Cheers,

  • Ole

comment:6 Changed 12 years ago by guest

I tried reimplementing sizeTo using two classes (Will attach). dojox.fx.Animation and dojox.fx.SizeTo?.

dojox.fx.Animation has template methods:

dojox.fx.Animation.refresh()

dojox.fx.Animation.initialize()

dojox.fx.Animation.play()

All the repetitive code that is contained by most animations is put in: dojox.fx.Animation.constructor()

Code that calculates animation parameters goes in: dojox.fx.Animation.refresh()

This method is also called during construction of instances of Animation and right before play(), in order to refresh animation parameters.

Code that creates objects used in the animation, but should only be created once goes in: dojox.fx.Animation.initialize()

This method fully assembles the animation and then sets the dojox.fx.Animation.animation reference to the fully assembled animation.

So that provides a framework for coding animations and a class hierarchy in order to enhance reuse and reduce boiler plate code in each animation.

So I tested the framework by coding dojox.fx.SizeTo?().

The result is the same. The parameters are stale, so clicking on the node being animated, results in it being reset to the original dimensions passed to dojo.animateProperty().

I did not expect this because refresh recalculates all the values referenced by dojo.animateProperty() each time, and a reference to the instance of dojox.fx.SizeTo? is passed to dojo.animateProperty().

Anyways seems like the issue lies in dojo.animateProperty().

Hope this is of some help.

Cheers,

  • Ole

Changed 12 years ago by guest

Attachment: Animation.js added

Changed 12 years ago by guest

Attachment: SizeTo.js added

Changed 12 years ago by guest

Attachment: Animation.html added

comment:7 Changed 11 years ago by dante

Description: modified (diff)
Milestone: 1.21.4

comment:8 Changed 11 years ago by dante

Resolution: fixed
Status: assignedclosed

(In [17318]) fixes #6060 - sizeTo no longer relies on cached parameters

comment:9 Changed 11 years ago by dante

didn't merge the framework, just fixed the underlying issue. Need to think about Animation details for 2.0 - we're sort of 'stuck' with the current API as it is.

comment:10 Changed 11 years ago by ole

Awesome fix - Thanks! Please let me know if there's anything I can do to help out with the 2.0 planning and implementation effort.

Note: See TracTickets for help on using tickets.