[cairo] API Shakeup: remove png backend

[Carl Worth]

On Sun, 27 Feb 2005 14:51:38 -0500, Kristian Høgsberg wrote:
> We've been discussing dropping the PNG backend for a while, and I 
> believe that we've reached consensus that it makes sense to phase it out 
> in favor of a couple of PNG helper functions.  The attached patch plus 
> removal of cairo_png_surface.c implements this.

Looks good in general. Thanks for doing this! A few minor points
below.

>    cairo_status_t
>    cairo_image_surface_write_png (cairo_surface_t *surface,
>                                   FILE            *file);
> 
> I'm thinking we should have a write callback base function as well:
> 
>    cairo_status_t
>    cairo_image_surface_write_png_with_callback (cairo_surface_t *surface,
>                                                 cairo_write_func_t write,
>                                                 void *closure);

Yeah, we'll probably want both of these.

But the naming convention here is reversed compared to what we have
for cairo_pdf_surface_create, (which also has FILE and callback
forms).

I think we should make these match. And I prefer the approach above,
(where the FILE* interface, which is obviously more convenient to use,
also has the shorter name).

> What error do we return if the surface isn't an image surface?

CAIRO_STATUS_SURFACE_TYPE_MISMATCH ?

We'll probably have a lot of backend-specific surface functions
eventually.

>    cairo_surface_t *
>    cairo_image_surface_create_from_png (FILE *file,
>                                         int  *width,
>                                         int  *height);

Should this be "for_png" instead of "from_png". They seem close enough
to be arbitrary to me, and if they are, I'm sure to always get them
wrong.

> There's a problem here, that the function can fail if the file is not a 
> PNG file.  The only means we have of reporting error is returning NULL, 
> since we want it to follow the same convention as the other 
> constructors.  I was thinking of something like:
> 
>    cairo_surface_t *
>    cairo_image_surface_create_from_png (FILE *file,
>                                         int *width, int *height,
>                                         cairo_status_t *status);

Yeah, the error handling here is awkward.

One approach I've almost considered is to make every function return
type be either void or cairo_status_t. That moves all "return values"
into pointer parameters. That's almost what we have for all of the
cairo_t API anyway.

But that would also be pretty awkward for the create functions.

The other approach is to take advantage of the plan to return dummy
objects for failure cases. We could have one dummy object per failure
mode and allow the user to query the status of the dummy object.

That would allow the error to propagate painlessly to the cairo_t if
the surface was used there.

There are some things that a user might do that would force the user
to check the surface status first in order to be safe,
(eg. cairo_image_surface_get_data), but that's no worse than if we
provided the "cairo_status_t *status" as above. And when the user
doesn't need to check the status, the call is actually simpler.

>   * Contributor(s):
> - *	Carl D. Worth <cworth at isi.edu>
> + *	Carl D. Worth <cworth at cworth.org>

Heh, thanks for catching that. :)

I don't mean any disrespect to ISI. It's a fantastic institution and
I'd be glad to continue to leave that address there to give credit
where due, but it was the ISI administrative staff that shutdown that
address on me. Oh well, they lose their free advertising I guess.

> +/* XXX: Setting the time is interfereing with the image comparison
> +    png_convert_from_time_t (&png_time, time (NULL));
> +    png_set_tIME (png, info, &png_time);
> +*/

Oh, this was a cheesy hack so that I could just use "diff" in the test
suite. I don't think we need that to be commented out anymore.

> +    /* XXX: Perhaps we'll want some other error handlers? */
> +    png = png_create_read_struct (PNG_LIBPNG_VER_STRING,
> +                                  NULL,
> +                                  NULL,
> +                                  NULL);

If we've got a plan for error handling now, do we need to fill those
in?

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050318/6cfb85e5/attachment.pgp