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

[Gonzalo Odiard]

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. :)
>
>
If you are happy .... :)


>
> [Pulser.__pulse_cb(self)]
> > -        self._level = (math.sin(self._phase) + 1) / 2
> > +        self._level = _MINIMAL_ALPHA_VALUE + \
> > +                (1 - _MINIMAL_ALPHA_VALUE) * (math.sin(self._phase) + 1)
> / 2
> > +        if self._current_zoom_step <= self._zoom_steps:
> > +            if self._start_scale != self._end_scale:
>
> These two ifs can be combined to a single one, thus removing one level
> of indentation.
>
>
Ok.


> [PulsingIcon]
> > +    def set_zooming_properties(self, start_size=style.SMALL_ICON_SIZE,
> > +                               end_size=style.XLARGE_ICON_SIZE,
> > +                               zoom_steps=10):
> > +        if start_size > end_size:
> > +            start_scale = 1.0
> > +            end_scale = float(end_size) / float(start_size)
>
> You don't need to convert the second operand to float. If the first
> operand is float, the result will always be float.
>

Ok


>
> > +        if end_size > start_size:
> > +            start_scale = float(start_size) / float(end_size)
> > +            end_scale = 1.0
>
> Replacing the second if with an else would avoid throwing a NameError if
> start_size == end_size.
>

Ok


>
> Sascha
>
> --
> http://sascha.silbe.org/
> http://www.infra-silbe.de/
>



-- 
Gonzalo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110328/7587ba80/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Do-startup-animation-of-the-activity-icon-using-scal.patch
Type: text/x-diff
Size: 10058 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110328/7587ba80/attachment-0001.patch>