Re: [PATCH 1/6] user32: Add get/release routines for accessing cursor frames.

[Alexandre Julliard]

Erich Hoover  writes:
> @@ -135,6 +139,16 @@ static void release_icon_ptr( HICON handle, struct cursoricon_object *ptr )
>      release_user_handle_ptr( ptr );
>  }
>
> +struct static_cursoricon_object *get_icon_data( struct cursoricon_object *obj, int istep )
> +{
> - return &obj->frames[istep];
>
> +}
> -
>
> +void release_icon_data( struct cursoricon_object *obj, int istep, struct static_cursoricon_object *icon_data )
> +{
> - /* placeholder */
>
> +}

I find your data structures confusing. You add a concept of icon_data
separate from icon_ptr, but it doesn't correspond to the data
structures, there's no such thing as an icon_data. There should be a way
of making this clearer.

[PATCH 1/6] user32: Add get/release routines for accessing cursor frames.

[Erich Hoover]

On Fri, Mar 11, 2011 at 8:01 AM, Alexandre Julliard  wrote:
> Erich Hoover  writes:
> > @@ -135,6 +139,16 @@ static void release_icon_ptr( HICON handle, struct cursoricon_object *ptr )
> >      release_user_handle_ptr( ptr );
> >  }
> >
> > +struct static_cursoricon_object *get_icon_data( struct cursoricon_object *obj, int istep )
> > +{
> > - return &obj->frames[istep];
> >
> > +}
> > -
> >
> > +void release_icon_data( struct cursoricon_object *obj, int istep, struct static_cursoricon_object *icon_data )
> > +{
> > - /* placeholder */
> >
> > +}
>
> I find your data structures confusing. You add a concept of icon_data
> separate from icon_ptr, but it doesn't correspond to the data
> structures, there's no such thing as an icon_data. There should be a way
> of making this clearer.

The idea of the get_icon_data() is to return the frame data for a
particular icon step, so for split animated cursors (patch 3) the
routine can appropriately call get_icon_ptr() on the "sub-icon" (and
release_icon_data can call release_icon_ptr).  I considered calling
this get_icon_frame() and having it return the existing
"cursoricon_frame" structure, but DrawIconEx needs to access the
frame-specific width and height - since many animated cursors do not
have a global width and height.  Would you prefer that I call these
get/release_icon_frame() and move the width and height into
"cursoricon_frame" ?

Erich Hoover
[e-mail]

[PATCH 1/6] user32: Add get/release routines for accessing cursor frames.

[Alexandre Julliard]

Erich Hoover  writes:
> The idea of the get_icon_data() is to return the frame data for a
> particular icon step, so for split animated cursors (patch 3) the
> routine can appropriately call get_icon_ptr() on the "sub-icon" (and
> release_icon_data can call release_icon_ptr).  I considered calling
> this get_icon_frame() and having it return the existing
> "cursoricon_frame" structure, but DrawIconEx needs to access the
> frame-specific width and height - since many animated cursors do not
> have a global width and height.  Would you prefer that I call these
> get/release_icon_frame() and move the width and height into
> "cursoricon_frame" ?

I'd have to see what the resulting code look like, but it does sound
like it would be cleaner.