From: Angelo Haller angelo@szanni.org
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.
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.
Angelo Haller (6): comctl32/tests: Expand ownerdata listview tests. comctl32/listview: Move LVN_ODSTATECHANGED notification to function. comctl32/listview: Send LVN_ODSTATECHANGED only for virtual lists. comctl32/listview: Send LVN_ODSTATECHANGED notification. comctl32/listview: Send LVN_ODSTATECHANGED only for true ranges. comctl32/listview: Fix deselect on LVS_OWNERDATA.
dlls/comctl32/listview.c | 71 ++++++++++++++++++++++++++-------- dlls/comctl32/tests/listview.c | 59 ++++++++++++++++++++++++---- 2 files changed, 105 insertions(+), 25 deletions(-)
Signed-off-by: Angelo Haller angelo@szanni.org
From: Angelo Haller angelo@szanni.org
Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs.
Signed-off-by: Angelo Haller angelo@szanni.org --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ 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_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_to_2_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_deselect_all_select_3_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_deselect_3_select_2_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 }, { 0 } };
@@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res);
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res);
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res);
+ 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_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); + + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); }
From: Angelo Haller angelo@szanni.org
Signed-off-by: Angelo Haller angelo@szanni.org --- dlls/comctl32/listview.c | 47 ++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 730bf4aaddd..1ecebb63077 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -437,6 +437,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 BOOL 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); @@ -3557,16 +3558,11 @@ 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;
- /* Temporarily disable change notification - * If the control is LVS_OWNERDATA, we need to send - * only one LVN_ODSTATECHANGED notification. - * See MSDN documentation for LVN_ITEMCHANGED. - */ + /* 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; @@ -3579,13 +3575,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; @@ -8950,6 +8941,38 @@ static BOOL LISTVIEW_SetItemPosition(LISTVIEW_INFO *infoPtr, INT nItem, const PO return LISTVIEW_MoveIconTo(infoPtr, nItem, &Pt, FALSE); }
+/*** + * DESCRIPTION: + * Sets the state of multiple items for LVS_OWNERDATA listviews. + * Make sure to also disable per item notifications via the notification mask. + * + * PARAMETER(S): + * [I] infoPtr : valid pointer to the listview structure + * [I] nFirst : first item index + * [I] nLast : last item index + * [I] item : item or subitem info + * + * RETURN: + * SUCCESS : TRUE + * FAILURE : FALSE + */ +static BOOL LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT nLast, const LVITEMW *item) +{ + NMLVODSTATECHANGE nmlv; + + if (!item) return FALSE; + + ZeroMemory(&nmlv, sizeof(nmlv)); + nmlv.iFrom = nFirst; + nmlv.iTo = nLast; + nmlv.uOldState = 0; + nmlv.uNewState = item->state; + + notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); + + return TRUE; +} + /*** * DESCRIPTION: * Sets the state of one or many items.
On 5/10/22 01:17, Angelo Haller wrote:
+/***
- DESCRIPTION:
- Sets the state of multiple items for LVS_OWNERDATA listviews.
- Make sure to also disable per item notifications via the notification mask.
- PARAMETER(S):
- [I] infoPtr : valid pointer to the listview structure
- [I] nFirst : first item index
- [I] nLast : last item index
- [I] item : item or subitem info
- RETURN:
- SUCCESS : TRUE
- FAILURE : FALSE
- */
Please remove this header.
+static BOOL LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT nLast, const LVITEMW *item) +{
- NMLVODSTATECHANGE nmlv;
- if (!item) return FALSE;
- ZeroMemory(&nmlv, sizeof(nmlv));
- nmlv.iFrom = nFirst;
- nmlv.iTo = nLast;
- nmlv.uOldState = 0;
- nmlv.uNewState = item->state;
Are state fields always like that? E.g. old is always 0? Can "item" be legitimately NULL?
- notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv);
- return TRUE;
+}
If return value is never going to be used it's better to have it as void.
On 11/05/2022 01.00, Nikolay Sivov wrote:
On 5/10/22 01:17, Angelo Haller wrote:
+/***
- DESCRIPTION:
- Sets the state of multiple items for LVS_OWNERDATA listviews.
- Make sure to also disable per item notifications via the
notification mask.
- PARAMETER(S):
- [I] infoPtr : valid pointer to the listview structure
- [I] nFirst : first item index
- [I] nLast : last item index
- [I] item : item or subitem info
- RETURN:
- * SUCCESS : TRUE
- * FAILURE : FALSE
- */
Please remove this header.
I can remove that if so desired. I however do believe a remark to disable per item notifications is important, as this needs to be manually done on all call sites.
+static BOOL LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT nLast, const LVITEMW *item) +{ + NMLVODSTATECHANGE nmlv;
+ if (!item) return FALSE;
+ ZeroMemory(&nmlv, sizeof(nmlv)); + nmlv.iFrom = nFirst; + nmlv.iTo = nLast; + nmlv.uOldState = 0; + nmlv.uNewState = item->state;
Are state fields always like that? E.g. old is always 0? Can "item" be legitimately NULL?
I did not look into the legitimacy of the existing code of old always being 0. I believe that is the case though. LVN_ODSTATECHANGED seems to be only used for "notify about new selection". Deselection is seemingly always done via "deselect all" (item = -1), as can be seen in the tests I submitted. I have not found any other instances were windows sends LVN_ODSTATECHANGED yet.
Regarding "item" being NULL, I do not know if there is a legitimate case for that. I just mimicked the defensive? coding practices I found in LISTVIEW_SetItemState, which does check "item" for NULL.
+ notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv);
+ return TRUE; +}
If return value is never going to be used it's better to have it as void.
Again, I am happy to change that. This is, again, just adhering to the existing coding practices found within the file.
From: Angelo Haller angelo@szanni.org
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller angelo@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 1ecebb63077..5258c2e9518 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8960,6 +8960,7 @@ static BOOL LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv;
+ if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return FALSE; if (!item) return FALSE;
ZeroMemory(&nmlv, sizeof(nmlv));
From: Angelo Haller angelo@szanni.org
Send LVN_ODSTATECHANGED notification on selection change for listviews when LVS_OWNERDATA is set.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52534 Signed-off-by: Angelo Haller angelo@szanni.org --- dlls/comctl32/listview.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 5258c2e9518..41246a8c1d3 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3597,6 +3597,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) */ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) { + INT nFirst = -1; + INT nLast = -1; RANGES selection; DWORD old_mask; LVITEMW item; @@ -3648,21 +3650,25 @@ 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); + while(iterator_next(&i)) { + if (nFirst == -1) + nFirst = i.nItem; + nLast = i.nItem; + LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + } /* this will also destroy the selection */ iterator_destroy(&i);
+ LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); + infoPtr->notify_mask |= old_mask; LISTVIEW_SetItemFocus(infoPtr, nItem); }
When looking at this series, I found more disturbing thing in existing code - static BOOL bGroupSelect = TRUE; in mouse selection helper.
Not only it's global, but it also does not work correctly as far as I can tell. I tried with LVS_ICON case, and when alternating ctrl+click, ctrl+shift+click, selected pattern does not match what I see on Windows.
Do you think you can look into that before fixing owner-data case?
On 11/05/2022 01.05, Nikolay Sivov wrote:
When looking at this series, I found more disturbing thing in existing code - static BOOL bGroupSelect = TRUE; in mouse selection helper.
Not only it's global, but it also does not work correctly as far as I can tell. I tried with LVS_ICON case, and when alternating ctrl+click, ctrl+shift+click, selected pattern does not match what I see on Windows.
Do you think you can look into that before fixing owner-data case?
There are seemingly quite a few things broken in list views, hence everything marked as todo?
I sadly do not believe I have spare time to tinker with the LVS_ICON case or similar, I personally have never used those modes of operation. Just a fellow developer trying to fix a downstream issue by improving the upstream library.
From: Angelo Haller angelo@szanni.org
Signed-off-by: Angelo Haller angelo@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 41246a8c1d3..065f8bc90d8 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8967,6 +8967,7 @@ static BOOL LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n NMLVODSTATECHANGE nmlv;
if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return FALSE; + if (nFirst == nLast) return FALSE; if (!item) return FALSE;
ZeroMemory(&nmlv, sizeof(nmlv));
From: Angelo Haller angelo@szanni.org
Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change.
Enable LVS_OWNERDATA multi select tests for wine.
Signed-off-by: Angelo Haller angelo@szanni.org --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 065f8bc90d8..394ad79634f 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip)
lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on 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; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE);
res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);;
DestroyWindow(hwnd); }
-
Angelo Haller -
Nikolay Sivov