Re: [PATCH 1/2] user32: SetSysColors updates brushes from GetSysColorBrush and add test

[Nikolay Sivov]

On 14.01.2017 22:33, Fabian Maurer wrote:
> Signed-off-by: Fabian Maurer
>
> dlls/gdi32/gdi32.spec         |  2 ++
>  dlls/gdi32/gdiobj.c           | 12 ++++++++++++
>  dlls/user32/sysparams.c       | 23 +++++++++++++++++------
>  dlls/user32/tests/sysparams.c | 16 ++++++++++++++++
>  include/wine/gdi_driver.h     |  2 ++
>  5 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/gdi32/gdi32.spec b/dlls/gdi32/gdi32.spec
> index 7792731b9e..f9c3fb9293 100644
> --- a/dlls/gdi32/gdi32.spec
> +++ b/dlls/gdi32/gdi32.spec
> @@ -518,6 +518,8 @@
>  # GDI objects
>  @ cdecl __wine_make_gdi_object_system(long long)
>  @ cdecl __wine_set_visible_region(long long ptr ptr ptr)
> +@ cdecl __wine_GDI_ReleaseObj(ptr)
> +@ cdecl __wine_GDI_GetObjPtr(ptr long)
>
> # Graphics drivers
>  @ cdecl __wine_set_display_driver(long)
> diff --git a/dlls/gdi32/gdiobj.c b/dlls/gdi32/gdiobj.c
> index 46b2241945..aa2ced1025 100644
> --- a/dlls/gdi32/gdiobj.c
> +++ b/dlls/gdi32/gdiobj.c
> @@ -880,6 +880,18 @@ void GDI_ReleaseObj( HGDIOBJ handle )
>      LeaveCriticalSection( &gdi_section );
>  }
>
> +/* Exports for user32, we need to be able to manipulate GDI objects where the official API doesn't allow us */
> -
>
> +void* CDECL __wine_GDI_GetObjPtr( HGDIOBJ handle, WORD type )
> +{
> - return GDI_GetObjPtr( handle, type );
>
> +}
> -
>
> +void CDECL __wine_GDI_ReleaseObj( HGDIOBJ handle )
> +{
> - GDI_ReleaseObj( handle );
>
> +}
> -

This is too much. If you only need to update object property while
keeping same handle, you only need one additional call like
__wine_set_gdi_object_color(), that should work for both brushes and
pens, checking if it's a system object too, as an additional precaution.
> +static void set_syscolors_gdi( INT index, COLORREF color )
> +{
> - HBRUSH brush = system_colors[index].brush;
> - LOGBRUSH *brush_info;
> -
> - if(brush)
> - {
> -
>    /* Since LOGBRUSH is the first member in BRUSHOBJ we just do a cast here */
> -
>    brush_info = __wine_GDI_GetObjPtr( brush, OBJ_BRUSH );
> -
>    if(brush_info->lbColor != color)
> -
>        brush_info->lbColor = color;
> -
>    __wine_GDI_ReleaseObj( brush );
> - }
>
> +}

This looks very dirty.

[PATCH 1/2] user32: SetSysColors updates brushes from GetSysColorBrush and add test

[Fabian Maurer]

On Saturday, January 14, 2017 11:14:31 PM CET Nikolay Sivov wrote:
> This is too much. If you only need to update object property while
> keeping same handle, you only need one additional call like
> __wine_set_gdi_object_color(), that should work for both brushes and
> pens, checking if it's a system object too, as an additional precaution.

Thanks, that makes sense.
> This looks very dirty.

Since it's the first member it's fine as long as the structure doesn't change.
I wanted to avoid pulling the struct into the header too, but maybe I can 
rework it.

Fabian