[PATCH] user32/cursoricon: Fix CreateIcon() bitmap mask handling.

Huw Davies huw at codeweavers.com
Tue Apr 3 08:21:28 CDT 2018


On Tue, Apr 03, 2018 at 08:41:45PM +0800, Zhiyi Zhang wrote:
> On Tue 4 3 19:58, Huw Davies wrote:
> >On Fri, Mar 30, 2018 at 07:32:31PM +0800, Zhiyi Zhang wrote:
> >>
> >>Signed-off-by: Zhiyi Zhang <zzhang at codeweavers.com>
> >>---
> >>  dlls/user32/cursoricon.c       |  80 +++++++++++----------
> >>  dlls/user32/tests/cursoricon.c | 160
> >>++++++++++++++++++++++++++++++++++++-----
> >>  2 files changed, 187 insertions(+), 53 deletions(-)
> >>
> >>
> >
> >There's a lot going on in this patch and the vague commit message doesn't
> >help.  I'd suggest a patch that fixes CreateIconIndirect() first and
> >a second patch to fix CreateIcon(), then write more description commit
> >messages.
> >
> Some of the logic in CreateIconIndirect() should be in CreateIcon(), so
> there is no easy way of fixing CreateIconIndirect() alone without breaking
> CreateIcon() and the tests.

I haven't looked hard enough, but would it be possible to do it the
other way around: add the logic to CreateIcon() first and then remove
the stuff in CreateIconIndirect() in a 2nd patch?
 
> More description is a good idea.

Looking forward to it ;-)

> >>diff --git a/dlls/user32/cursoricon.c b/dlls/user32/cursoricon.c
> >>index 8e272f09ce..69c7a937ff 100644
> >>--- a/dlls/user32/cursoricon.c
> >>+++ b/dlls/user32/cursoricon.c
> >>@@ -92,6 +92,9 @@ struct animated_cursoricon_object
> >>      HICON                    frames[1];  /* list of animated cursor frames */
> >>  };
> >>+static void stretch_blt_icon(HDC hdc_dst, int dst_x, int dst_y, int dst_width, int dst_height,
> >>+                             HBITMAP src, int width, int height);
> >>+

Another thing, if you moved CreateIcon() to below stretch_blt_icon()
(in fact below CreateIconIndirect() would make most sense), then you
don't need this.  I'd suggest making the moving patch a NOP, that doesn't
do anything else.

> >>  static HBITMAP create_color_bitmap( int width, int height )
> >>  {
> >>      HDC hdc = get_display_dc();
> >>@@ -1561,20 +1564,38 @@ HICON WINAPI CreateIcon(
> >>  {
> >>      ICONINFO iinfo;
> >>      HICON hIcon;
> >>+    HBITMAP hbmMask_upperhalf;
> >>+    HBITMAP hbmMask_lowerhalf;
> >>+    HDC hdc;
> >>      TRACE_(icon)("%dx%d, planes %d, bpp %d, xor %p, and %p\n",
> >>                   nWidth, nHeight, bPlanes, bBitsPixel, lpXORbits, lpANDbits);
> >>      iinfo.fIcon = TRUE;
> >>-    iinfo.xHotspot = nWidth / 2;
> >>-    iinfo.yHotspot = nHeight / 2;
> >
> >So this leaves x/yHotspot uninitialized...
> No, because they get initialized later in CreateIconIndirect(), so I think
> there is no need to initialize them here.

Well CreateIconIndirect() ignores these params, but we should still probably
initialize them, so set them to zero.  That can be a separate patch.

> >>diff --git a/dlls/user32/tests/cursoricon.c b/dlls/user32/tests/cursoricon.c
> >>index 5099c08d70..e5faa0050e 100644
> >>--- a/dlls/user32/tests/cursoricon.c
> >>+++ b/dlls/user32/tests/cursoricon.c
> >>@@ -803,23 +803,27 @@ static void test_CreateIcon(void)
> >>      void *bits;
> >>      UINT display_bpp;
> >>      int i;
> >>+    BOOL is_color;
> >>      hdc = GetDC(0);
> >>      display_bpp = GetDeviceCaps(hdc, BITSPIXEL);
> >>+    is_color = display_bpp > 1;
> >
> >Did you really try this on a 1 bpp display?
> No, I don't have a 1 bpp display to test it visually. However, I covered 1
> bpp bitmap in conformance tests, tested the result image width, height, bpp.

My point was that 1 bpp displays don't exist.  So you can replace this
variable with TRUE throughout (and then simplify any relevant code).

Huw.



More information about the wine-devel mailing list