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: UPDATED: Clean up LzAnimator/LzNode interaction, part 2
Bugs Fixed: LPP-9020 - Support CSS3 transitions (partial)
Technical Reviewer: [email protected]
QA Reviewer: ptw
Details: 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 - Use a single property to track animator values.
LzAnimatorGroup - Update comments, remove cruft. Update __LZhalt so it
can be called from a delegate.
LaszloAnimation - Use attributename + '__lzcounter' to track animation
counter 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
