[Gdiplus try3 02/16] Implement GdipCreateRegion
Adam Petaccia
adam at tpetaccia.com
Tue Jul 22 11:13:41 CDT 2008
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.
More information about the wine-devel
mailing list