[Sugar-devel] [PATCH sugar-toolkit] icon: use GDK for pixbuf-to-cairo conversion

[Daniel Drake]

On Wed, Mar 7, 2012 at 8:32 AM, Simon Schampijer <simon at schampijer.de> wrote:
> Yes, your patch does fix the breakage in F17. The breakage has been
> introduced between the alpha release day and today, I wonder what package
> exactly caused it. Out of curiosity, Do you know that?

I guess the hippo code here is causing some memory corruption, but it
was never solidly identified until now.
This is further supported by the fact that making some other code
changes (like the constructor chaining change I mentioned on IRC)
avoided this crash in at least 1 situation - its sensitive to
unrelated changes in the environment.

> Can't we do the same than in toolkit-gtk3? Or do I misinterpret your words.
> The code from below works for me.
>
> diff --git a/src/sugar/graphics/icon.py b/src/sugar/graphics/icon.py
> index 3f540d7..1736c27 100644
> --- a/src/sugar/graphics/icon.py
> +++ b/src/sugar/graphics/icon.py
> @@ -280,10 +280,9 @@ class _IconBuffer(object):
>
>             surface = cairo.ImageSurface(cairo.FORMAT_RGB24, int(width),
>                                          int(height))
>             context = cairo.Context(surface)
> -            context.set_source_rgb(self.background_color.red,
> -                                   self.background_color.blue,
> -                                   self.background_color.green)
>
> -            context.paint()
> +            gdkcontext = gtk.gdk.CairoContext(context)
> +            gdkcontext.set_source_color(self.background_color)
> +            gdkcontext.paint()

This approach would work (basically the code before my patch but with
a variable name change), but it seems pointless/wasteful to have to
encapsulate it as a CairoContext just to set the source color, when it
is trivial to extract the color components and call cairo directly.

The purpose of my change here was to exit the 'if' condition with a
known type of 'context' (previously it would either be cairo.Context
or gtk.gdk.CairoContext depending on which 'if' branch was hit). Both
my code and your proposed code avoid that - in both cases, context
will be cairo.Context - but I would argue that reading the RGB values
from a color is slightly better than having to instantiate a new
wrapper object every time.

Also, this is the sugar-toolkit-gtk3 code you are looking at:

            context = cairo.Context(surface)
            context = Gdk.CairoContext(context)
            context.set_source_color(self.background_color)
            context.paint()

This will actually fail in sugar-toolkit-gtk3 because Gdk.CairoContext
does not exist. The correct thing to use there is
Gdk.cairo_set_source_color.

Daniel