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

[Angelo Haller]

On 30/06/2022 04.41, Zhiyi Zhang wrote:
> On 6/29/22 05:16, Angelo Haller wrote:
> > From: Angelo Haller
> >
> > 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
> >
> > 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.` ?

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.
> 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?

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