[PATCH v3 5/5] comctl32: LVM_INSERTITEM handler should send notifications with uChanged = LVIF_STATE.

[Dmitry Timoshkov]

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

diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
index 5c3b1a1636c..a961c8d4032 100644
--- a/dlls/comctl32/listview.c
+++ b/dlls/comctl32/listview.c
@@ -4257,7 +4257,13 @@ static BOOL set_main_item(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL
         nmlv.uNewState = (item.state & ~stateMask) | (lpLVItem->state & stateMask);
         nmlv.uOldState = item.state;
     }
-    nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
+    /* According to the tests uChanged in the notification for a new item
+     * has only LVIF_STATE flag set.
+     */
+    if (isNew)
+        nmlv.uChanged = LVIF_STATE;
+    else
+        nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
     nmlv.lParam = item.lParam;
 
     /* Send LVN_ITEMCHANGING notification if the item is not being
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c
index 92e11685331..3810cd8f4bc 100644
--- a/dlls/comctl32/tests/listview.c
+++ b/dlls/comctl32/tests/listview.c
@@ -6023,7 +6023,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, insert_item[i].mask != LVIF_STATE);
+            ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_focused0_seq, buf, FALSE);
         }
         else
         {
@@ -6100,7 +6100,7 @@ static void test_insertitem(void)
     item.lParam = 0xdeadbeef;
     ret = SendMessageA(hwnd, LVM_INSERTITEMA, 0, (LPARAM)&item);
     ok(ret == 3, "got %d\n", ret);
-    ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_selected_seq, "insert selected", TRUE);
+    ok_sequence(sequences, PARENT_SEQ_INDEX, parent_insert_selected_seq, "insert selected", FALSE);
 
     /* insert item 4 */
     item.mask = LVIF_PARAM;
-- 
2.35.1

[Nikolay Sivov]

On 2/11/22 17:25, Dmitry Timoshkov wrote:
> }
> -    nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> +    /* According to the tests uChanged in the notification for a new item
> +     * has only LVIF_STATE flag set.
> +     */
> +    if (isNew)
> +        nmlv.uChanged = LVIF_STATE;
> +    else
> +        nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
>       nmlv.lParam = item.lParam;

Shorter way I think is to tweak uChanged like this:

if (isNew) uChanged &= ~LVIF_STATE;

[Dmitry Timoshkov]

Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> > -    nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> > +    /* According to the tests uChanged in the notification for a new item
> > +     * has only LVIF_STATE flag set.
> > +     */
> > +    if (isNew)
> > +        nmlv.uChanged = LVIF_STATE;
> > +    else
> > +        nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> >       nmlv.lParam = item.lParam;
>
> Shorter way I think is to tweak uChanged like this:
>
> if (isNew) uChanged &= ~LVIF_STATE;

I didn't check if this makes the tests run without todo_wine, but if it
does, then yes, that looks good to me. Are you planning to send your version?

-- 
Dmitry.

[Nikolay Sivov]

On 2/21/22 11:56, Dmitry Timoshkov wrote:
> Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> > > -    nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> > > +    /* According to the tests uChanged in the notification for a new item
> > > +     * has only LVIF_STATE flag set.
> > > +     */
> > > +    if (isNew)
> > > +        nmlv.uChanged = LVIF_STATE;
> > > +    else
> > > +        nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> > >        nmlv.lParam = item.lParam;
> >
> > Shorter way I think is to tweak uChanged like this:
> >
> > if (isNew) uChanged &= ~LVIF_STATE;
> > I didn't check if this makes the tests run without todo_wine, but if it
> > does, then yes, that looks good to me. Are you planning to send your version?

Yes, I'll send both 4 and 5 after tests patches get in. I was going to 
send patch 4 under your name, since it's meant to be functional 
identical. For patch 5, I'll double check if my change works, but a note 
that it's based on your work will be definitely there. Is that alright?

[Dmitry Timoshkov]

Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> > > > -    nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> > > > +    /* According to the tests uChanged in the notification for a new item
> > > > +     * has only LVIF_STATE flag set.
> > > > +     */
> > > > +    if (isNew)
> > > > +        nmlv.uChanged = LVIF_STATE;
> > > > +    else
> > > > +        nmlv.uChanged = uChanged ? uChanged : lpLVItem->mask;
> > > >        nmlv.lParam = item.lParam;
> > >
> > > Shorter way I think is to tweak uChanged like this:
> > >
> > > if (isNew) uChanged &= ~LVIF_STATE;
> > > I didn't check if this makes the tests run without todo_wine, but if it
> > > does, then yes, that looks good to me. Are you planning to send your version?
>
> Yes, I'll send both 4 and 5 after tests patches get in. I was going to 
> send patch 4 under your name, since it's meant to be functional 
> identical. For patch 5, I'll double check if my change works, but a note 
> that it's based on your work will be definitely there. Is that alright?

Sure, that works.

-- 
Dmitry.