[cairo] API Shakeup: cairo_begin_group, cairo_end_group, cairo_get_group

[Owen Taylor]

On Thu, 2005-07-07 at 08:54 -0700, Carl Worth wrote:
> On Thu, 23 Jun 2005 07:32:10 -0700, Carl Worth wrote:
> > Here, the format is intended to specify one of three different modes
> > for what the pattern will store:
> > 
> > 	Color only
> > 	Alpha only
> > 	Color and alpha
> > 
> > The existing cairo_format_t is not the right way to specify this
> > information as it is overspecific, (describing bits per channel and
> > channel order as well).
> > 
> > There's an outstanding TODO item regarding splitting uses of
> > cairo_format_t into two separate things specifying either bit-layout
> > or surface capability.
> 
> Here's a first-cut patch at this.
> 
> I used a name of cairo_content_t for the new "surface content
> description" as opposed to cairo_format_t which describes the format
> of the bytes of the surface.
> 
> I only came up with two values (CAIRO_CONTENT_COLOR and
> CAIRO_CONTENT_ALPHA) and I implemented these so that they are passed
> as a bitfield, (ie. bitwise ORed together).

Two things here.

 1. CAIRO_CONTENT_COLOR | CAIRO_CONTENT_ALPHA is quite long-winded
    for one of the most common cases.

 2. Bitfields don't language-bind very well.. even using the C API
    from C++ will produce compiler warnings without the addition
    of casts.

    So, introducing one into Cairo for just this case perhaps isn't
    a good idea.

I'd probably just go with a 3-element enum. (If you wanted to preserve
the ability to have bitfields in the future, you could make the
three values 0x1, 0x2, 0x3...)

> As mentioned before, this change does mean that it's no longer
> possible to create a similar surface with an A1 format. Is that a big
> deal? My feeling is that it's not. And if I'm proven wrong in the
> future I think adding a new function to cover this case would be fine.

Sounds fine to me.

> The remainder of the implementation changes are as minor as
> possible. The backend create_similar function now takes a
> cairo_content_t, but the implementation almost universally converts
> directly from that to a cairo_format_t in a trivial way. In some
> cases, the behavior here might want to change, but that will be easy
> to do now since the actual format being used is no longer part of the
> public interface.

Patch comments:

===
-    return _cairo_surface_create_similar_solid (other, format,
+    /* XXX: If we switch to never returning a NULL surface, (like we
+     * do for cairo_t and cairo_pattern_t), then we'd have the
+     * advantage here of actually being able to indicate to the caller
+     * what the error is here, rather than just returning a vague
+     * NULL. */
+
+    /* Check that the content value is valid. */
+    if (content == 0 ||        content & ~(CAIRO_CONTENT_COLOR |
CAIRO_CONTENT_ALPHA))
+       return NULL;
===

I don't think it's ever interesting for a program to check "if I passed
a junk cairo_content_t, then I should...". What is needed here (and
elsewhere) is simply debug spew.

===
 static cairo_surface_t *
 _cairo_win32_surface_create_similar (void          *abstract_src,
-                                    cairo_format_t  format,
+                                    cairo_content_t content,
                                     int             width,
                                     int             height)
===

The 'format' argument is used here... your patch is going to break win32
compilation.

====
+ * the API it should consist of the bitwise OR of one or more of the
+ * following values:
+ *
+ * @CAIRO_CONTENT_COLOR: The surface will hold color content.
+ *
+ * @CAIRO_CO
===

I don't think gtk-doc likes this sort of intermingling. And the output
certainly won't preserve the intermingling. The enumeration constants
should be the first thing in the doc comment, just like function 
parameters.

Other than that, the patch looks good.

Regards,
						Owen

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050707/5eb8c72c/attachment.pgp