[PATCH] user32/cursoricon: Fix CreateIcon() bitmap mask handling.
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/cursoricon.c | 80 +++++++++++---------- dlls/user32/tests/cursoricon.c | 160 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 187 insertions(+), 53 deletions(-)
On Fri, Mar 30, 2018 at 07:32:31PM +0800, Zhiyi Zhang wrote:
Signed-off-by: Zhiyi Zhang <zzhang(a)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.
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); + 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...
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? Huw.
Thanks for reviewing. 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(a)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. More description is a good idea.
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); + 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.
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. I also tested the patch against a sample application downloaded online which creates icon with masks with CreateIconIndirect(). And after this patch, the application is visually fine. I could add some visual tests in conformance test if necessary.
Huw.
Thanks, Zhiyi
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(a)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.
participants (2)
-
Huw Davies -
Zhiyi Zhang