Re: [Gdiplus try3 02/16] Implement GdipCreateRegion
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(a)codeweavers.com
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.
Adam Petaccia wrote:
On Tue, 2008-07-22 at 11:11 +0100, Huw Davies wrote:
+} 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?
inline is fine.
+ 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?
Works for me.
+ } 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.
It should only be a question of how you start your recursion. Obviously the CombineRegion stuff will have a alloc and memcpy the old root node to 'left', but then you don't need to alloc the new root node.
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.
Great.
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.
I think you'll actually find it quicker in the long run to have smaller patch dumps, that way you'll spend less time re-doing a whole load of patches many times. Huw.
participants (2)
-
Adam Petaccia -
Huw Davies