[cairo] clip-getters patch

[Carl Worth]

On Wed, 24 May 2006 16:49:27 +1200, Robert O'Callahan wrote:
>
> Here's the revised patch for consideration. It returns the rectangle(s)
> in user coordinates and bails if that isn't possible.

Thanks for the updated patch. Here are some superficial comments,
(more substantive review will follow).

> +typedef struct _cairo_rectangle {
> +  double x, y, width, height;
> +} cairo_rectangle_t;
...
> +typedef struct _cairo_rectangles {
> +  cairo_rectangle_t* rectangles;
> +  /** The number of rectangles for which we have allocated space in
> +      'rectangles' */
> +  int max_rectangles;
> +  /** The number of valid rectangles in the 'rectangles' array,
> +      or the number of rectangles for which space is needed, if
> +      max_rectangles is not big enough. */
> +  int num_rectangles;
> +} cairo_rectangles_t;

There is insufficient indentation in the above.

> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Would you substitute the standard cairo licensing blurb please? There
should be no impact on legal details, but I'd like to keep the cairo
sources consistent, (and the cairo blurb does point to locally
distributed files for the complete text of all licenses).

Oh, but this is in the test directory anyway. Most of the tests are
under an alternate (MIT) license. The idea there was to allow people
to feel more free to look for sample code in there. Having a separate
module for demo code would much much better of course, (the tests do
some strange things that people shouldn't copy). So, whatever.

> +main (void)
> +{
> +    cairo_surface_t *surface;
> +    cairo_t *cr;
> +    cairo_rectangle_t rect_array[10];
> +    cairo_rectangles_t rects = { rect_array, 0, 0 };
> +
> +    surface = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, 100, 100);
> +    cr = cairo_create (surface);

This should be rewritten to use the standard test suite structure,
which has a form like this:

	#define WIDTH 100
	#define HEIGHT 100

	cairo_test_t test = {
	    "get-clip",
	    "Test cairo_has_clip and cairo_get_clip_as_rectangles",
	    WIDTH, HEIGHT
	};

	static cairo_test_status_t
	draw (cairo_t *cr, int width, int height)
	{
	    /* ... */
	}

	int
	main (void)
	{
	    return cairo_test (&test, draw);
	}

Then the code you currently have in main would instead go into the
draw function with the following changes:

1. Drop the cairo_image_surface_create and cairo_create stuff

2. Replace assert statements with code that returns
   CAIRO_TEST_STATUS_FAILURE, (should really also call cairo_test_log
   to put details about the failure into the log file).

Look at test/nil-surface.c for a rather minimal example test that works
in a similar way to your get-clip test.

The test structure has the following advantages:

1. Tests all backends rather than just the image backend

2. Creates log files that can be parsed by report-generating tools,
   (the only one we have so far is make-html.pl).

3. Avoids asserts being defeated by -DNDEBUG

The test structure also does output image comparison. Tests that don't
generate any output at all can use a width,height of 0,0 to turn those
checks off completely.

Since your test is already doing its own checking, you might use that
0,0 approach here. Now, having a surface of size 0 might have bad
interactions with the clipping code you're testing, so you might do
something like this inside your draw function:

	cairo_t *cr2;
	cairo_surface_t *surface;

	surface = cairo_surface_create_similar (cairo_get_target (cr),
						CAIRO_CONTENT_COLOR,
						100, 100);
	cr2 = cairo_create (surface);

and then run all your tests against cr2.

You'll notice that the existing nil-surface test does something
similar.

-Carl

PS. If someone were to edit the above and update the test/README file
to explain how to create a new test, that could be useful.
-------------- 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/20060524/cab44cc3/attachment.pgp