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

[Zhiyi Zhang]

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.


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