[PATCH v6 0/6] MR550: comctl32/listview: Fix LVS_OWNERDATA list view multi select.
The following patches fix sending of the LVN_ODSTATECHANGED notification for LVS_OWNERDATA list views, adding more refined tests in the process and fixing various bugs. This is v5, with the added comment in 3/6 as requested on the mailing list. These are the patches sent as a response to v4 by Zhiyi Zhang on the mailing list, unmodified apart from 3/6 or f24f4b7fd. --- Warning: I have had access to the Windows Research Kernel (WRK) 1.2 ~10 years ago. These changes are regarding comctrl32 & tests which are NOT part of the WRK. As outlined in https://wiki.winehq.org/Developer_FAQ this should therefore satisfy the requirement of ONLY submitting patches to components I have NOT had access to. -- v6: comctl32/tests: Add more ownerdata listview tests. https://gitlab.winehq.org/wine/wine/-/merge_requests/550
From: Angelo Haller <angelo(a)szanni.org> Send one deselect all items notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change. Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index ab328b3e798..3761a61286e 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,7 +3406,14 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip) lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; - + + /* Only send one deselect all (-1) notification for LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) + { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } + /* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; iterator_rangesitems(&i, ranges_diff(clone, toSkip)); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/550
From: Angelo Haller <angelo(a)szanni.org> Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 3761a61286e..a12b19b8083 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -438,6 +438,7 @@ static INT LISTVIEW_GetStringWidthT(const LISTVIEW_INFO *, LPCWSTR, BOOL); static BOOL LISTVIEW_KeySelection(LISTVIEW_INFO *, INT, BOOL); static UINT LISTVIEW_GetItemState(const LISTVIEW_INFO *, INT, UINT); static BOOL LISTVIEW_SetItemState(LISTVIEW_INFO *, INT, const LVITEMW *); +static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *, INT, INT, const LVITEMW *); static LRESULT LISTVIEW_VScroll(LISTVIEW_INFO *, INT, INT); static LRESULT LISTVIEW_HScroll(LISTVIEW_INFO *, INT, INT); static BOOL LISTVIEW_EnsureVisible(LISTVIEW_INFO *, INT, BOOL); @@ -3565,7 +3566,6 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) INT nFirst = min(infoPtr->nSelectionMark, nItem); INT nLast = max(infoPtr->nSelectionMark, nItem); HWND hwndSelf = infoPtr->hwndSelf; - NMLVODSTATECHANGE nmlv; DWORD old_mask; LVITEMW item; INT i; @@ -3587,13 +3587,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) for (i = nFirst; i <= nLast; i++) LISTVIEW_SetItemState(infoPtr,i,&item); - ZeroMemory(&nmlv, sizeof(nmlv)); - nmlv.iFrom = nFirst; - nmlv.iTo = nLast; - nmlv.uOldState = 0; - nmlv.uNewState = item.state; + LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); - notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); if (!IsWindow(hwndSelf)) return FALSE; infoPtr->notify_mask |= old_mask; @@ -9023,6 +9018,22 @@ static BOOL LISTVIEW_SetItemPosition(LISTVIEW_INFO *infoPtr, INT nItem, const PO return LISTVIEW_MoveIconTo(infoPtr, nItem, &Pt, FALSE); } +/* Make sure to also disable per item notifications via the notification mask. */ +static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT nLast, const LVITEMW *item) +{ + NMLVODSTATECHANGE nmlv; + + if (!item) return; + + ZeroMemory(&nmlv, sizeof(nmlv)); + nmlv.iFrom = nFirst; + nmlv.iTo = nLast; + nmlv.uOldState = 0; + nmlv.uNewState = item->state; + + notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); +} + /*** * DESCRIPTION: * Sets the state of one or many items. -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/550
From: Angelo Haller <angelo(a)szanni.org> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53123 Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index a12b19b8083..bf22a90e987 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3587,7 +3587,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) for (i = nFirst; i <= nLast; i++) LISTVIEW_SetItemState(infoPtr,i,&item); - LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); + if (infoPtr->dwStyle & LVS_OWNERDATA) + LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); if (!IsWindow(hwndSelf)) return FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/550
From: Angelo Haller <angelo(a)szanni.org> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52534 Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 16 ++++++++++++---- dlls/comctl32/tests/listview.c | 4 ++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bf22a90e987..e48b1fa2847 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3610,6 +3610,7 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) */ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) { + INT nFirst = -1, nLast = -1; RANGES selection; DWORD old_mask; LVITEMW item; @@ -3661,21 +3662,28 @@ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) iterator_destroy(&i); } - /* disable per item notifications on LVS_OWNERDATA style - FIXME: single LVN_ODSTATECHANGED should be used */ + /* Disable per item notifications on LVS_OWNERDATA style */ old_mask = infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE; if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->notify_mask &= ~NOTIFY_MASK_ITEM_CHANGE; LISTVIEW_DeselectAllSkipItems(infoPtr, selection); - iterator_rangesitems(&i, selection); while(iterator_next(&i)) - LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + { + /* Find the range for LVN_ODSTATECHANGED */ + if (nFirst == -1) + nFirst = i.nItem; + nLast = i.nItem; + LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + } /* this will also destroy the selection */ iterator_destroy(&i); + if (infoPtr->dwStyle & LVS_OWNERDATA) + LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); + infoPtr->notify_mask |= old_mask; LISTVIEW_SetItemFocus(infoPtr, nItem); } diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index ed5222a5ee8..fec1a83de4d 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3577,7 +3577,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + "ownerdata select multiple notification", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3594,7 +3594,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + "ownerdata select multiple notification", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/550
From: Angelo Haller <angelo(a)szanni.org> Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index e48b1fa2847..38d43a9f742 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -9032,6 +9032,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv; + if (nFirst == nLast) return; if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv)); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/550
From: Angelo Haller <angelo(a)szanni.org> Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 183 ++++++++++++++++++++++++++------- 1 file changed, 143 insertions(+), 40 deletions(-) diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index fec1a83de4d..ac82ef9b168 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -256,11 +256,77 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } }; -static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_modkey_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_move_0_to_1_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_3_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_3_modkey_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_3_to_2_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_move_3_to_2_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_3_to_1_odstatechanged_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, { 0 } }; @@ -3544,11 +3610,58 @@ static void test_ownerdata(void) DestroyWindow(hwnd); } + static void test_ownerdata_multiselect(void) { HWND hwnd; DWORD res; LVITEMA item; + unsigned int i; + char buf[256]; + + static const struct + { + BOOL hold_shift; + BOOL hold_control; + UINT press_key; + UINT selected_count; + const char *context; + const struct message *expected; + BOOL todo; + } + key_tests[] = + { + /* First down then up */ + { TRUE, FALSE, VK_DOWN, 2, "select multiple via SHIFT+DOWN", + ownerdata_multiselect_select_0_to_1_odstatechanged_seq, FALSE }, + { TRUE, FALSE, VK_UP, 1, "select one item via SHIFT+UP", + ownerdata_multiselect_select_0_modkey_odstatechanged_seq, TRUE }, + { TRUE, TRUE, VK_DOWN, 2, "select multiple via SHIFT+CONTROL+DOWN", + ownerdata_multiselect_select_0_to_1_odstatechanged_seq, FALSE }, + { TRUE, TRUE, VK_UP, 1, "select one item via SHIFT+CONTROL+UP", + ownerdata_multiselect_select_0_modkey_odstatechanged_seq, TRUE }, + { FALSE, TRUE, VK_DOWN, 1, "keep selection but move cursor via CONTROL+DOWN", + ownerdata_multiselect_move_0_to_1_odstatechanged_seq, FALSE }, + { TRUE, TRUE, VK_DOWN, 3, "select multiple after skip via SHIFT+CONTROL+DOWN", + ownerdata_multiselect_select_0_to_2_odstatechanged_seq, FALSE }, + { FALSE, FALSE, VK_DOWN, 1, "deselect all, select item 3 via DOWN", + ownerdata_multiselect_select_3_odstatechanged_seq, FALSE }, + /* First up then down */ + { TRUE, FALSE, VK_UP, 2, "select multiple via SHIFT+UP", + ownerdata_multiselect_select_3_to_2_odstatechanged_seq, FALSE }, + { TRUE, FALSE, VK_DOWN, 1, "select one item via SHIFT+DOWN", + ownerdata_multiselect_select_3_modkey_odstatechanged_seq, TRUE }, + { TRUE, TRUE, VK_UP, 2, "select multiple via SHIFT+CONTROL+UP", + ownerdata_multiselect_select_3_to_2_odstatechanged_seq, FALSE }, + { TRUE, TRUE, VK_DOWN, 1, "select one item via SHIFT+CONTROL+DOWN", + ownerdata_multiselect_select_3_modkey_odstatechanged_seq, TRUE }, + { FALSE, TRUE, VK_UP, 1, "keep selection but move cursor via CONTROL+UP", + ownerdata_multiselect_move_3_to_2_odstatechanged_seq, FALSE }, + { TRUE, TRUE, VK_UP, 3, "select multiple after skip via SHIFT+CONTROL+UP", + ownerdata_multiselect_select_3_to_1_odstatechanged_seq, FALSE }, + { FALSE, FALSE, VK_UP, 1, "deselect all, select item 0 via UP", + ownerdata_multiselect_select_0_odstatechanged_seq, FALSE }, + }; hwnd = create_listview_control(LVS_OWNERDATA | LVS_REPORT); ok(hwnd != NULL, "failed to create a listview window\n"); @@ -3557,6 +3670,7 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(0, res); + /* Select and focus the first row */ memset(&item, 0, sizeof(item)); item.state = LVIS_SELECTED | LVIS_FOCUSED; item.stateMask = LVIS_SELECTED | LVIS_FOCUSED; @@ -3564,46 +3678,35 @@ static void test_ownerdata_multiselect(void) expect(TRUE, res); res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); - res = SendMessageA(hwnd, LVM_SETSELECTIONMARK, 0, 0); expect(0, res); - hold_key(VK_SHIFT); - - flush_sequences(sequences, NUM_MSG_SEQUENCES); - - res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); - expect(0, res); - - ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", FALSE); - - res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); - expect(0, res); - - res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); - expect(2, res); - - hold_key(VK_CONTROL); - - flush_sequences(sequences, NUM_MSG_SEQUENCES); - - res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); - expect(0, res); - - ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", FALSE); - - res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); - expect(0, res); - - release_key(VK_CONTROL); - release_key(VK_SHIFT); + /* Select/deselect rows using UP/DOWN and SHIFT/CONTROL keys */ + for (i = 0; i < ARRAY_SIZE(key_tests); i++) + { + flush_sequences(sequences, NUM_MSG_SEQUENCES); - res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); - expect(3, res); + if (key_tests[i].hold_shift) + hold_key(VK_SHIFT); + if (key_tests[i].hold_control) + hold_key(VK_CONTROL); + + res = SendMessageA(hwnd, WM_KEYDOWN, key_tests[i].press_key, 0); + expect(0, res); + sprintf(buf, "ownerdata multiselect: %s", key_tests[i].context); + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, key_tests[i].expected, + buf, key_tests[i].todo); + res = SendMessageA(hwnd, WM_KEYUP, key_tests[i].press_key, 0); + expect(0, res); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(key_tests[i].selected_count, res); + + if (key_tests[i].hold_shift) + release_key(VK_SHIFT); + if (key_tests[i].hold_control) + release_key(VK_CONTROL); + } DestroyWindow(hwnd); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/550
On Tue Aug 2 02:41:54 2022 +0000, Zhiyi Zhang wrote:
Hi, we usually mark such test sequences static const. Also the struct is usually not exposed to other tests. I made a patch at https://pastebin.com/XycMrvjq. Otherwise, things look good. Thanks. Fixed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/550#note_5600
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
DestroyWindow(hwnd); }
+
Please remove this new line. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/550#note_5622
participants (3)
-
Angelo Haller -
Angelo Haller (＠szanni)
-
Zhiyi Zhang (＠zhiyi)