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

[Sascha Silbe]

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. ;)

> 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.

> 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_.

>     Signed-off-by: Gonzalo Odiard <gonzalo at laptop.org>
>     Acked-By: Simon Schampijer <simon at laptop.org>

Did he (Ack your patch)?


[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).


[src/jarabe/view/pulsingicon.py]
> +_MINIMAL_ALPHA_VALUE = 0.2

How was this value determined?


[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.

> +        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).

Do we need to keep a copy of the current scale (i.e. self._scale)?
AFAICT assigning directly to self._icon.scale should suffice.


[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.

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. :)


[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.


[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.

> +        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.

Sascha

-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 500 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110325/188138bf/attachment.pgp>