[PATCH v4 5/7] comctl32/listview: Send LVN_ODSTATECHANGED notification.

[Angelo Haller]

On 17/07/2022 02.29, Zhiyi Zhang wrote:
>
> On 7/1/22 01:57, Angelo Haller wrote:
>> On 30/06/2022 04.41, Zhiyi Zhang wrote:
>>> On 6/29/22 05:16, Angelo Haller wrote:
>>>> From: Angelo Haller <angelo at 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 at szanni.org>
>>>>
>>>> ---
>>>> v3: Merge nFirst & nLast definition into a single line.
>>>>       Add function call guard due to preceding patch changes.
>>>> ---
>>>>    dlls/comctl32/listview.c       | 16 +++++++++++-----
>>>>    dlls/comctl32/tests/listview.c |  6 +++---
>>>>    2 files changed, 14 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
>>>> index 5ba1924cbd7..0243d9a84ce 100644
>>>> --- a/dlls/comctl32/listview.c
>>>> +++ b/dlls/comctl32/listview.c
>>>> @@ -3604,6 +3604,7 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem)
>>>>     */
>>>>    static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem)
>>>>    {
>>>> +    INT nFirst = -1, nLast = -1;
>>>>        RANGES selection;
>>>>        DWORD old_mask;
>>>>        LVITEMW item;
>>>> @@ -3655,21 +3656,26 @@ 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);
>>>> +    }
>>> Hi Angelo,
>>>
>>> This looks like a separate change than what you describe in the subject. Please put it in a separate patch.
>> Hi Zhiyi,
>>
>> thanks for the review.
>>
>> I believe the patch does exactly what is described in the subject. Maybe I could add another line describing in detail what is being done, as it does not seem entirely clear? Maybe something like:
>>
>> `Compute the range by determining the first and last item of the selection, then send one LVN_ODSTATECHANGED notification.` ?
> Hi Angelo,
>
> Sorry for the late response. I was busy with other things and kind of forgot about this patch set. I would recommend you to use GitLab in the future so that I know
> there is a task assigned to me.
>
> For the code, add a line of 'Find the range for LVN_ODSTATECHANGED' before 'if (nFirst == -1)' and add an empty line before "LISTVIEW_SetItemState(infoPtr, i.nItem, &item); " should be enough.

Hi Zhiyi,

I migrated the patches to Gitlab and added the requested comment. Hope 
things look good now.

I don't believe I have the required rights to assign you, so here the 
link to the Gitlab MR:
https://gitlab.winehq.org/wine/wine/-/merge_requests/550

Best,
Angelo

>
>
>> The above code changes are for determining what the first and last items are. These are needed for making the call to `LISTVIEW_SetOwnerDataState()`, which is done in the code below. No other function calls are introduced in the patch.
>>
>> Moving the computation of nFirst and nLast to a different patch would trigger a "set but not used" warning by the compiler.
>>
>>> Other than the above. There are some typos in 1/7 but I took care of it. I made some changes to the patch series overall
>>> and attached them in the mail attachments. Mostly some style fix. You can make your changes on top of them.
>> Thanks, I guess I must have missed those even with my spell checker on.
>>> Then there are still some message sequences in test_ownerdata_multiselect() that don't pass. It looks like one issue for
>>> the rest of the todo_wines and they're probably existing failures. But could you fix them as well?
>> Yes, these are existing failures. I believe these should be addressed in a different patch series though.
>>
>> Windows seems to send some very strange message sequences on ownerdata lists. The current behavior in wine is correct for non ownerdata lists. I believe more rigorous testing for non ownerdata listviews would be needed to not introduce regressions.
>>
>> All the signals are being sent, it is just that the current sequence does not match Windows fully. I am happy to look into that when I have some extra time, but as previously stated, this will most likely be a similarly sized patch series to this one.
> Thank you for looking into it. If the whole series is too large, you can send the whole patch set to email first. Then send the first 5~7 patches to GitLab if they look good.
>
>>> Thanks for your work. It's much better than the last revision.
>> Should I resubmit the patches you attached to the mailing list? Leaving your Signed off intact as long as I do not change the patches?
> Yes, feel free to use those patches. Sign-offs are not necessary after the transition to Gitlab.
> So you can remove my sign offs. And when you send them through Gitlab, I will approve them if the merge request looks good.
>
> Thanks,
> Zhiyi
>
>> Best,
>> Angelo
>>
>>> Best Regards,
>>> Zhiyi
>>>
>>>
>>>>        /* this will also destroy the selection */
>>>>        iterator_destroy(&i);
>>>>    +    if (infoPtr->dwStyle & LVS_OWNERDATA)
>>>> +        LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item);
>>>> +
>>>>        infoPtr->notify_mask |= old_mask;
>>>>        LISTVIEW_SetItemFocus(infoPtr, nItem);
>>>>    }
>>>> diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c
>>>> index 7243a1858cd..d8a27ea18f4 100644
>>>> --- a/dlls/comctl32/tests/listview.c
>>>> +++ b/dlls/comctl32/tests/listview.c
>>>> @@ -3605,7 +3605,7 @@ static void test_ownerdata_multiselect(void)
>>>>        expect(0, res);
>>>>        ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX,
>>>>                    ownerdata_multiselect_select_0_to_1_odstatechanged_seq,
>>>> -                "ownerdata multiselect: select multiple via SHIFT+DOWN", TRUE);
>>>> +                "ownerdata multiselect: select multiple via SHIFT+DOWN", FALSE);
>>>>        res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0);
>>>>        expect(0, res);
>>>>        res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0);
>>>> @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void)
>>>>        expect(0, res);
>>>>        ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX,
>>>>                    ownerdata_multiselect_select_0_to_1_odstatechanged_seq,
>>>> -                "ownerdata multiselect: select multiple via SHIFT+CONTROL+DOWN", TRUE);
>>>> +                "ownerdata multiselect: select multiple via SHIFT+CONTROL+DOWN", FALSE);
>>>>        res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0);
>>>>        expect(0, res);
>>>>        res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0);
>>>> @@ -3676,7 +3676,7 @@ static void test_ownerdata_multiselect(void)
>>>>        expect(0, res);
>>>>        ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX,
>>>>                    ownerdata_multiselect_select_0_to_2_odstatechanged_seq,
>>>> -                "ownerdata multiselect: select multiple after skip via SHIFT+CONTROL+DOWN", TRUE);
>>>> +                "ownerdata multiselect: select multiple after skip via SHIFT+CONTROL+DOWN", FALSE);
>>>>        res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0);
>>>>        expect(0, res);
>>>>        res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0);
>>