[cairo] Reworking text handling

[Owen Taylor]

On Mon, 2005-08-22 at 18:34 -0700, Keith Packard wrote:
> On Mon, 2005-08-22 at 19:41 -0400, Owen Taylor wrote:
> > I've only looked through the cairoint.h changes, but they look good to
> > me... really no comments other than:
> > 
> > +typedef enum _cairo_scaled_glyph_shape {
> > +    cairo_scaled_glyph_shape_none = 0,
> > +    cairo_scaled_glyph_shape_surface = 1,
> > +    cairo_scaled_glyph_shape_path = 2,
> > +    cairo_scaled_glyph_shape_surface_and_path = 3,
> > +} cairo_scaled_glyph_shape_t;
> > 
> > Doesn't seem to conform to Cairo style with upper case enumeration
> > values (and why specify the numeric values?). You could also
> > use a bitfield here of course.
> 
> Oops; I'll fix the case of the value names.
>
> I used explicit values because I wanted to use & operators to
> detect the presense/absense of the individual options. Is there another
> style you'd prefer here?

It's not at all clear from the above that it is a bitfield like this.
Cairo doesn't have any existing bitfields, so there isn't really
precendent. What what I'm used to (GTK+ style)

 typedef enum _cairo_scaled_glyph_shape {
     CAIRO_SCALED_GLYPH_SHAPE_SURFACE = 1 << 0,
     CAIRO_SCALED_GLYPH_SHAPE_PATH    = 1 << 1
 } cairo_scaled_glyph_shape_t;

and use 0 and 
(CAIRO_SCALED_GLYPH_SHAPE_SURFACE | CAIRO_SCALED_GLYPH_SHAPE_PATH)
for the other two values.

> >      cairo_status_t
> >     (*scaled_glyph_init)       (void                        *scaled_font,
> >                                 cairo_scaled_glyph_t        *scaled_glyph,
> >                                 cairo_scaled_glyph_shape_t   shapes);
> > 
> >      cairo_status_t
> >     (*scaled_glyph_ensure_shape)(void                       *scaled_font,
> >                                 cairo_scaled_glyph_t        *scaled_glyph,
> >                                 cairo_scaled_glyph_shape_t   shapes);
> > 
> > I don't really understand the relationship between these two - what
> > does init() do? If we need a separate init(), wouldn't it be cleaner
> > to omit 'shapes' from it and just init() then ensure_shape()?
> 
> Yes, these two are rather muddled together. glyph_init is the same as
> glyph_ensure_shape except that it also computes the glyph metrics. 

I'm not sure if it's the names or the operations that are confusing
to me? Can you call ensure_shape() without having called init()
first? I'd think of init() to be always the first thing you do?

> There are two reasons for this particular style, the first is that
> glyph_ensure_shape was added later, which of course isn't relevant.
> 
> The second reason they are different is that loading the glyph is
> expensive, and you have to load (and hint) the glyph to compute metrics
> and to compute either the path or the surface contents. Combining these
> two steps into one function is a significant performance benefit.

What if you extended your enum to:

 typedef enum _cairo_scaled_glyph_shape {
     CAIRO_SCALED_GLYPH_SHAPE_METRICS = 1 << 0,
     CAIRO_SCALED_GLYPH_SHAPE_SURFACE = 1 << 1,
     CAIRO_SCALED_GLYPH_SHAPE_PATH    = 1 << 2
 } cairo_scaled_glyph_shape_t;

s/shape/info/ and dropped init()? If that encodes the meaning, then
it would be a lot clearer to me.

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/20050822/cdc1bfc0/attachment-0001.pgp