[cairo] cairo pattern/glitz patches

[David Reveman]

On Thu, 2005-02-24 at 23:04 -0500, Kristian Høgsberg wrote: 
> David Reveman wrote:
> ...
> > Having the pattern as one big union cleaned up things a lot and I think
> > it's a more appropriate way to store patterns internally. I'd also like
> > to change so that PATTERN_LINEAR and PATTERN_RADIAL are gradient types
> > and not pattern types. 
> 
> Do you want gradients as a user visible object distinct from pattern? 
> That is, cairo_gradient_t?

Possibly, I'm not completely happy with the current public gradient
interface. I don't like the way color stops are specified. We should
probably give this interface a second thought.

> 
> > with the pattern as a union we can write code similar to this: 
> > 
> > cairo_image_surface_t *
> > get_radial (cairo_radial_gradient_t *radial, cairo_stops_t *stops)
> > {
> >     double a, b, c, d, tx, ty;
> > 
> >     cairo_matrix_get_affine (&radial->matrix, &a, &b, &c, &d, &tx, &ty);
> > 
> >     switch (radial->extend) {
> >     ...
> >     ...
> > }
> 
> These two cases are probably not the best examples, since matrix and 
> extend would be in the common part of the pattern struct:
> 
> 	struct cairo_pattern_t {
> 		cairo_matrix_t matrix;
> 		cairo_pattern_extend_t extend;
> 		...
> 		union {
> 			...
> 		} u;
> 	};
> 
> and you could access them as pattern->matrix and pattern->extend.  In 
> fact reading your patch again, there's a lot of these changes
> 
> 	-    pattern.matrix = user_to_image;
> 	-    pattern.filter = surface->filter;
> 	+    pattern.any.matrix = user_to_image;
> 	+    pattern.any.filter = surface->filter;
> 
> which sort of defeats the purpose of the change.  Or maybe I'm reading 
> it wrong and the idea to get a more "typesafe" implementation where you 
> pass more precise pointer types (cairo_radial_gradient_pattern_t instead 
> of cairo_pattern_t) around, rather than to save typing?  

Both type safety and cleaner code. By cleaner code, I don't necessarily
mean that I've reduced the number of lines or the number of characters
but more that parameters passed to functions are more appropriate and
the relation between functions and the pattern structure is nicer.

> If that's the 
> case, shouldn't we use a construction more like cairo_surface_t?  For 
> example:
> 
> 	struct _cairo_pattern {
> 		cairo_pattern_type_t type;
> 		unsigned int        ref_count;
> 		double              alpha;
> 		cairo_extend_t      extend;
> 		cairo_filter_t      filter;
> 		cairo_matrix_t      matrix;
> 	};
> 
> 	struct _cairo_solid_pattern {
> 		cairo_pattern_t	base;
> 		double red, green, blue;
> 	};
> 
> which is much like your patch, except without the preprocessor hack and 
> without changing the public visible struct _cairo_pattern to a union.
> 
> ...

This means that we can't allocate a general pattern on the stack. We do
this a lot (_cairo_pattern_copy) and I think we like to keep doing
this. 

> 
> >>How about passing the pattern type to _cairo_pattern_init()?
> > 
> > As of right now, we always want to init patterns with some type specific
> > parameters. e.g. _cairo_pattern_init_solid (&pattern, red, green, blue).
> > So passing a type to _cairo_pattern_init doesn't seem like a good idea.
> 
> I guess I wasn't too clear there.  Instead of
> 
> 	pattern->type = CAIRO_PATTERN_SOLID;
> 	_cairo_pattern_init (pattern);
> 	pattern->solid.red   = red;
> 	pattern->solid.green = green;
> 	pattern->solid.blue  = blue;
> 
> I'm suggesting
> 
> 	_cairo_pattern_init (pattern, CAIRO_PATTERN_SOLID);
> 	pattern->solid.red   = red;
> 	pattern->solid.green = green;
> 	pattern->solid.blue  = blue;
> 
> It just seems logical to me that _cairo_pattern_init() should initialize 
> all the common fields, of which 'type' is one.  Anyway, I'm splitting 
> hairs now.

OK, sorry, I though you meant for use outside of cairo_pattern.c. Just
for _cairo_pattern_init within cairo_pattern.c, yes, that seems like a
good idea.  

-David