[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