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