[Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
From: Gonzalo Odiard
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.
Another side effect is that the ticket #2080 effects are reduced greatly.
Signed-off-by: Gonzalo Odiard
Acked-By: Simon Schampijer
---
src/jarabe/view/launcher.py| 83 +--
src/jarabe/view/pulsingicon.py | 82 ++-
2 files changed, 65 insertions(+), 100 deletions(-)
diff --git a/src/jarabe/view/launcher.py b/src/jarabe/view/launcher.py
index 89251e5..7437d21 100644
--- a/src/jarabe/view/launcher.py
+++ b/src/jarabe/view/launcher.py
@@ -18,16 +18,13 @@ import logging
from gettext import gettext as _
import gtk
-import hippo
import gobject
from sugar import wm
from sugar.graphics import style
-from sugar.graphics import animator
-from sugar.graphics.xocolor import XoColor
from jarabe.model import shell
-from jarabe.view.pulsingicon import CanvasPulsingIcon
+from jarabe.view.pulsingicon import PulsingIcon
class LaunchWindow(gtk.Window):
@@ -51,12 +48,15 @@ class LaunchWindow(gtk.Window):
canvas.pack_start(header, expand=False)
self._activity_id = activity_id
-self._box = LaunchBox(activity_id, icon_path, icon_color)
-box = hippo.Canvas()
-box.modify_bg(gtk.STATE_NORMAL, style.COLOR_WHITE.get_gdk_color())
-box.set_root(self._box)
-box.show()
-canvas.pack_start(box)
+
+self._activity_icon = PulsingIcon(file=icon_path,
+ pixel_size=style.XLARGE_ICON_SIZE)
+self._activity_icon.set_base_color(icon_color)
+self._activity_icon.set_zooming_properties(style.SMALL_ICON_SIZE,
+ style.XLARGE_ICON_SIZE, 10)
+self._activity_icon.set_pulsing(True)
+self._activity_icon.show()
+canvas.pack_start(self._activity_icon)
footer = gtk.VBox(spacing=style.DEFAULT_SPACING)
footer.set_size_request(-1, bar_size)
@@ -78,11 +78,16 @@ class LaunchWindow(gtk.Window):
screen = gtk.gdk.screen_get_default()
screen.connect('size-changed', self.__size_changed_cb)
+self._home = shell.get_model()
+self._home.connect('active-activity-changed',
+ self.__active_activity_changed_cb)
+
+self.connect('destroy', self.__destroy_cb)
+
self._update_size()
def show(self):
self.present()
-self._box.zoom_in()
def _update_size(self):
self.resize(gtk.gdk.screen_width(), gtk.gdk.screen_height())
@@ -95,65 +100,15 @@ class LaunchWindow(gtk.Window):
def __size_changed_cb(self, screen):
self._update_size()
-
-class LaunchBox(hippo.CanvasBox):
-
-def __init__(self, activity_id, icon_path, icon_color):
-gobject.GObject.__init__(self, orientation=hippo.ORIENTATION_VERTICAL)
-
-self._activity_id = activity_id
-self._activity_icon = CanvasPulsingIcon(
-file_name=icon_path,
-pulse_color=icon_color,
-background_color=style.COLOR_WHITE.get_gdk_color())
-self.append(self._activity_icon, hippo.PACK_EXPAND)
-
-# FIXME support non-xo colors in CanvasPulsingIcon
-self._activity_icon.props.base_color = \
-XoColor('%s,%s' % (style.COLOR_BUTTON_GREY.get_svg(),
- style.COLOR_TRANSPARENT.get_svg()))
-
-self._animator = animator.Animator(1.0)
-
-self._home = shell.get_model()
-self._home.connect('active-activity-changed',
- self.__active_activity_changed_cb)
-
-self.connect('destroy', self.__destroy_cb)
-
-def __destroy_cb(self, box):
-self._activity_icon.props.pulsing = False
-self._home.disconnect_by_func(self.__active_activity_changed_cb)
-
-def zoom_in(self):
-self._activity_icon.props.size = style.STANDARD_ICON_SIZE
-
-self._animator.remove_all()
-self._animator.add(_Animation(self._activity_icon,
- style.STANDARD_ICON_SIZE,
- style.XLARGE_ICON_SIZE))
-self._animator.start()
-self._activity_icon.props.pulsing = True
-
def __active_activity_changed_cb(self, model, activity):
if activity.get_activity_id() == self._activity_id:
self._activity_icon.props.paused = False
else:
self._activity_icon.props.paused = True
-
-class _Animation(animator.Animation):
-
-def __init__(self, icon, start_size, end_size):
-animator.Animation.__init__(self, 0.0, 1.0)
-
-self._icon = icon
-self.start_size = start_size
-self.end_size = end_size
-
-def next_frame(self, current):
-d = (self.end_size - self.start_size) * current
-self._icon.props.size = int(sel
Re: [Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
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 > Acked-By: Simon Schampijer 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/ signature.asc Description: PGP signature ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
On Fri, Mar 25, 2011 at 5:20 PM, Sascha Silbe 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 > > Acked-By: Simon Schampijer > > 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
Re: [Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
On 03/27/2011 11:35 PM, Gonzalo Odiard wrote: On Fri, Mar 25, 2011 at 5:20 PM, Sascha Silbewrote: 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 Acked-By: Simon Schampijer 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. :) I think the 'set_zooming_properties' was my idea, it does expla
Re: [Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
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 ev
Re: [Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
On 03/31/2011 02:20 PM, Sascha Silbe wrote: 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 have tested Gonzalo's patches and I do not see that us much of an issue. I did as well test the launcher in b/w but does look fine as well. Regards, Simon ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH] Do startup animation of the activity icon using scaling and alpha - v2
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. > > Hi Sasha, I have proposed this to the Design Team, and had a positive review. Gary proposed a better approach, but we agreed in doing the change after this patch land. Gonzalo ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
