[PATCH v2 4/5] comctl32: LVM_INSERTITEM handler should send LVN_ITEMCHANGING/LVN_ITEMCHANGED notifications when the item state is being set.

[Dmitry Timoshkov]

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 dlls/comctl32/listview.c       | 9 +++++----
 dlls/comctl32/tests/listview.c | 6 +++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
index cd7fec28e27..137ca7c6529 100644
--- a/dlls/comctl32/listview.c
+++ b/dlls/comctl32/listview.c
@@ -4260,11 +4260,12 @@ static BOOL set_main_item(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL
     nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
     nmlv.lParam = item.lParam;
 
-    /* Send LVN_ITEMCHANGING notification, if the item is not being inserted
+    /* Send LVN_ITEMCHANGING notification:
+       if the item is not being inserted or its state is being set,
        and we are _NOT_ virtual (LVS_OWNERDATA), and change notifications
        are enabled. Even nothing really changed we still need to send this,
        in this case uChanged mask is just set to passed item mask. */
-    if (lpItem && !isNew && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
+    if (lpItem && (!isNew || (uChanged & LVIF_STATE)) && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
     {
       HWND hwndSelf = infoPtr->hwndSelf;
 
@@ -4352,8 +4353,8 @@ static BOOL set_main_item(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL
 	}
     }
 
-    /* if we're inserting the item, we're done */
-    if (isNew) return TRUE;
+    /* if we're inserting the item and its state is not being set, we're done */
+    if (isNew && !(uChanged & LVIF_STATE)) return TRUE;
 
     /* send LVN_ITEMCHANGED notification */
     if (lpLVItem->mask & LVIF_PARAM) nmlv.lParam = lpLVItem->lParam;
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c
index a53fec81c90..4521158d141 100644
--- a/dlls/comctl32/tests/listview.c
+++ b/dlls/comctl32/tests/listview.c
@@ -6017,7 +6017,7 @@ static void test_LVM_INSERTITEM(void)
         if ((insert_item[i].mask & LVIF_STATE) && (insert_item[i].state & (LVIS_FOCUSED | LVIS_SELECTED)))
         {
             sprintf(buf, "%d: insert focused", i);
-            ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused0_seq, buf, TRUE);
+            ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused0_seq, buf, insert_item[i].mask != LVIF_STATE);
         }
         else
         {
@@ -6054,7 +6054,7 @@ static void test_insertitem(void)
     item.iSubItem = 0;
     ret = SendMessageA(hwnd, LVM_INSERTITEMA, 0, (LPARAM)&item);
     ok(ret == 0, "got %d\n", ret);
-    ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused0_seq, "insert focused 0", TRUE);
+    ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused0_seq, "insert focused 0", FALSE);
 
     state = SendMessageA(hwnd, LVM_GETITEMSTATE, 0, LVIS_FOCUSED);
     ok(state == LVIS_FOCUSED, "got %x\n", state);
@@ -6067,7 +6067,7 @@ static void test_insertitem(void)
     item.iSubItem = 0;
     ret = SendMessageA(hwnd, LVM_INSERTITEMA, 0, (LPARAM)&item);
     ok(ret == 1, "got %d\n", ret);
-    ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused1_seq, "insert focused 1", TRUE);
+    ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused1_seq, "insert focused 1", FALSE);
 
     state = SendMessageA(hwnd, LVM_GETITEMSTATE, 1, LVIS_FOCUSED);
     ok(state == LVIS_FOCUSED, "got %x\n", state);
-- 
2.34.1

[Nikolay Sivov]

On 2/11/22 15:36, Dmitry Timoshkov wrote:
> -    if (lpItem && !isNew && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
> +    if (lpItem && (!isNew || (uChanged & LVIF_STATE)) && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
>       {
>         HWND hwndSelf = infoPtr->hwndSelf;
>
> @@ -4352,8 +4353,8 @@ static BOOL set_main_item(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL
>   }
>       }
>
> -    /* if we're inserting the item, we're done */
> -    if (isNew) return TRUE;
> +    /* if we're inserting the item and its state is not being set, we're done */
> +    if (isNew && !(uChanged & LVIF_STATE)) return TRUE;

This isn't right either. On Windows, inserting with e.g. 
LVIF_STATE/LVIS_CUT does not produce a change notification.

[Dmitry Timoshkov]

Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> On 2/11/22 15:36, Dmitry Timoshkov wrote:
> > -    if (lpItem && !isNew && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
> > +    if (lpItem && (!isNew || (uChanged & LVIF_STATE)) && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
> >       {
> >         HWND hwndSelf = infoPtr->hwndSelf;
> >
> > @@ -4352,8 +4353,8 @@ static BOOL set_main_item(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL
> >   }
> >       }
> >
> > -    /* if we're inserting the item, we're done */
> > -    if (isNew) return TRUE;
> > +    /* if we're inserting the item and its state is not being set, we're done */
> > +    if (isNew && !(uChanged & LVIF_STATE)) return TRUE;
>
> This isn't right either. On Windows, inserting with e.g. 
> LVIF_STATE/LVIS_CUT does not produce a change notification.

Thanks for the quick review, looks like I need to add a test for this
case as well.

-- 
Dmitry.

[Nikolay Sivov]

On 2/11/22 15:54, Dmitry Timoshkov wrote:
> Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> > On 2/11/22 15:36, Dmitry Timoshkov wrote:
> > > -    if (lpItem && !isNew && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
> > > +    if (lpItem && (!isNew || (uChanged & LVIF_STATE)) && (infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE))
> > >        {
> > >          HWND hwndSelf = infoPtr->hwndSelf;
> > >
> > > @@ -4352,8 +4353,8 @@ static BOOL set_main_item(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL
> > >        }
> > >        }
> > >
> > > -    /* if we're inserting the item, we're done */
> > > -    if (isNew) return TRUE;
> > > +    /* if we're inserting the item and its state is not being set, we're done */
> > > +    if (isNew && !(uChanged & LVIF_STATE)) return TRUE;
> > > This isn't right either. On Windows, inserting with e.g.
> > > LVIF_STATE/LVIS_CUT does not produce a change notification.
> > > Thanks for the quick review, looks like I need to add a test for this
> > > case as well.

I haven't tested properly, but I suspect it's only considered a change 
if it's (LVIS_SELECTED|LVIS_FOCUSED).