[cairo] Working on degenerate matrix problem, some questions

[Chris Wilson]

On Mon, 2008-03-24 at 17:22 +0000, Daniel Kraft wrote:
> BTW, how should I communicate patches like this 
> to you?  Is it ok like this, should I append the diff directly to the 
> mail text, use something other than the mailing list...?

[I'll expect Carl will be able to answer more completely. Indeed since
this is a FAQ, I'd expected to be able to find some instructions on the
wiki...]
For any non-trivial series of patches, it's preferable to publish a git
branch with your changes. Carl will set up a freedesktop.org account for
you, from which you'll be able to host a repository, if you ask politely
(and follow the instructions at
http://freedesktop.org/wiki/AccountRequests). Given such a repo, for us
to keep current with your work is as simple as:
$ git remote add kraft git://somewhere.org/~dkraft/cairo # once
$ git fetch kraft

> * At some points I am currently ignoring a cairo_status_t return value; 
> that is, in functions already present with a void return type.  How can 
> I set the error flag if an error occurs there without having to return 
> the error code?  What should I do there?  (For instance, the gstate 
> device-to-user transformations, IIRC)

Here you should add a cairo_status_t return and propagate the error code
back to the caller, and there you will be able to set the error flag on
the correct object. The functions are currently void as they only ever
return SUCCESS so the code was simplified by removing the redundant
conditionals.

[snip]
I'm still digesting the bulk of the patch, in particular whether it is
actually a step in the correct direction. ;-) It does seem to avoid the
initial problem of flagging an error for cairo_scale(0,0), but you've
neither updated the test suite (test/invalid-matrix.c, checking that the
error is not set immediately, but only on the invalid use) nor added new
tests to check drawing with cairo_scale(0,0). 

However, I did spot this:
> @@ -1140,14 +1091,20 @@ _cairo_gstate_stroke_extents (cairo_gstate_t	 *gstate,
>  
>      _cairo_traps_init (&traps);
>  
> -    status = _cairo_path_fixed_stroke_to_traps (path,
> -						&gstate->stroke_style,
> -						&gstate->ctm,
> -						&gstate->ctm_inverse,
> -						gstate->tolerance,
> -						&traps);
> -    if (status == CAIRO_STATUS_SUCCESS) {
> -        _cairo_gstate_traps_extents_to_user_rectangle(gstate, &traps, x1, y1, x2, y2);
> +    status = cairo_transform_get_inverse (&gstate->transform, &matrix_inverse);
> +    if (status)
> +    {
> +        /* XXX: Transform */
> +        status = _cairo_path_fixed_stroke_to_traps (path,
> +                                                    &gstate->stroke_style,
> +                                                    &gstate->transform.matrix,
> +                                                    &matrix_inverse,
> +                                                    gstate->tolerance,
> +                                                    &traps);
> +        if (status == CAIRO_STATUS_SUCCESS) {
> +            _cairo_gstate_traps_extents_to_user_rectangle(gstate, &traps,
> +                                                          x1, y1, x2, y2);
> +        }
>      }
>  
>      _cairo_traps_fini (&traps);

which looks wrong, as the bbox determination seems only to occur on the
error path from cairo_transform_get_inverse(). [Hmm, that should be
_cairo_transform_get_inverse(), unless you really are proposing that
cairo exports the cairo_transform_t as public API.]
--
Chris Wilson