[Gdiplus try3 02/16] Implement GdipCreateRegion

[Huw Davies]

Hi Adam,

Some comments inlined below:

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.

> +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.

> +    } 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 

> 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.

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

Huw.
-- 
Huw Davies
huw at codeweavers.com