[Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2

[Simon Schampijer]

On 03/27/2011 11:35 PM, Gonzalo Odiard wrote:
> On Fri, Mar 25, 2011 at 5:20 PM, Sascha Silbe<silbe at activitycentral.com>wrote:
>
>> Excerpts from godiard's message of Wed Mar 23 19:40:47 +0100 2011:
>>
>>
>> Thanks for your patch! I can't wait for an accelerated launcher screen. ;)
>>
>
> Thanks for review this patch. I can't wait for faster startup in activities
> :)
> I attach a new patch and reply your comments here
>
>
>>
>>> This render the SVG image only one time, and then use alpha and scaling
>>> of the rendered icon and will speed up the overall activity startup.
>>
>> Any patch that claims improved performance needs to include proof of
>> that, i.e. test results plus a description of the test hardware and test
>> procedure so the results can be reproduced by the reviewer and/or a
>> third party.
>>
>
>
> We compared two XO with and without the patch and reading the lines like
> "a4a52fc2a33b5356bcce073513223d7a62fe38af launched in 2.128857 seconds."
> in shell.log
>
> All the activities started faster, in many cases 2 seconds faster.
> The effect is more visible in XO-1
> I don't know if this information must be included in the comment of the
> patch.
>
>
>>
>>> Another side effect is that the ticket #2080 effects are reduced greatly.
>>
>> Which side effects exactly? #2080 mentions a) empty window getting
>> displayed for several seconds and b) the launch window getting shown
>> after the activity was _closed_.
>>
>
> The effect we are talking is, in many activities, the startup zoom was not
> visible at all,
> because the svg rendering takes more time than the 0.1 second available
> before
> the next image must be displayed.
> With the patch you can see the zoom effect even in XO-1
>
>
>
>>
>>>      Signed-off-by: Gonzalo Odiard<gonzalo at laptop.org>
>>>      Acked-By: Simon Schampijer<simon at laptop.org>
>>
>> Did he (Ack your patch)?
>>
>>
> Of course. I will not add this line on my own.
>
>
>>
>> [src/jarabe/view/launcher.py]
>>
>> I like that you're getting rid of the HippoCanvas based class. I'm not
>> so sure about not using sugar.graphics.animator.Animation anymore,
>> though. The old code used an exponential formula while yours is purely
>> linear. We might want to check with the Design Team whether that makes
>> a difference to the user.
>>
>
>
>> [LaunchWindow.__destroy_cb(self, box)]
>>> +        self._activity_icon.props.pulsing = False
>>> +        self._home.disconnect_by_func(self.__active_activity_changed_cb)
>>
>> If the launcher and with it the icon gets destroyed anyway, why do we
>> need to tell it to stop pulsing? (Yes, I'm aware this was already the
>> case in the old code - just wondering whether we really need it).
>>
>>
> I think the old code is stopping the animation because the destroy can be
> delayed by python. Does not harm anyway.
>
>
>>
>> [src/jarabe/view/pulsingicon.py]
>>> +_MINIMAL_ALPHA_VALUE = 0.2
>>
>> How was this value determined?
>>
>>
> Experimentation and testing :)
> Anyway is a visual value, can be checked with Design Team too.
>
>
>>
>> [Pulser.__init__(self, icon)]
>>> -        self._level = 0
>>> +        self._level = 1.0
>>>           self._phase = 0
>>> +        self._start_scale = 1.0
>>> +        self._end_scale = 1.0
>>
>> I would prefer to use None for indicating no scaling is to take place
>> (this applies to the other patch as well). It's probably ok in this
>> particular place, but in general (in)equality comparisons of floating
>> point numbers are tricky at best.
>>
>>
> Ok. I agree. But in this case, the code is simplest in this way.
>
>
>>> +        self._zoom_steps = 1
>>> +        self._current_zoom_step = 1
>>> +        self._scale = 1.0
>>> +        self._current_scale_step = 1
>>
>> self._current_zoom_step seems to be a counter, duplicating self._phase
>> (though scaling by 1/_STEP). self._current_scale_step OTOH seems to be a
>> scaling factor. Please rename the variables appropriately and if
>> possible eliminate some of them (self._phase and self._level would be
>> good candidates).
>>
>>
> self._current_zoom_step is not the same than self._phase, because the zoom
> can stop and the pulsing continues. Also you can have have a pulsing icon
> without
> zooming at all (like in the neighborhood view).
>
> You are righth about self._phase and self._level,  can be simplified, i did
> it.
>
>
>
>> Do we need to keep a copy of the current scale (i.e. self._scale)?
>> AFAICT assigning directly to self._icon.scale should suffice.
>>
>
> Ok.
>
>
>> [Pulser]
>>> +    def set_zooming_properties(self, start_scale, end_scale,
>> zoom_steps):
>>> +        self._start_scale = start_scale
>>> +        self._end_scale = end_scale
>>> +        self._zoom_steps = zoom_steps
>>> +        self._current_scale_step = abs(self._start_scale -
>> self._end_scale) / \
>>> +                self._zoom_steps
>>> +        self._scale = self._start_scale
>>> +        self._icon.scale = self._scale
>>
>> Please provide a docstring explaining what this function is about (see
>> PEP 257). I also wonder if the caller isn't more interested in
>> specifying a time interval rather than the number of steps.
>>
>>
> Ok
>
>
>> set_zooming_properties sounds a bit strange, but I can't think of a much
>> better name. set_zooming might be a start - it should at least get rid
>> of some of the line continuations. :)

I think the 'set_zooming_properties' was my idea, it does explain what 
it does fine to me :)

All the rest looks fine - my 'Ack' does still stand.

Regards,
    Simon