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

Angelo Haller wine-devel at szanni.org
Sat Jun 11 10:34:32 CDT 2022


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.

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);
>>>>    }
>>




More information about the wine-devel mailing list