[PATCH] comctl32/listview: Fix the listview sorting error.
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(a)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"); -- 2.20.1
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
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 <chenhaoyang(a)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; }
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.
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 <chenhaoyang(a)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");
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 <chenhaoyang(a)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");
在 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 <chenhaoyang(a)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");
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(a)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");
participants (3)
-
Haoyang Chen -
Marvin -
Nikolay Sivov