[PATCH v2 resend 6/6] comctl32/listview: Fix deselect on LVS_OWNERDATA.

[Zhiyi Zhang]

On 6/11/22 02:18, Angelo Haller wrote:
> On 10/06/2022 03.13, Zhiyi Zhang wrote:
>>
>> On 5/26/22 04:00, Angelo Haller wrote:
>>> From: Angelo Haller <angelo at 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 at 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 bb394974906..a71e34b99d9 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;
>>> +    }
>> Please add a separate test for this. Select and then deselect all items instead of mixing it with other tests.
>
> Maybe the comment needs to be better. Deselect all (-1) is the ONLY signal ever sent on ANY deselection,
> be it one or multiple items (for LVS_OWNERDATA).
> This is how win32 implements it, hence there is no way of making this a separate test.
>
> Maybe the line should read:
>
> /* Always send one deselect all (-1) notification for LVS_OWNERDATA style instead of
>  * informing individual items of deselection */
>
> And I am still looking into the other remarks. Some might need reordering as I won't be able to
> enable the tests without this last patch.
>
> Thanks for the feedback so far!

I was thinking adding a test that selects items, and them maybe send a mouse click somewhere
in the blank space of the listview control to deselect them all to show that only a -1 notification is sent.
But it may be difficult to get the tests work reliably. If you can't make it work, I guess mixing it with
other tests is okay.


>
>>>             /* 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);;
>> Extra ;
>>
>>
>> Thanks,
>> Zhiyi
>>
>>
>>>         DestroyWindow(hwnd);
>>>   }
>
>