[Mono-devel-list] Re: [Mono-patches] libgdiplus/src brush.h,NONE,1.1 solidbrush.h,NONE,1.1 ChangeLog,1.5,1.6 gdip.h,1.4,1.5 brush.c,1.2,1.3 solidbrush.c,1.2,1.3 pen.c,1.1.1.1,1.2 stringformat.c,1.1,1.2

[Duncan Mak]

Hello Ravindra,

I saw that you've been committing changes to libgdiplus, here are some
questions and comments.

On Fri, 2004-02-13 at 11:01, Ravindra Kumar  wrote:
> Modified Files:
> 	ChangeLog gdip.h brush.c solidbrush.c pen.c stringformat.c 
> Added Files:
> 	brush.h solidbrush.h 
>
> --- NEW FILE: brush.h
> /* Structures */
> typedef struct _Graphics GpGraphics;

It doesn't seem right to me that we're defining Graphics here within
brush.h. Is it possible that we make a graphics.h and include that
instead?

> +2004-02-13  Ravindra <rkumar at novell.com>
> +	* brush.h: Added. Moving brushes to OO-C.
> +	* solidbrush.h: Added. Moving brushes to OO-C.

Just calling OO-C doesn't really mean anything. Could you list the
actual changes you made in the future?

> +	* gdip.h: Moved out some methods to new headers.

Please list the methods that you moved.

> +	* brush.c, solidbrush.c: Changes related to OO-C.
> +	* pen.c: Modifications related to changes in solidbrush.

What are these modifications?

> Index: gdip.h
> +#include "brush.h"

If you include this later in the file, I think you can avoid moving the
code around. By keeping all the general enums and structures before
other header files, we can ensure that the name lookup will happen
correctly.

> @@ -66,30 +68,6 @@
> -typedef enum {
> -    Ok = 0,
> -    GenericError = 1,
> -    InvalidParameter = 2,
> -    OutOfMemory = 3,
> -    ObjectBusy = 4,
> -    InsufficientBuffer = 5,
> -    NotImplemented = 6,
> -    Win32Error = 7,
> -    WrongState = 8,
> -    Aborted = 9,
> -    FileNotFound = 10,
> -    ValueOverflow = 11,
> -    AccessDenied = 12,
> -    UnknownImageFormat = 13,
> -    FontFamilyNotFound = 14,
> -    FontStyleNotFound = 15,
> -    NotTrueTypeFont = 16,
> -    UnsupportedGdiplusVersion = 17,
> -    GdiplusNotInitialized = 18,
> -    PropertyNotFound = 19,
> -    PropertyNotSupported = 20
> -} GpStatus;

Why did you move this to brush.h? The GpStatus enum is the primary
return type used by all the functions in GDI+, I don't think it is
specific to Brush at all. Shouldn't it stay within gdip.h?

> -typedef struct {
> +typedef struct _Graphics {
>  	cairo_t         *ct;
>  	cairo_matrix_t  *copy_of_ctm;
>  	void            *hdc;
>  	int             hdc_busy_count;
>  	void            *image;
>  	int             type; 
> -} GpGraphics;
> +} Graphics;

Why did you have to rename this?

thanks,

Duncan.