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/
signature.asc
Description: PGP signature
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel