Re: [PATCH v2 resend 3/6] comctl32/listview: Send LVN_ODSTATECHANGED only for virtual lists.

[Angelo Haller]

On 11/06/2022 10.30, Angelo Haller wrote:
> On 11/06/2022 03.02, Zhiyi Zhang wrote:
> > On 6/11/22 07:56, Angelo Haller wrote:
> > > On 10/06/2022 03.13, Zhiyi Zhang wrote:
> > > > On 5/26/22 04:00, Angelo Haller wrote:
> > > > > From: Angelo Haller
> > > > >
> > > > > The LVN_ODSTATECHANGED notification should only be sent to lists
> > > > > that have LVS_OWNERDATA set.
> > > > > Signed-off-by: Angelo Haller
> > > > >
> > > > > dlls/comctl32/listview.c | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
> > > > > index 72ade724313..318df0a4093 100644
> > > > > --- a/dlls/comctl32/listview.c
> > > > > +++ b/dlls/comctl32/listview.c
> > > > > @@ -8946,6 +8946,7 @@ static VOID 
> > > > > LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n
> > > > >    {
> > > > >        NMLVODSTATECHANGE nmlv;
> > > > >    +    if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return;
> > > >
> > > > Make sense. It will be better if you can add a simple test before 
> > > > this patch and remove the todo_wines after the fix.
> > >
> > > Is this strictly necessary? The call site has special handling for:
> > > infoPtr->dwStyle & LVS_OWNERDATA
> > >
> > > We could guard the call to the function at the call site, if that is 
> > > preferred.
> > >
> > > Apart from that I am not sure how to write a test for this. This bug 
> > > is triggered by creating a non ownerdata list and selecting multiple 
> > > entries holding shift+ctrl and clicking with the mouse. This will 
> > > send an LVN_ODSTATECHANGED notification where it is not supposed to 
> > > (as it is not an ownerdata list).
> >
> > You can write a test that demonstrate the exact same thing. Create a 
> > non onwerdata listview control and test the message sequence doesn't 
> > contain LVN_ODSTATECHANGED, which should have a todo_wine
> > because Wine is currently broken in this case. Then you fix it in the 
> > next patch and removes the todo_wine.
> >
> > As for the mouse emulation, could you do it using only the keyboard? 
> > Such as sending VK_DOWN while holding Shift and Ctrl?
>
> This is exactly what I was trying to communicate. I can NOT trigger 
> the erroneous sending of LVN_ODSTATECHANGED via the keyboard.
>
> The bug is in LISTVIEW_AddGroupSelection which gets called from 
> LISTVIEW_LButtonDown.
>
> Patch 2/6 moves the offending code to a new function 
> LISTVIEW_SetOwnerdataState().
>
> This is why I asked about opening a bug report with an contrived 
> application that crashes with wine but not with windows.

I just opened a bug report with a test application attached. Hope that 
helps :)

https://bugs.winehq.org/show_bug.cgi?id=53123
> > > I have not found any test examples with mouse emulation. Maybe I 
> > > missed something.
> > >
> > > Or should I open a separate bug report for this line?
> > > > > if (!item) return;
> > > > >          ZeroMemory(&nmlv, sizeof(nmlv));