[cairo] New PDF backend snapshot

[Carl Worth]

--[[text/plain]]
Kristian,

Thanks so much for all your hard work on the PDF backend! I'm sorry
I've been so slow to review it. I think this is good work, and I want
it to land in CVS soon. I have a few comments, (most importantly with
respect to the public API).

On Wed, 15 Dec 2004 12:52:38 -0500, Kristian Høgsberg wrote:
> I've pondering the cairo_document_t idea a bit and restructured the PDF 
> backend so that it's split into a cairo_pdf_document_t object and a 
> cairo_pdf_surface_t object.  I believe this makes the code easier to 
> follow, but I'm no longer sure we need to expose a cairo_document_t in 
> the user API.

If it's easier to have two objects in the backend, that is fine.

> public API.  2) It will be a nice way to set metadata and other PDF 
> specific information for the document.

Can't we just do that with cairo_pdf_surface functions? What advantage
does the user get from having two different objects? So far, it seems
the user just has to do more work before being able to use the PDF
backend, (compared to the others).

> +void
> +cairo_set_target_pdf (cairo_t  *cr,
> +                     cairo_pdf_document_t *document);

The notion behind the cairo_set_target family of functions is to allow
the user to select a backend without having to create any new
object. So the cairo_set_target_pdf function should accept the same
arguments as cairo_pdf_surface_create.

So, I think I would like to see the new public cairo_pdf API match the
existing cairo_ps API exactly:

	cairo_surface_t *
	cairo_ps_surface_create (FILE *file,
				 double       width_inches,
				 double       height_inches,
				 double       x_pixels_per_inch,
				 double       y_pixels_per_inch);

	void
	cairo_set_target_pdf (cairo_t    *cr,
			      FILE       *file,
			      double     width_inches,
			      double     height_inches,
			      double     x_pixels_per_inch,
			      double     y_pixels_per_inch);

> There are two significant changes outside cairo_pdf_surface.c in this 
> patch: I've added the set_clip_trapezoids surface method which is 
> supposed to let cairo pass the absolute clipping path to the backends. 

(Kristian and I discussed this off-list and he has now proposed a new
patch without the set_clip_trapezoids method. See:

	http://people.redhat.com/krh/cairo-pdf/cairo-pdf-4.patch
)

> The other change is the addition of cairo_array_t implemented in 
> cario_array.c.  This is an standard autogrowing array, doubling the 
> allocation size when it runs out of space.  I was using chained lists 
> before in the PDF backend, but I converted those to arrays.  I added the 
> array as a general data type, since cairo uses the same type of array in 
> other places and it might make sense to rewrite those places to use this 
> array type.

Eliminating some code duplication seems like a good thing. But I don't
like giving up type safety, and worse, I don't want to give up direct
access of arrays using C [] notation. This kind of thing is always a
tough call since C has no support for what we really want here.

Regardless, if the code is to stay, then:

> +    if (old_size == 0)
> +	new_size = 1;
> +    else
> +	new_size = old_size * 2;
> +
> +    while (new_size < required_size)
> +	new_size = old_size * 2;

should be:

	new_size = new_size * 2;

to eliminate the infinite loop.

And these three functions:

> +static cairo_status_t
> +_cairo_pdf_surface_set_matrix (void             *abstract_surface,
> +                               cairo_matrix_t   *matrix)
> +
> +static cairo_status_t
> +_cairo_pdf_surface_set_filter (void             *abstract_surface,
> +                               cairo_filter_t   filter)
> +
> +static cairo_status_t
> +_cairo_pdf_surface_set_repeat (void             *abstract_surface,
> +                               int              repeat)

merit at least a comment stating they are unsupported.

This isn't *too* big a deal as I want to eliminate these functions
from the surface interface anyway, (they really only belong on
patterns).

Once the issues above are resolved I'd like to get this into CVS. I
haven't actually played with the PDF output yet, but we can have fun
playing with that after it lands in the tree.

Thanks again.

-Carl