From: Angelo Haller angelo@szanni.org
Warning: I have had access to the Windows Research Kernel (WRK) 1.2 ~10 years ago. These changes are regarding comctrl32 which is 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 (4): 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.
dlls/comctl32/listview.c | 59 ++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 12 deletions(-)
Signed-off-by: Angelo Haller angelo@szanni.org
From: Angelo Haller angelo@szanni.org
Signed-off-by: Angelo Haller angelo@szanni.org --- dlls/comctl32/listview.c | 41 +++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index cd7fec28e27..aafab6db3a7 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,7 +3558,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; @@ -3579,13 +3579,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; @@ -8945,6 +8940,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.
Hi, Angelo.
Thank you for patches. I'll have to ask you to add some tests first, in comctl32/tests/listview.c, to verify all changes you're making.
Thank you Nikolay.
I have actually tried doing so via the `ok_sequence(sequences, PARENT_SEQ_INDEX,...` commands but sadly failed as quite a few of the existing tests are still marked as wine_todo. Especially a lot of code regarding select/focus/deselect which are code paths that potential tests for these patches would trigger as well.
Is there a way of masking messages as "ignore" in `ok_sequence` that I missed? I would like to mask some WM_NOTIFY messages like LVN_ODCACHEHINT as this has not been implemented at all yet and LVN_GETDISPINFOA which gets called in a different order to windows.
Or would a test marked as wine_todo suffice?
On 21/02/2022 04.11, Nikolay Sivov wrote:
Hi, Angelo.
Thank you for patches. I'll have to ask you to add some tests first, in comctl32/tests/listview.c, to verify all changes you're making.
On 2/21/22 19:11, Angelo Haller wrote:
Thank you Nikolay.
I have actually tried doing so via the `ok_sequence(sequences, PARENT_SEQ_INDEX,...` commands but sadly failed as quite a few of the existing tests are still marked as wine_todo. Especially a lot of code regarding select/focus/deselect which are code paths that potential tests for these patches would trigger as well.
Is there a way of masking messages as "ignore" in `ok_sequence` that I missed? I would like to mask some WM_NOTIFY messages like LVN_ODCACHEHINT as this has not been implemented at all yet and LVN_GETDISPINFOA which gets called in a different order to windows.
Or would a test marked as wine_todo suffice?
We have "optional" flag for that, you can find it in existing message tests. If that becomes too problematic there is always an option to introduce another sequence just for LVN_OD* tests, and filter irrelevant notifications there, so they don't appear in test results at all.
On 21/02/2022 04.11, Nikolay Sivov wrote:
Hi, Angelo.
Thank you for patches. I'll have to ask you to add some tests first, in comctl32/tests/listview.c, to verify all changes you're making.
On 21/02/2022 11.35, Nikolay Sivov wrote:
On 2/21/22 19:11, Angelo Haller wrote:
Thank you Nikolay.
I have actually tried doing so via the `ok_sequence(sequences, PARENT_SEQ_INDEX,...` commands but sadly failed as quite a few of the existing tests are still marked as wine_todo. Especially a lot of code regarding select/focus/deselect which are code paths that potential tests for these patches would trigger as well.
Is there a way of masking messages as "ignore" in `ok_sequence` that I missed? I would like to mask some WM_NOTIFY messages like LVN_ODCACHEHINT as this has not been implemented at all yet and LVN_GETDISPINFOA which gets called in a different order to windows.
Or would a test marked as wine_todo suffice?
We have "optional" flag for that, you can find it in existing message tests. If that becomes too problematic there is always an option
I saw the optional flag. I thought those were `4.7x specific` or specific to different windows versions? I was looking more for a `wine_todo` flag for individual messages, because LVN_ODCACHEHINT is not really `optional` but rather a `wine_todo`.
Although such a flag would not solve the problem of LVN_ITEMCHANGED and LVN_GETDISPINFOA getting called in inverse order in windows and wine.
to introduce another sequence just for LVN_OD* tests, and filter irrelevant notifications there, so they don't appear in test results at all.
I'm finally grasping the workings of the different sequences. Easiest would of course be to introduce a new sequence entirely. Something like `PARENT_ODSTATECHANGED_SEQ_INDEX` that filters solely for WM_NOTIFY && LVN_ODSTATECHANGED. That is the only way I can think of to make the tests pass after applying these patches.
Maybe adding and testing for such a specific sequence would be acceptable when accompanied with a `PARENT_SEQ_INDEX` testing sequence that is still marked as wine todo?
On 21/02/2022 04.11, Nikolay Sivov wrote:
Hi, Angelo.
Thank you for patches. I'll have to ask you to add some tests first, in comctl32/tests/listview.c, to verify all changes you're making.
On 2/22/22 03:22, Angelo Haller wrote:
On 21/02/2022 11.35, Nikolay Sivov wrote:
On 2/21/22 19:11, Angelo Haller wrote:
Thank you Nikolay.
I have actually tried doing so via the `ok_sequence(sequences, PARENT_SEQ_INDEX,...` commands but sadly failed as quite a few of the existing tests are still marked as wine_todo. Especially a lot of code regarding select/focus/deselect which are code paths that potential tests for these patches would trigger as well.
Is there a way of masking messages as "ignore" in `ok_sequence` that I missed? I would like to mask some WM_NOTIFY messages like LVN_ODCACHEHINT as this has not been implemented at all yet and LVN_GETDISPINFOA which gets called in a different order to windows.
Or would a test marked as wine_todo suffice?
We have "optional" flag for that, you can find it in existing message tests. If that becomes too problematic there is always an option
I saw the optional flag. I thought those were `4.7x specific` or specific to different windows versions? I was looking more for a `wine_todo` flag for individual messages, because LVN_ODCACHEHINT is not really `optional` but rather a `wine_todo`.
Optional is used for both cases - it's something we allow if it happens, and ignore if it doesn't. There is no wine_todo for individual messages.
Although such a flag would not solve the problem of LVN_ITEMCHANGED and LVN_GETDISPINFOA getting called in inverse order in windows and wine.
I'd simply mark LVN_GETDISPINFO as optional.
to introduce another sequence just for LVN_OD* tests, and filter irrelevant notifications there, so they don't appear in test results at all.
I'm finally grasping the workings of the different sequences. Easiest would of course be to introduce a new sequence entirely. Something like `PARENT_ODSTATECHANGED_SEQ_INDEX` that filters solely for WM_NOTIFY && LVN_ODSTATECHANGED. That is the only way I can think of to make the tests pass after applying these patches.
That would be a last resort solution, as you can see from existing tests, we don't use a new sequence for each test.
Maybe adding and testing for such a specific sequence would be acceptable when accompanied with a `PARENT_SEQ_INDEX` testing sequence that is still marked as wine todo?
Once we have a test that triggers LVN_ODSTATECHANGED, we'll figure out how to make it pass.
On 21/02/2022 04.11, Nikolay Sivov wrote:
Hi, Angelo.
Thank you for patches. I'll have to ask you to add some tests first, in comctl32/tests/listview.c, to verify all changes you're making.
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 aafab6db3a7..18041eb239e 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8959,6 +8959,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 18041eb239e..39293467290 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3601,6 +3601,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; @@ -3652,21 +3654,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); }
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 39293467290..88a988a67e3 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8966,6 +8966,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));
-
Angelo Haller -
Nikolay Sivov