[PATCH] comctl32/listview: Fix the listview sorting error.

[Nikolay Sivov]

On 3/8/21 11:35 AM, Haoyang Chen wrote:
>
> 在 2021/3/8 下午4:17, Nikolay Sivov 写道:
>>
>> On 3/8/21 11:09 AM, Haoyang Chen wrote:
>>> hi,
>>>
>>> lParam may hold the initial sequence. During the sorting process, this
>>> sequence is disrupted. But the initial queue order may still be used
>>> during sorting.
>>> For example, the original queue: 6 4 5.
>>> First sort result: 4 6 5.
>>> Second sort result: 4 6 5.
>>> Here it is wrong, because it should compare "6" and "5", but the index
>>> stored in the lParam of "6" is 0 (it has not changed), which is the
>>> first data, so it actually compares "4" and "5".
>> I still don't see how this affects listview item's lParam. Do you mean
>> that during LVM_SORTITEMS execution order does not change? E.g. if you
>> ask for index N in sort callback you'll get index N in pre-sorted order?
>
> It should be said that the "lParam" does not change, while the
> hdpa->ptrs queue keeps changing.
>
> The lParam holds the index that the user has to use to get the data,
> but wine treats it as the current order.
>
> eg: The index of lParam in "6" is always 0, "4" = 1, "5" = 2.

I think I see what you mean now, it should be fixed differently,
DPA_Sort() does not provide such stability on windows either.

I'll send something. Test should be improved regardless, it only needs
to verify that on each callback invocation LVM_GET* returns data in
original order.

>
>
> thands.
>
>>> I will send V2 for the test error.
>> Alright.
>>
>>> Thanks.
>>>
>>> 在 2021/3/8 上午11:22, Haoyang Chen 写道:
>>>> The original sequence may be stored in lparam. During the sorting
>>>> process,
>>>> this sequence is out-of-order, but lparam remains unchanged.
>>>>
>>>> Signed-off-by: Haoyang Chen <chenhaoyang at uniontech.com>
>>>> ---
>>>>    dlls/comctl32/dpa.c            | 13 ++++++--
>>>>    dlls/comctl32/tests/listview.c | 59
>>>> ++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 70 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/dlls/comctl32/dpa.c b/dlls/comctl32/dpa.c
>>>> index 36b3a422e8c..69e9d0a33bb 100644
>>>> --- a/dlls/comctl32/dpa.c
>>>> +++ b/dlls/comctl32/dpa.c
>>>> @@ -818,8 +818,17 @@ BOOL WINAPI DPA_Sort (HDPA hdpa, PFNDPACOMPARE
>>>> pfnCompare, LPARAM lParam)
>>>>        TRACE("(%p %p 0x%lx)\n", hdpa, pfnCompare, lParam);
>>>>          if ((hdpa->nItemCount > 1) && (hdpa->ptrs))
>>>> -        DPA_QuickSort (hdpa->ptrs, 0, hdpa->nItemCount - 1,
>>>> -                       pfnCompare, lParam);
>>>> +    {
>>>> +        LPVOID *ptrs = HeapAlloc (hdpa->hHeap, HEAP_ZERO_MEMORY,
>>>> +                                             hdpa->nItemCount *
>>>> sizeof(LPVOID));
>>>> +        if (!ptrs)
>>>> +                return FALSE;
>>>> +        memcpy(ptrs, hdpa->ptrs, hdpa->nItemCount * sizeof(LPVOID));
>>>> +        DPA_QuickSort (ptrs, 0, hdpa->nItemCount - 1, pfnCompare,
>>>> lParam);
>>>> +
>>>> +        memcpy(hdpa->ptrs, ptrs, hdpa->nItemCount * sizeof(LPVOID));
>>>> +        HeapFree (hdpa->hHeap, 0, ptrs);
>>>> +    }
>>>>          return TRUE;
>>>>    }
>>>> diff --git a/dlls/comctl32/tests/listview.c
>>>> b/dlls/comctl32/tests/listview.c
>>>> index e95b81f5bb1..7b036a9f6c1 100644
>>>> --- a/dlls/comctl32/tests/listview.c
>>>> +++ b/dlls/comctl32/tests/listview.c
>>>> @@ -2921,6 +2921,28 @@ static INT WINAPI test_CallBackCompare(LPARAM
>>>> first, LPARAM second, LPARAM lPara
>>>>        return (first > second ? 1 : -1);
>>>>    }
>>>>    +static INT WINAPI test_CallBackCompare1(LPARAM first, LPARAM
>>>> second, LPARAM lParam)
>>>> +{
>>>> +    CHAR str1[5];
>>>> +    CHAR str2[5];
>>>> +    INT  r;
>>>> +    HWND hwnd = (HWND)lParam;
>>>> +    LV_ITEMA item = {0};
>>>> +
>>>> +    if (first == second) return 0;
>>>> +    item.cchTextMax = 5;
>>>> +    item.iSubItem = 0;
>>>> +    item.pszText = str1;
>>>> +    r = SendMessageA(hwnd, LVM_GETITEMTEXTA, first, (LPARAM)&item);
>>>> +    expect(TRUE, r);
>>>> +
>>>> +    item.pszText = str2;
>>>> +    r = SendMessageA(hwnd, LVM_GETITEMTEXTA, second, (LPARAM)&item);
>>>> +    expect(TRUE, r);
>>>> +
>>>> +    return atoi(str1) > atoi(str2) ? 1 : -1;
>>>> +}
>>>> +
>>>>    static void test_sorting(void)
>>>>    {
>>>>        HWND hwnd;
>>>> @@ -2929,6 +2951,9 @@ static void test_sorting(void)
>>>>        LONG_PTR style;
>>>>        static CHAR names[][5] = {"A", "B", "C", "D", "0"};
>>>>        CHAR buff[10];
>>>> +    static CHAR before_sort_array[][5] = {"6","3","1","4","2"};
>>>> +    static CHAR after_sort_arary[][5] = {"1","2","3","4","6"};
>>>> +    INT i;
>>>>          hwnd = create_listview_control(LVS_REPORT);
>>>>        ok(hwnd != NULL, "failed to create a listview window\n");
>>>> @@ -2979,6 +3004,40 @@ static void test_sorting(void)
>>>>          DestroyWindow(hwnd);
>>>>    +    hwnd = create_listview_control(LVS_REPORT);
>>>> +    ok(hwnd != NULL, "failed to create a listview window\n");
>>>> +
>>>> +    item.mask = LVIF_PARAM | LVIF_TEXT;
>>>> +    item.iSubItem = 0;
>>>> +    item.cchTextMax = 5;
>>>> +
>>>> +    for (i = 0; i < sizeof(before_sort_array)/5; i++)
>>>> +    {
>>>> +        item.iItem = i;
>>>> +        item.lParam = i;
>>>> +        item.pszText = &before_sort_array[i][0];
>>>> +        r = SendMessageA(hwnd, LVM_INSERTITEMA, 0, (LPARAM)&item);
>>>> +        expect(i, r);
>>>> +    }
>>>> +
>>>> +    r = SendMessageA(hwnd, LVM_SORTITEMS, (WPARAM)(LPARAM)hwnd,
>>>> (LPARAM)test_CallBackCompare1);
>>>> +    expect(TRUE, r);
>>>> +
>>>> +    for (i = 0; i < sizeof(after_sort_arary)/5; i++)
>>>> +    {
>>>> +        CHAR str[5];
>>>> +        item.iItem = i;
>>>> +        item.cchTextMax = 5;
>>>> +        item.iSubItem = 0;
>>>> +        item.pszText = str;
>>>> +        r = SendMessageA(hwnd, LVM_GETITEMTEXTA, i, (LPARAM)&item);
>>>> +        expect(TRUE, r);
>>>> +
>>>> +        expect(0, strcmp(str,&after_sort_arary[i][0]));
>>>> +    }
>>>> +
>>>> +    DestroyWindow(hwnd);
>>>> +
>>>>        /* switch to LVS_SORTASCENDING when some items added */
>>>>        hwnd = create_listview_control(LVS_REPORT);
>>>>        ok(hwnd != NULL, "failed to create a listview window\n");
>>>
>>
>>
>>
>
>