[cairo] cairo_draw_with_xlib API

[Carl Worth]

On Thu, 23 Feb 2006 16:39:14 +1300, Robert O'Callahan wrote:
> We (Mozilla) need a way to take existing code that draws to an X
> drawable and redirect the drawing to an arbitrary cairo context, so that
...
> To support this implementation, we have extended cairo in two ways:

Ahah! You managed to sneak in the supporting patches at the end of
your message and I almost missed them. :-)

> -- Added a number of getter functions to extract properties of a cairo
> xlib surface

I don't likethe uncertainty of the current patch with regard to
type-mismatch errors:

	/* XXX: How do we want to handle this error case? */

(And yes, I realize that that comment was in the code already, was
something I typed, and this patch is just maintaining existing style.)

A pre-requisite for fixing this is that the caller must be able to
determine the surface type and ensure that mismatch never
occurs. That's the motivation for the cairo_surface_get_type patch I
just posted in a separate thread. With that, the user can use a style
such as:

	assert (cairo_surface_get_type (surface) == CAIRO_SURFACE_TYPE_XLIB);
	drawable = cairo_xlib_surface_get_drawable (surface);

and then that user is immune from any mismatch behavior we decide.

But we need to decide anyway.

I did a small survey of similar cases in cairo, and found a fair
amount of apparent inconsistency.

1) Check for mismatch, and shutdown the object (see
   cairo_pattern_add_color_stop).

2) Check for mismatch, call _cairo_error, return a "zero" value, but
   give no direct notification to the user that an error occurred (see
   cairo_image_surface_get_width).

3) Don't check at all for mismatch, just march on blindly (see
   cairo_ft_scaled_font_lock_face).

I like option (2) for the current patch, which would mean removing the
XXX comment and adding a call to _cairo_error.

This is actually consistent with (1) too. The distinction is that
objects should only be shutdown if the function's behavior is to
modify the object, (so "getters" are exempt from shutdown on error).

And (2) is certainly easier than returning a status and moving the
"real" return value to a pointer parameter. That just makes cairo
harder to use, and would result in users often ignoring status
returns, (which is something that should indicate an error in cairo
usage the way it is designed).

I think every occurence of (3) is a bug.

> -- Added cairo_extract_clip_rectangles to obtain the current clip in
> device coordinates. If the clip cannot be expressed as rectangles, the
> function simply gives up ... for our purposes, a non-rectangular clip is
> useless anyway.

Allowing the user to get at the clip seems perfectly reasonable. But I
envision slightly different API for it. See below.

> +static cairo_bool_t _is_valid_xlib_surface (cairo_surface_t *abstract_surface)
> +{

The function name should be at the beginning of the next line.

> +/**
> + * cairo_clip_rect_t:
> + * 
> + * A data structure for holding clip rectangles.
> + */
> +typedef struct _cairo_clip_rect {
> +  double x, y, width, height;
> +} cairo_clip_rect_t;

There's not really anything clip-specific about this structure, right?
And I don't like the "rect" abbreviation. If we were to export this
structure, I would expect it to be named cairo_rectangle_t. (That
would certainly be more awkard to implement given the name clash with
the current internal cairo_rectangle_t, but we could just first name
that cairo_rectangle_fixed_t.)

> +cairo_public cairo_bool_t
> +cairo_has_clip (cairo_t *cr);

Hmm... there's always a clip. Initially it can be thought of as either
an infinitely large rectangle, (which is rather hard to represent), or
else it's a single rectangle no larger than the surface. We can always
return the single rectangle for the case when cairo_clip has never
been called, so do you actually need this function?

> +cairo_public cairo_bool_t
> +cairo_extract_clip_rectangles (cairo_t *cr,
> +                               int max_rectangles,
> +                               cairo_clip_rect_t *rectangles_out,
> +                               int *num_rectangles_out);

I think I would prefer to see an API that is more consistent with how
we get at the current path. Something I've always imagined is:

	cairo_public cairo_path_t *
	cairo_copy_clip (cairo_t *cr);

	cairo_public cairo_path_t *
	cairo_copy_clip_flat (cairo_t *cr);

We certainly don't need to implement that now. But it might be natural
to do so down the road if we end up always storing the clip path
(which I suspect we might). So I think we should at least keep that
API in mind while designing the rectangular version.

Which perhaps suggests something like:

	typedef struct _cairo_rectangle {
	    double x, y, width, height;
	} cairo_rectangle_t;

	typedef struct _cairo_rectangles {
	    cairo_rectangle_t *rectangles;
	    int num_rectangles;
	} cairo_rectangles_t;

	cairo_public cairo_rectangles_t *
	cairo_copy_clip_rectangles (cairo_t *cr);

If the copying must be avoided we could instead (or also) do:

	cairo_public cairo_status_t
	cairo_get_clip_rectangles (cairo_t *cr,
				   cairo_rectangles_t *rectangles);

Where if rectangles->num_rectangles is insufficient then an error
status would be returned, rectangles->rectangles would be set to NULL,
and rectangles->num_rectangles would be set to the number needed.

Maybe clip_as_rectangles would actually be better then clip_rectangles
in both of those function names.

Another option would be to allow for something like
cairo_xlib_surface_get_clip_region or so to even eliminate some
conversion, but I think we need the generatl solution in place first.

-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/20060224/e0aac307/attachment.pgp