Re: RGB UI Diff done using -u

[Nat Friedman <nat helixcode com>]

Ok, I just tried this patch.  Unfortunately some of the long lines
wrapped, so I had to hand-edit it before applying.  Your mailer must
be screwing up the diff.

In any case, the patch has some problems.  I fixed the configure.in
stuff, and a few other things, but the patch still has big issues.

"Gene Z. Ragan" <gzr@eazel.com> writes:
> +static gpointer
> +bonobo_ui_handler_pixmap_rgb_copy_data (const gconstpointer src)

This function needs to check to make sure that src->art_pixbuf is not
NULL before copying; otherwise, it will cause a segfault in the case
of a bogus GdkPixbuf.
> +               /* Create GtkPixmap to return */
> +               pixmap = gtk_pixmap_new (gdk_pixmap, gdk_bitmap);
>                 break;

Don't you need to unref the gdk pixmap/bitmap after this?

>         case BONOBO_UI_HANDLER_PIXMAP_RGBA_DATA:
> -               g_warning ("Unsupported pixmap type (RGB[A]_DATA)\n");
> +               g_free(pixmap_info);
>                 break;

Presumable pixmap_info is a pointer to a GdkPixbuf here.  I think you
need to free this properly.

>  bonobo_ui_handler_pixmap_data_to_corba (BonoboUIHandlerPixmapType type,

This function is the real cause of the problems.  You can't just blit
the pixels across to the remote end; when you do that, the geometry
information is lost.  How are you ever going to convert the CORBA
buffer that hits the other end into a usable GdkPixbuf?  You can't.

And you don't.  Which is why this code doesn't work at all.  It just
blasts the pixels across the corba connection, and then tries to cast
the pixel buffer to a GdkPixbuf.  Which, of course, doesn't work.  You
also use CORBA_sequence_Bonobo_Unknown_allocbuf to allocate the
sequence, which is for allocating sequences of Bonobo_Unknown objects,
not sequences of bytes.  Please make sure you're compiling with -Wall
so you catch these things.

For XPMs, I had to flatten the image from being a 2d array (list of
row pointers) into a 1d array before I could marshal it across.  You
have to do similar magic here.

You really should test this stuff before submitting it; I was under
the impression this was a tested patch, which was why I was spending
time reviewing it in the first place.  Here's a working/up-to-date
configure.in/Makefile.am, though (attached at the end).

If you want me to explain any of this further, please feel free to
ask.

Nat

configure.in

bonobo/Makefile.am