[Gdiplus try3 02/16] Implement GdipCreateRegion

[Adam Petaccia]

On Tue, 2008-07-22 at 11:11 +0100, Huw Davies wrote:
> Hi Adam,
> 
> Some comments inlined below:
Thanks for the review. 

> 
> On Tue, Jul 22, 2008 at 01:22:56AM -0400, Adam Petaccia wrote:
> > diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
> > index e7ca874..9c0246b 100644
> > --- a/dlls/gdiplus/gdiplus_private.h
> > +++ b/dlls/gdiplus/gdiplus_private.h
> > @@ -196,4 +198,40 @@ struct GpFontFamily{
> >      WCHAR FamilyName[LF_FACESIZE];
> >  };
> >  
> > +typedef struct RegionElement
> > +{
> > +    DWORD type; /* Rectangle, Path, SpecialRectangle, or CombineMode */
> > +    union
> > +    {
> > +        GpRectF rect;
> > +        struct
> > +        {
> > +            GpPath* path;
> > +            struct
> > +            {
> > +                DWORD size;
> > +                DWORD magic;
> > +                DWORD pointCount;
> > +                DWORD storageFlags;
> > +            } pathHeader;
> > +        } pathData;
> > +        struct
> > +        {
> > +            struct RegionElement *left;  /* the original region */
> > +            struct RegionElement *right; /* what *left was combined with */
> > +        } combine;
> > +    } elementData;
> > +} RegionElement;
> 
> It would be better to avoid the mixed upper/lower case names, since
> this makes it look like they're win32 api structures.  Something like
> typedef struct region_element region_element is nicer.
> Same goes for the element names like pathHeader etc.

Will fix. Question: would it be better to declare these structures
outside, or are inline declarations acceptable? 

> 
> > +struct GpRegion{
> > +    struct
> > +    {
> > +        DWORD size;
> > +        DWORD checksum;
> > +        DWORD magic;
> > +        DWORD numOps;
> 
> A better name for numOps would be num_child_nodes.  This is the total
> number of children under the root node (and is equal to twice the
> number of ops).  I should update my comment at the beginning of
> region.c to reflect this.

Will do. How about just num_children?

> 
> > +    } header;
> > +    RegionElement *node;
> 
> How about actually embedding the structure here rather than a pointer to
> it?  Since you'll always have a root node there's no need to allocate
> it explicitly.  And let's call it 'root' not 'node'.
> Something like region_element root 

The CombineRegion* code takes advantage over the fact that they are
pointers and can be shuffled around, rather than
shuffling/cloning/memcpy'ing potentially large amounts of data around.

Also, because of the recursive nature of CombineMode, I found it much
easier to deal with nodes separately (and recursively when necessary)
than the whole Region at a time.

> 
> > diff --git a/include/gdiplusenums.h b/include/gdiplusenums.h
> > index 53401bd..f3110c9 100644
> > --- a/include/gdiplusenums.h
> > +++ b/include/gdiplusenums.h
> > @@ -308,6 +308,15 @@ enum CombineMode
> >      CombineModeComplement
> >  };
> >  
> > +enum RegionType
> > +{
> > +    RegionDataRect          = 0x10000000,
> > +    RegionDataPath          = 0x10000001,
> > +    RegionDataEmptyRect     = 0x10000002,
> > +    RegionDataInfiniteRect  = 0x10000003,
> > +};
> > +
> > +
> >  enum FlushIntention
> >  {
> >      FlushIntentionFlush = 0,
> > @@ -353,6 +362,7 @@ typedef enum HotkeyPrefix HotkeyPrefix;
> >  typedef enum PenAlignment GpPenAlignment;
> >  typedef enum ImageCodecFlags ImageCodecFlags;
> >  typedef enum CombineMode CombineMode;
> > +typedef enum RegionType RegionType;
> >  typedef enum FlushIntention FlushIntention;
> >  typedef enum CoordinateSpace CoordinateSpace;
> >  
> 
> Are you sure RegionType is in the win32 api?  I can't find this is my
> gdiplusenums.h.  If it isn't then it should be put in
> gdiplus_private.h (or even just in region.c if nothing else uses it)
> and renamed to enum region_type.

I'll move it to region.c.

> 
> Again, I think you'd be better off submitting a couple of patches at a
> time.

Sorry, its kind of painful when the work is mostly done, but the problem
is getting it pushed. I'm slightly behind where I want to be on my GSoC
schedule, and I was trying to catch up. I'll slow down.

> 
> Huw.