Not approved yet.

Issues:

1) I don't follow what you are trying to do in LzUtils.  It might be more 
straightforward to say:

>         var color = 
>           ((parts[0] & 255) << 16) + 
>           ((parts[1] & 255) << 8) + 
>           (parts[2]  & 255); 

`&` being a bit operator will force the strings to be 32-bit ints, and `255` 
will force them to be in range, solving 2 problems at once.

2) I don't understand your color math.  setColorTransform implies that a tint 
color is made solely of the 'b' components of a transform, yet 
$lzc$set_tintcolor uses a transform that is made solely of 'a' components.  In 
converting the basecomponent tint code from using a transform to a tint, you've 
used yet a third computation (via hsv).  Shouldn't these all be the same 
conversion?  Shouldn't they be invertible?

3) It seems to me we ought to have a test that ensures that $lzc$set_tintcolor 
computes a transform matrix, which if I hand to setColorTransform computes the 
original tintcolor.  I don't believe the code in your change does that.

4) If the basecomponent code is really setting a tint, why is their so much 
math required to compute the color to set as the tint?  I'd like to see another 
test that verifies the old and new code result in the same sprite transform for 
a range of inputs.  I can't deduce from inspection that this is going to be the 
case.

On 2010-06-21, at 19:36, Max Carlson wrote:

> Change 20100621-maxcarlson-i by maxcarl...@friendly on 2010-06-21 16:31:42 PDT
>    in /Users/maxcarlson/openlaszlo/trunk-clean
>    for http://svn.openlaszlo.org/openlaszlo/trunk
> 
> Summary: Fix warnings with basecomponent tinting
> 
> Bugs Fixed: LPP-9132 - Regression: 
> examples/animation/animation.lzx?lzr=swf8&debug=true failing to start up
> 
> Technical Reviewer: ptw
> QA Reviewer: hminsky, mdemmon
> 
> Details: LzUtils - Ensure LzColorUtils.fromrgb() returns integers when no 
> alpha component is supplied.
> 
> LaszloView - Update docs, update deprecation warning to not include 'this' 
> which caused swf8 to fail to start up.  Updatei tintcolor setter 
> implementation to actually tint.
> 
> basecomponent - Use LzView.tintcolor instead of setColorTransform().
> 
> Tests: examples/animation/animation.lzx runs in swf8 debug mode, runs as 
> before in swf10 also - but without warnings.
> 
> Files:
> M       WEB-INF/lps/lfc/services/LzUtils.lzs
> M       WEB-INF/lps/lfc/views/LaszloView.lzs
> M       lps/components/base/basecomponent.lzx
> 
> Changeset: 
> http://svn.openlaszlo.org/openlaszlo/patches/20100621-maxcarlson-i.tar


Reply via email to