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

[Zhiyi Zhang]

On 6/11/22 23:34, Angelo Haller wrote:
> On 11/06/2022 02.55, Zhiyi Zhang wrote:
>>
>> 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.
>
> We do not need mouse clicks. See my reply to your comment in 1/6. The deselect of multiple items can be triggered by selecting multiple holding shift, releasing shift and moving the cursor again. This deselects multiple. The code testing for this is already in 1/6.

Using the keyboard VK_DOWN to deselect simultaneously selects another item. That's why I think it's mixing the results.
You should be able to emulate a mouse click on the blank listview client area to deselect all items, without generating
extra messages. This way we can see only the essential messages are sent and it's clearer.

>
> My comment was with regards to enabling the tests individually. I might be able to do so by reordering and spitting some of the patches.
>
>>
>>
>>>>>              /* 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);
>>>>>    }
>>>
>