[PATCH v2 0/4] MR8330: comctl32/listview: Miscellaneous fixes for uninitialized and unnecessarily initialized variables.
-- v2: comctl32/listview: Properly handle item state value for LVS_OWNERDATA controls. comctl32/tests: Add item state value tests for LVS_OWNERDATA controls. comctl32/listview: Don't touch iImage value for subitems if LVS_EX_SUBITEMIMAGES is not set. comctl32/tests: Add tests for iImage value for listview subitems. https://gitlab.winehq.org/wine/wine/-/merge_requests/8330
From: Connor McAdams <cmcadams(a)codeweavers.com> Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com> --- dlls/comctl32/tests/listview.c | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index b767106e97a..73eb127fc35 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -1067,6 +1067,7 @@ static void test_lvm_subitemhittest_(HWND hwnd, INT x, INT y, INT item, INT subi #define test_lvm_subitemhittest(a,b,c,d,e,f,g,h,i) test_lvm_subitemhittest_(a,b,c,d,e,f,g,h,i,__LINE__) +static void insert_column(HWND hwnd, int idx); static void test_images(void) { HWND hwnd; @@ -1160,14 +1161,53 @@ static void test_images(void) flush_sequences(sequences, NUM_MSG_SEQUENCES); + /* + * If LVS_EX_SUBITEMIMAGES is not set, iImage field value is untouched + * for subitems. + */ memset(&item, 0, sizeof(item)); item.mask = LVIF_IMAGE; item.iSubItem = 1; + item.iImage = 500; r = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); ok(r, "Failed to get item.\n"); + todo_wine ok(item.iImage == 500, "Unexpected iImage value %d.\n", item.iImage); ok_sequence(sequences, PARENT_SEQ_INDEX, empty_seq, "get image dispinfo 2", FALSE); + r = SendMessageA(hwnd, LVM_SETEXTENDEDLISTVIEWSTYLE, 0, LVS_EX_SUBITEMIMAGES); + ok(!r, "Unexpected return value %d.\n", r); + + /* + * LVS_EX_SUBITEMIMAGES is set, iImage field value is set for subitems. + */ + memset(&item, 0, sizeof(item)); + item.mask = LVIF_IMAGE; + item.iSubItem = 1; + item.iImage = 500; + r = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); + ok(r, "Failed to get item.\n"); + ok(item.iImage == I_IMAGECALLBACK, "Unexpected iImage value %d.\n", item.iImage); + + insert_column(hwnd, 0); + insert_column(hwnd, 1); + + /* Set subitem iImage value. */ + memset(&item, 0, sizeof(item)); + item.mask = LVIF_IMAGE; + item.iSubItem = 1; + item.iImage = 500; + r = SendMessageA(hwnd, LVM_SETITEMA, 0, (LPARAM)&item); + ok(r, "Failed to set item.\n"); + + item.mask = LVIF_IMAGE; + item.iSubItem = 1; + item.iImage = 0; + r = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); + ok(r, "Failed to get item.\n"); + ok(item.iImage == 500, "Unexpected iImage value %d.\n", item.iImage); + + flush_sequences(sequences, NUM_MSG_SEQUENCES); DestroyWindow(hwnd); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8330
From: Connor McAdams <cmcadams(a)codeweavers.com> Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com> --- dlls/comctl32/listview.c | 2 -- dlls/comctl32/tests/listview.c | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 31f8a933e56..57ad887364f 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -6867,8 +6867,6 @@ static BOOL LISTVIEW_GetItemT(const LISTVIEW_INFO *infoPtr, LPLVITEMW lpLVItem, { if (!lpLVItem->iSubItem || (infoPtr->dwLvExStyle & LVS_EX_SUBITEMIMAGES)) lpLVItem->iImage = pItemHdr->iImage; - else - lpLVItem->iImage = 0; } /* The pszText field */ diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 73eb127fc35..54ac79b5f86 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -1171,7 +1171,7 @@ static void test_images(void) item.iImage = 500; r = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); ok(r, "Failed to get item.\n"); - todo_wine ok(item.iImage == 500, "Unexpected iImage value %d.\n", item.iImage); + ok(item.iImage == 500, "Unexpected iImage value %d.\n", item.iImage); ok_sequence(sequences, PARENT_SEQ_INDEX, empty_seq, "get image dispinfo 2", FALSE); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8330
From: Connor McAdams <cmcadams(a)codeweavers.com> Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com> --- dlls/comctl32/tests/listview.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 54ac79b5f86..2abe27f354e 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3520,6 +3520,35 @@ static void test_ownerdata(void) expect(1, res); res = SendMessageA(hwnd, LVM_GETITEMCOUNT, 0, 0); expect(1, res); + + /* LVIF_STATE is not set, state field untouched. */ + memset(&item, 0, sizeof(item)); + item.mask = LVIF_TEXT; + item.iItem = 0; + item.state = 0xffff; + res = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); + ok(res, "Failed to get item.\n"); + todo_wine ok(item.state == 0xffff, "Unexpected item state %#x.\n", item.state); + + /* LVIF_STATE is set with no statemask, state field zeroed. */ + memset(&item, 0, sizeof(item)); + item.mask = LVIF_STATE; + item.iItem = 0; + item.state = 0xffff; + res = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); + ok(res, "Failed to get item.\n"); + todo_wine ok(!item.state, "Unexpected item state %#x.\n", item.state); + + res = SendMessageA(hwnd, LVM_GETITEMSTATE, 0, 0xff); + todo_wine ok(res == LVIS_SELECTED, "Unexpected item state %#lx.\n", res); + + /* Set the callback mask for LVIS_SELECTED. */ + res = SendMessageA(hwnd, LVM_SETCALLBACKMASK, LVIS_SELECTED, 0); + expect(TRUE, res); + + res = SendMessageA(hwnd, LVM_GETITEMSTATE, 0, 0xff); + ok(!res, "Unexpected item state %#lx.\n", res); + DestroyWindow(hwnd); /* LVM_SETITEM and LVM_SETITEMTEXT is unsupported on LVS_OWNERDATA */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8330
From: Connor McAdams <cmcadams(a)codeweavers.com> Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com> --- dlls/comctl32/listview.c | 3 ++- dlls/comctl32/tests/listview.c | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 57ad887364f..840e2cf4897 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -6711,7 +6711,8 @@ static BOOL LISTVIEW_GetItemT(const LISTVIEW_INFO *infoPtr, LPLVITEMW lpLVItem, /* if the app stores all the data, handle it separately */ if (infoPtr->dwStyle & LVS_OWNERDATA) { - dispInfo.item.state = 0; + if ((lpLVItem->mask & LVIF_STATE)) lpLVItem->state = 0; + dispInfo.item.state = lpLVItem->state; /* apparently, we should not callback for lParam in LVS_OWNERDATA */ if ((lpLVItem->mask & ~(LVIF_STATE | LVIF_PARAM)) || diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 2abe27f354e..85cecd11159 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3528,7 +3528,7 @@ static void test_ownerdata(void) item.state = 0xffff; res = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); ok(res, "Failed to get item.\n"); - todo_wine ok(item.state == 0xffff, "Unexpected item state %#x.\n", item.state); + ok(item.state == 0xffff, "Unexpected item state %#x.\n", item.state); /* LVIF_STATE is set with no statemask, state field zeroed. */ memset(&item, 0, sizeof(item)); @@ -3537,10 +3537,10 @@ static void test_ownerdata(void) item.state = 0xffff; res = SendMessageA(hwnd, LVM_GETITEMA, 0, (LPARAM)&item); ok(res, "Failed to get item.\n"); - todo_wine ok(!item.state, "Unexpected item state %#x.\n", item.state); + ok(!item.state, "Unexpected item state %#x.\n", item.state); res = SendMessageA(hwnd, LVM_GETITEMSTATE, 0, 0xff); - todo_wine ok(res == LVIS_SELECTED, "Unexpected item state %#lx.\n", res); + ok(res == LVIS_SELECTED, "Unexpected item state %#lx.\n", res); /* Set the callback mask for LVIS_SELECTED. */ res = SendMessageA(hwnd, LVM_SETCALLBACKMASK, LVIS_SELECTED, 0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8330
On Tue Jun 17 21:07:38 2025 +0000, Nikolay Sivov wrote:
The test looks fine, but I think using some value that's not I_IMAGECALLBACK would be better. Added more tests here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8330#note_107091
On Wed Jun 18 18:35:42 2025 +0000, Nikolay Sivov wrote:
We need LVM_GETITEM test too for this, the fix looks misplaced. Good point, should be fixed now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8330#note_107092
On Tue Jun 17 21:07:37 2025 +0000, Nikolay Sivov wrote:
If this is specific to LVS_OWNERDATA path, we need more tests with callback mask. If it's not specific to LVS_OWNERDATA, the test should be placed elsewhere. Note that return value is not an index, so please compare with defined state constants, and without expect(), just plain ok(). Added some more tests for this and got rid of the expect, let me know what you think.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8330#note_107093
Nikolay Sivov (@nsivov) commented about dlls/comctl32/listview.c:
/* if the app stores all the data, handle it separately */ if (infoPtr->dwStyle & LVS_OWNERDATA) { - dispInfo.item.state = 0; + if ((lpLVItem->mask & LVIF_STATE)) lpLVItem->state = 0;
Remove extra parentheses please, the rest seems fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8330#note_107101
participants (3)
-
Connor McAdams -
Connor McAdams (＠cmcadams)
-
Nikolay Sivov (＠nsivov)