[cairo] Re: new clipping change patch

[Owen Taylor]

On Mon, 2005-05-23 at 23:28 -0700, Keith Packard wrote:
> After our IRC discussion today, I went and rewrote the clipping code
> again to pull most of the clipping work into the gstate where it belongs.
> 
> The surface now just reflects state down to the backend, the only thing
> retained is the serial number for the clipping that was set.
> 
> Extents for the clip are kept up in the gstate; the surface will only
> return the extents for the entire object.
> 
> My email is dead until tomorrow sometime; bad things happened to my 
> home box this week and my ISP disabled my line as a result...
> 
> Patch available at http:/freedesktop.org/~keithp/cairo-serialclip-2.diff
> 
> I should be back on-line sometime tomorrow; failing that, I'll rework
> email so I can get at it somehow.

Hopefully this mail will get through to you one way or the other.

I like this version of the patch a lot more than the last one. 

Various comments:

 * I'm not sure the name change cairo_surface_set_clip_region() =>
   cairo_surface_clip_region() makes sense. It's still a setter...
   there are no plans to make clip_region() intersect with the
   current clip region.

 * The code goes through various gymnastics to make sure that when
   restoring to a previous clip serial, the same serial is used.
   I don't see why this is useful. I'd expect 
   _cairo_gstate_set_clip() to look like:

    if (_cairo_surface_get_clip_serial (surface) == gstate->clip.serial)
      return CAIRO_STATUS_SUCCESS;

    /* reestablish the clip */

    gstate->clip.serial = _cairo_surface_get_clip_serial (surface);

   And leave all updating of the serial internal to the surface.

   The "no clip gets 0 serial" optimization can still be done this
   way.

 * The duplication of rectangle intersection code one my time in
   _cairo_gstate_get_clip_extents() is painful. That really should
   be split out somewhere. (boolean return: FALSE on empty intersection)

   Or alternatively, for get_clip_extents(), do the intersection
   on regions not rectangles.

 * cairo-gstate.c:_get_mask_extents() immediately intersects the 
   result of get_clip_extents() with the clip region (as a region).
   Which is duplication one way or the other.
 
 * I don't understand the logic in _cairo_gstate_set_target_surface()
   calling _cairo_surface_get_next_clip_serial().

 * _cairo_xlib_surface_ensure_picture (surface, &surface->&src_picture);

  Maybe just split that into ensure_src_picture() and 
  ensure_dst_picture()?

 * In _cairo_xlib_surface_set_attributes(), the call to ensure_picture()
   is inserted between a call that sets status and the subsequent
   check of status. It should be inside the if (CAIRO_OK (status))

-------------- 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/20050524/d3f6ef6b/attachment.pgp