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

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

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=86551

Your paranoid android.

=== w2008s64 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w7u_2qxl (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w7u_adm (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w7u_el (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w8 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w8adm (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w864 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064v1507 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064v1809 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w10pro64 (32 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== wvistau64 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w2008s64 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w864 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064v1507 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064v1809 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w1064_2qxl (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w10pro64 (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w10pro64_ar (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w10pro64_he (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w10pro64_ja (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

=== w10pro64_zh_CN (64 bit report) ===

comctl32:
listview.c:3036: Test failed: Expected 0, got 1
listview.c:3036: Test failed: Expected 0, got -1

[Nikolay Sivov]

On 3/8/21 6:22 AM, Haoyang Chen wrote:
> The original sequence may be stored in lparam. During the sorting process,
> this sequence is out-of-order, but lparam remains unchanged.

Could you explain some more, how does it break?
> Signed-off-by: Haoyang Chen
>
> 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;
>
> }

If it's DPA issue, you should test DPA API directly, the fact that we
use it internally for Listview is implementation details.
> 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");

Looks like this produces test failures.

[Haoyang Chen]

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 will send V2 for the test error.

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

[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?
> 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
> >
> > 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");

[Haoyang Chen]

在 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.

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

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