---------- Forwarded message ---------- Date: Tue, 21 Dec 2004 15:41:47 -0500 From: Carl Worth <cworth@...573...> To: Kristian Høgsberg <krh@...618...> Cc: cairo@...278... Subject: Re: [cairo] New PDF backend snapshot
--[[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 _______________________________________________ cairo mailing list cairo@...278... http://lists.freedesktop.org/mailman/listinfo/cairo