[cairo] API Shakeup: cairo_output_stream_t and cairo_surface_finish()

[Kristian Høgsberg]

Carl Worth wrote:
> 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.

Hehe, I was going through the same chain of thoughts when I implemented 
this.

> 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.

Hmm, I see what you mean.  I did pick the current ordering for a reason, 
though, but didn't consider consistency between these two functions. 
What I was thinking was that with cairo_surface_set_user_data(), you're 
attaching a piece of data to a surface.  The destroy notifier is a less 
important detail, and thus comes later in the argument list.  It's what 
g_object_set_data() from glib does.

For cairo_pdf_surface_create() it's the other way around; the important 
information is write_func, while the closure data is a detail required 
to implement it.  I think a reasonable parallel is g_signal_connect() 
from glib, which add a callback to a signal and takes the callback 
function pointer before the closure data.

So I guess what I'm getting at is that I think the order I chose is a 
bit more intuitive, if not consistent between the to functions.  I'm not 
sure I think that it's too important in this case.  Yes, things like the 
order of source and dest, data and length, x and y (hehe), should be the 
same, but in the case of cairo_surface_set_user_data() and 
cairo_pdf_surface_create() I would say we're talking about two different 
situations.  The user data is not just a closure.

> 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.

Yep.

>>   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.

Here I agree, the closure should be at the same position in the argument 
list in all cases.  Again, I was taking a clue from glib which always 
pass the closure argument as the last argument.  If you want it as the 
first argument, then that's what we do.

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

This is what write() takes.  Also, read, memcpy, memset all take void 
pointers and byte counts.  Given a void pointer and a "length" would you 
assume anything else than bytes?  Of course this should be documented (a 
detail I left out in my documentation).

>>   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.

Couldn't you say the same about cairo_show_page() and cairo_copy_page()?

>>+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.

Yeah, I've puzzled a bit over these names...

> 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 ?

This reads to me as "create for (foo bar)", that is, it breaks the 
association with the foo backend.  I think it's a good idea to throw in 
a preposition to emphasize that foo and bar are distinct.  How about

	cairo_create_for_foo_with_bar ?

For example

	cairo_create_for_image_with_data

I have yet another preposition in my png patch:

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

The situation here is slightly different.  The other surface 
constructors all reference and use the passed in resources throughout 
the lifetime (actually from creation to finish) of the surface.  In this 
case the png file is only used for initializing the contents of the 
surface.  When the constructor returns, file can be closed immediately. 
  I think 'from' implies this fairly well.

>> 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?

If cairo_finish() is going away, this case is moot.  Oh, and since we 
can't have a cairo_t without a surface, it's double-moot.

>>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?

Oops.

>>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?

Oops again.

> Other than that, the implementation looks fine to me.

Sounds good.

cheers,
Kristian