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

[Sascha Silbe]

Excerpts from Gonzalo Odiard's message of Mon Mar 28 05:35:22 +0200 2011:

> Thanks for review this patch. I can't wait for faster startup in activities
> :)
> I attach a new patch and reply your comments here

I accidentally pulled in the OLPC Sugar 0.92 packages into my Dextrose
test images and they seem to already include your patch. The animation
is faster, but looks quite different: With the old behaviour, the icon
is always visible and just cross-fades the two colours. With your patch
however, the entire icon (i.e. both colours) is faded and (almost)
disappears from screen, making it harder to recognise (especially in
non-optimal lighting conditions). This is a significant UI change. And
it doesn't affect just the launcher animation as your patch description
suggests, but all pulsing icons - e.g. the wireless device icons in the
Neighbourhood and the Frame.

I don't think the patch qualifies for inclusion in 0.92.x (it's not a
bug fix). And if possible, it should be reworked before merging into
0.94 to avoid the UI change mentioned above. If there's a trade-off to
be made between performance and UI behaviour (i.e. if we can't fix the
patch without making the animation slow again), we should include the
Design Team in our decision process.

Sorry about the confusion that results from Simon having already Ack'ed
your patch; we clearly need to improve our internal coordination (the
same happened in the other direction as well).


> We compared two XO with and without the patch and reading the lines like
> "a4a52fc2a33b5356bcce073513223d7a62fe38af launched in 2.128857 seconds."
> in shell.log

Please include the timings (number of runs, average times, standard
deviation) in the patch description.

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

OK. We can handle the "pulsing icon is sometimes delayed by a few
seconds, during which you only get to see a white window" part of the
original report in #2080 [1] (so please append "(SL#2080)" to your patch
summary) and the remaining issues in separate tickets (like #2565).

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

Ah, I see. Thanks!

> > [src/jarabe/view/pulsingicon.py]
> > > +_MINIMAL_ALPHA_VALUE = 0.2
> >
> > How was this value determined?
> Experimentation and testing :)

We should document the criteria for choosing this value then.

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

Hmm, I checked the code and there doesn't seem to be any place where
setting self._start_scale and self._end_scale to None instead of 1.0
would make a difference. If we were to set self._zoom_steps to None,
we'd just need to swap the two comparisons in __pulse_cb():

+        if self._start_scale != self._end_scale and \
+            self._current_zoom_step <= self._zoom_steps:

If self._start_scale and self._end_scale are None, the first comparison
will return True and short-circuit the second one.

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

Ok, I see now.

> You are righth about self._phase and self._level,  can be simplified, i did
> it.

Looks better now, thanks!

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

Hmm, I imagined a bit more information than the function and parameter
names already provided. ;)

How about this:

        """Configure zoom effect.

        The icon will be shown at the fraction (0-1.0) of the allocated
        display space given by start_scale. Over the course of zoom_steps
        discrete zooming steps (time interval 0.1s) the icon will be enlarged
        or shrunken to reach its final size, again given as a fraction of the
        display space. The zoom animation will not be repeated.

        Pulsing will happen in parallel with zooming, but continue even after
        the zoom animation has stopped.

        """
        

BTW, we should consider whether we actually want the pulsing to continue
in parallel with zooming, or start the pulsing only once the zooming has
finished.


P.S.: Including the patch inline instead of attaching it makes it easier
      for me to review, as I can then quote parts of the patch.

Sascha

[1] http://bugs.sugarlabs.org/ticket/2080
[2] http://bugs.sugarlabs.org/ticket/2565
-- 
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/20110331/43eb6da0/attachment.pgp>