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

[Zhiyi Zhang]

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.

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.

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?

Thanks for your work. It's much better than the last revision.

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