[cairo] clip-getters [patch]

[Carl Worth]

On Thu, 14 Sep 2006 15:22:03 +1200, Robert O'Callahan wrote:
> Basically this is what cworth suggested ages ago, I just finally got
> around to doing it.

Thanks for doing this. And now I'm finally getting around to reviewing it.

> I've also got tests for the new API. For more bonus points I wrote some
> tests for cairo_fill/stroke_extents which aren't currently being tested.

Thanks very much for the tests. These give me lots of good assurance
to just review the public API and then accept the patch. So I think
this is pretty much all ready to go. Just a couple of comments.


> +        if (status) {
> +            _cairo_traps_fini (&traps);
> +	    return status;
> +	}

There's some inconsistent leading whitespace there. The exact form of
leading whitespace is something I allowed multiple options for in
CODING_STYLE. But mixing them in one block like this is really odd,
(particularly when viewed in an email message like this).

> +/**
> + * cairo_copy_clip_rectangles:
> + *
> + * Returns the current clip region as a list of rectangles in user coordinates.
> + * Never returns %NULL.
> + *
> + * The status in the list may be CAIRO_STATUS_CLIP_NOT_REPRESENTABLE to
> + * indicate that the clip region cannot be represented as a list of
> + * user-space rectangles. The status may have other values to indicate
> + * other errors.
> + **/

The documentation here should mention that the user is always expected
to call cairo_rectangles_destroy.

> +typedef struct _cairo_rectangle_list {
> +} cairo_rectangle_list_t;
...
> +cairo_public void
> +cairo_rectangles_destroy (cairo_rectangle_list_t *rectangle_list);

There's an inconsistency here between "rectangle_list" in the type name
and "rectangles" in the function name. Those should be made to
match. An argument against "rectangle_list" is that this is an array,
not a list. An argument against "rectangles" is that its remarkably
similar to "rectangle".

Meanwhile, I also think it might be worth comparing this API to the
recent dash-getter stuff to see if we shouldn't make something
consistent there.

In the past I've been criticized for being too picky about API before
letting it land in the tree, (while perhaps still experimental). So
I'll do something different this time.

I won't let any dash vs. clip getter inconsistency prevent this patch
from landing in cairo. Instead we can let them live next to each other
and gain some experience with them before we make a final decision one
way or the other.

Then, we can either change one to match the other, or decide that the
use cases are distinct enough to justify the difference.

All I ask is that we make that decision before the next stable
release, and that early adopters of these new APIs be willing to port
to track whatever changes we decide on.

[Snip of very thorough new test cases]

Well done! These are fabulous cases, putting much of what I have
written to shame. Thanks for this good, (and surely very tedious),
work.

-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/20060925/1cfc952d/attachment.pgp