Change 20100526-maxcarlson-9 by maxcarl...@friendly on 2010-05-26 16:35:11 PDT
in /Users/maxcarlson/openlaszlo/trunk-clean
for http://svn.openlaszlo.org/openlaszlo/trunk
Summary: Clean up LzAnimator/LzNode interaction, cpmprehensive
Bugs Fixed: LPP-9020 - Support CSS3 transitions (partial)
Technical Reviewer: [email protected]
QA Reviewer: ptw
Details: Updated to address Andre's comments:
>> LzAnimatorGroup.__LZhalt() also sends onstop. Bret asked me to add
>> this to ensure onstop is fired after the screen has had a chance to
>> update, and this seemed like the cleanest way for now.
Could you split changes which introduce new behaviour in different change sets?
This makes it easier to test changes and to back out these changes if necessary
due to regression and so forth.
Also, moved all animator state into a single object -
LzAnimator.__animatedAttributes.
Otherwise:
Updated to address Tucker's comments:
1) Since we're removing all the substrate from LzNode, the comments that say
'inlined LzNode...' make no sense any more.
Done.
2) The logic with expectedvalue is just too convoluted for me to follow. This
expression (expectedvalue != null ? expectedvalue : targ[attr]) is repeated
over and over, which can't be efficient. AFAICT, the only time expected value
will be null is if the attribute is transitioning from not animated to
animated. So, wouldn't it be simpler to first test if there is a counter
entry, and if there is not, set the counter to 0 and the expected value to the
current value? Then the rest of the logic can simply work with the counter and
the expected value without all the redundant conditionalization. When all the
animations are done, you remove the counter (and optionally the expected
value), so that you know the attribute is no longer animating.
Agreed.
3) It seems to me if your counter goes negative, you have a bug. Why are we
papering over this?
Agreed. I added a warning to catch this in debug mode for now.
Issue:
1) I don't understand why you have to introduce another delegate to call halt
from finalize from stop? All halt does is unregister the animator.
LzAnimatorGroup.__LZhalt() also sends onstop. Bret asked me to add this to
ensure onstop is fired after the screen has had a chance to update, and this
seemed like the cleanest way for now.
Otherwise, the same:
Updated to address Andre and Tucker's comments:
> + this.__animExpected = {};
> + this.__animCounter = {};
Only a few nodes will be animated in an application, so both hashes should be
built lazily. Otherwise every node construction will cost +2 Objects.
Fixed.
> [...]
> + var __animExpected = {};
> [...]
> + var __animCounter = {};
Every node construction will cost +4 Objects for swf9/10
(http://jira.openlaszlo.org/jira/browse/LPP-5232).
Fixed.
> @@ -218,23 +227,32 @@
> var attr:String = this.attribute;
>
> var from:* = this.from;
> + var expected = targ.__animExpected;
> + var expectedvalue = (expected[attr] != null ? expected[attr] :
> targ[attr]); // inlined targ.getExpectedAttribute(attr);
> if (from != null) {
> targ.setAttribute(attr, from);
> - var d:Number = from - targ.getExpectedAttribute(attr);
> - targ.addToExpectedAttribute(attr, d);
> + var d:Number = from - expectedvalue;
> + // inlined targ.addToExpectedAttribute(attr, d);
> + var current = expected[attr];
> + expected[attr] = (current != null ? current : targ[attr]) + d;
> }
>
> if (! this.relative) {
> - this.to = this.origto - targ.getExpectedAttribute(attr);
> + this.to = this.origto - expectedvalue;
This seems to be wrong:
expected[attr] is updated (inlined call to addToExpectedAttribute), but later
"expectedvalue" is used again.
Fixed.
On 5/25/2010 4:54 PM, P T Withington wrote:
> Comments:
>
> 1) I'd just remove all the animator functions you commented out of LzNode.
> They are clutter and will (already for one of them) rot. svn can find them
> if we really need them back.
>
You mean to rot like setExpectedAttribute() which wasn't updated to use
__animExpected ;-)
Fixed.
> 2) Rather than creating multiple object hashes for the various animation
> attributes, why not define a class (data structure) that holds all the
> animation properties of an attribute? Thus, when animating an attribute, you
> will just say something like:
>
> var aas = this.__animatedAttributes;
> if (! aas[name]) { aas[name] = new AnimationAttribute(start, end, ...);
>
> ?
Fixed.
Also in stop()
> var chash = targ.__animCounter;
> var counter = chash[attr];
> if (counter == null || counter - 1<= 0) {
> chash[attr] = 0;
> targ.__animExpected[attr] = null;
> } else {
> chash[attr] = counter - 1;
> targ.__animExpected[attr] = this.to - this.currentValue;
> }
>
> /*
> var e_prop:String = "e_" + (this.attribute cast String);
> if (! targ[e_prop].c) {
> targ[e_prop].c = 0;
> }
> targ[e_prop].c -= 1; //decrement animation counter for prop
> if (targ[e_prop].c<= 0) {
> targ[e_prop].c = 0;
> targ[e_prop].v = null;
> } else {
> targ[e_prop].v -= this.to - this.currentValue;
> }
> */
>
1) the complete commented out block seems to be unnecessary
Fixed.
2)
targ[e_prop].v -= this.to - this.currentValue;
is changed to
targ.__animExpected[attr] = this.to - this.currentValue;
Shouldn't that be -= ?
Good catch. Fixed.
And I still think you cannot reuse "expectedvalue" here:
> if (! this.relative) {
> - this.to = this.origto - targ.getExpectedAttribute(attr);
> + this.to = this.origto - expectedvalue;
>
Fixed.
LzNode - Remove all animator-related code.
LzAnimatorGroup - Update comments, remove cruft, update types. Prevent
duplicate calls to doStart() or stop().
LaszloAnimation - Use LzAnimator.__animatedAttributes to track animation
counter and expected values. Shorten lookups. Fix bugs in reviewer comments.
Tests: Visual inspection, animators work as before across all runtimes.
Files:
M WEB-INF/lps/lfc/core/LzNode.lzs
M WEB-INF/lps/lfc/controllers/LzAnimatorGroup.lzs
M WEB-INF/lps/lfc/controllers/LaszloAnimation.lzs
Changeset:
http://svn.openlaszlo.org/openlaszlo/patches/20100526-maxcarlson-9.tar
