[cairo] API Shakeup: cairo_output_stream_t and cairo_surface_finish()

[Carl Worth]

On Mon, 21 Feb 2005 20:52:26 -0500, Kristian Høgsberg wrote:
> And here's a first stab at the implementation.

Thanks for putting in this work, Kristian! Comments below.

> I decided to go with the
> API that keeps the cairo_output_stream_t out of the public API:

I think I prefer that as well, (at least compared with the earlier
version that had cairo_output_stream_create). Owen had a pretty
reasonable suggestion for a rule-of-thumb that no function should
accept more than one function pointer.

If we were to adopt that, then I propose providing an exposed
structure that the user can directly initialize with the function
pointers.

But there's not a huge win there, (particularly since C structure
initializers have the same user-must-get-the-order-right problems as C
function calls). I don't anticipate we'll need other functions with
multiple callbacks, so maybe we're better off keeping the usage of
this function similar in style to cairo_surface_set_user_data.

Speaking of which, the current proposal for cairo_surface_set_user_data

has the arguments ordered "data, destroy_func", while here we have
"destroy_func, closure". I'd like to see something more consistent
here. So, if we can come up with a small guiding principle for
argument ordering, that would help.

>    cairo_surface_t *
>    cairo_pdf_surface_create (cairo_write_func_t    write_func,
>                              cairo_destroy_func_t  destroy_closure_func,
>                              void                  *closure,
>                              double                width_inches,
>                              double                height_inches,
>                              double                x_pixels_per_inch,
>                              double                y_pixels_per_inch);

We haven't mentioned it yet as part of the API Shakeup, but we did
talk about this in the meeting:

The "pixels_per_inch" parameters are not really an intrinsic part of
a PDF surface. There really only needed for things like image-based
fallbacks, and perhaps trimming down the size of embedded image
data. So, I propose removing these arguments from the constructor,
specifying some per-backend default value, and adding a
cairo_surface_set_pixels_per_inch function instead.

>    typedef cairo_status_t (*cairo_write_func_t) (const void *data,
>                                                  unsigned int length,
>                                                  void *closure);

Here's another opportunity for making the argument order
consistent. I propose that the closure argument should be the first
argument passed to any callback.

And should data really be a "void *" rather than "char *". Otherwise,
what does length mean?

>    void
>    cairo_finish (cairo_t *cr);

I may have originally proposed this function, but as I mentioned in
the separate cairo_surface_finish thread, I now think it's a
gratuitous convenience function that we should eliminate.

> +void
> +cairo_set_target_pdf_as_file (cairo_t	*cr,

I'm not sure about the "as" naming here. Of course, this will change a
bit with the elimination of set_target in favor of cairo_create
convenience functions.

The mapping of basic surface_create functions to cairo_create
convenience functions seems straightforward:

	cairo_foo_surface_create -> cairo_create_for_foo

But, the "overloaded" surface create functions are bit trickier:

	cairo_foo_surface_create_for_bar -> ?

Perhaps just:

	cairo_create_for_foo_bar ?

>  cairo_status_t
> +_cairo_gstate_finish_target_surface (cairo_gstate_t *gstate)
> +{
> +    if (gstate->surface)
> +	return cairo_surface_finish (gstate->surface);
> +    else
> +	return CAIRO_STATUS_NO_TARGET_SURFACE;
> +}

The else case looks harmless to me here, do we really want to trigger
an error?

> Index: src/cairo_win32_surface.c
> ===================================================================
> RCS file: /cvs/cairo/cairo/src/cairo_win32_surface.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 cairo_win32_surface.c
> --- src/cairo_win32_surface.c	3 Feb 2005 02:57:40 -0000	1.4
> +++ src/cairo_win32_surface.c	21 Feb 2005 22:01:20 -0000
> @@ -352,8 +352,6 @@ _cairo_win32_surface_destroy (void *abst
>  
>      if (surface->bitmap)
>  	DeleteObject (surface->bitmap);
> -
> -    free (surface);
>  }

Is the function above missing a rename to _cairo_win32_surface_finish?

> Index: src/cairo_xcb_surface.c
> ===================================================================
> RCS file: /cvs/cairo/cairo/src/cairo_xcb_surface.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 cairo_xcb_surface.c
> --- src/cairo_xcb_surface.c	25 Jan 2005 20:11:02 -0000	1.15
> +++ src/cairo_xcb_surface.c	21 Feb 2005 22:01:20 -0000
> @@ -291,8 +291,6 @@ _cairo_xcb_surface_destroy (void *abstra
>  	XCBFreeGC (surface->dpy, surface->gc);
>  
>      surface->dpy = 0;
> -
> -    free (surface);
>  }

Same here?

Other than that, the implementation looks fine to me.

-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/20050301/f5c3279e/attachment.pgp