The listview bug is caused by passing the incorrect index to the LISTVIEW_SortItems callers custom compare function. If we pass the copied array indices, which are different because of the sorting, the caller is going to use those indices for their unsorted array which don't match.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56140
-- v5: comctl32: Fix sorting for listview. comctl32/tests: Add test for listview sorting order.
From: Jacob Czekalla jczekalla@codeweavers.com
--- dlls/comctl32/tests/listview.c | 79 ++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 1081a045cde..2163d1b14ee 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3028,6 +3028,83 @@ static void test_subitem_rect(void) DestroyWindow(hwnd); }
+static INT WINAPI test_CallBackCompareEx(LPARAM first, LPARAM second, LPARAM lParam) +{ + HWND hwnd_List_view = (HWND)lParam; + CHAR buffer1[256], buffer2[256]; + int itm1, itm2; + + ListView_GetItemTextA(hwnd_List_view, first, 0, buffer1, sizeof(buffer1)); + ListView_GetItemTextA(hwnd_List_view, second, 0, buffer2, sizeof(buffer2)); + + itm1 = atoi(buffer1); + itm2 = atoi(buffer2); + + return (itm1 - itm2); +} + +static void test_sort_order(void) +{ + LVITEMA lvi = {0}; + LV_COLUMNA lvc = {0}; + RECT rcClient; + CHAR buffer[256]; + CHAR col_names[][2] = { "1", "2" }; + HWND hwndListView = create_listview_control(LVS_REPORT); + int prev_value; + + GetClientRect(hwndparent, &rcClient); + + lvc.mask = LVCF_TEXT | LVCF_WIDTH | LVCF_SUBITEM; + lvc.pszText = col_names[0]; + lvc.cx = rcClient.right / 2; + lvc.iSubItem = 0; + SendMessageA(hwndListView, LVM_INSERTCOLUMNA, 0, (LPARAM) &lvc); + + lvc.pszText = col_names[1]; + lvc.iSubItem = 1; + lvc.cx = rcClient.right / 2; + SendMessageA(hwndListView, LVM_INSERTCOLUMNA, 1, (LPARAM) &lvc); + + srand(1234); + + for (int i = 0; i < 100; i++) + { + lvi.mask = LVIF_TEXT; + lvi.iItem = i; + lvi.iSubItem = 0; + lvi.pszText = buffer; + sprintf(buffer, "%d", rand() % 100); + SendMessageA(hwndListView, LVM_INSERTITEMA, 0, (LPARAM) &lvi); + + lvi.iSubItem = 1; + sprintf(buffer, "%d", rand() % 100); + SendMessageA(hwndListView, LVM_SETITEMTEXTA, i, (LPARAM) &lvi); + } + + SendMessageA(hwndListView, LVM_SORTITEMSEX, (WPARAM) hwndListView, (LPARAM) test_CallBackCompareEx); + + memset(&lvi, 0, sizeof(lvi)); + for (int i = 1; i < 100; i++) + { + ListView_GetItemTextA(hwndListView, i-1, 0, buffer, sizeof(buffer)); + prev_value = atoi(buffer); + + ListView_GetItemTextA(hwndListView, i, 0, buffer, sizeof(buffer)); + + if (atoi(buffer) < prev_value) + { + prev_value = 1; + break; + } + prev_value = 0; + } + + todo_wine ok(!prev_value, "ListView not sorted correctly.\n"); + + DestroyWindow(hwndListView); +} + /* comparison callback for test_sorting */ static INT WINAPI test_CallBackCompare(LPARAM first, LPARAM second, LPARAM lParam) { @@ -3211,6 +3288,8 @@ static void test_sorting(void) ok(lstrcmpA(buff, names[3]) == 0, "Expected '%s', got '%s'\n", names[3], buff);
DestroyWindow(hwnd); + + test_sort_order(); }
static void test_ownerdata(void)
From: Jacob Czekalla jczekalla@codeweavers.com
--- dlls/comctl32/listview.c | 2 +- dlls/comctl32/tests/listview.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index c82473b6205..0302e3e19a0 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -9396,7 +9396,7 @@ static BOOL LISTVIEW_SortItems(LISTVIEW_INFO *infoPtr, PFNLVCOMPARE pfnCompare, if (infoPtr->nFocusedItem >= 0) focusedItem = DPA_GetPtr(infoPtr->hdpaItems, infoPtr->nFocusedItem);
- context.items = hdpaItems; + context.items = infoPtr->hdpaItems; context.compare_func = pfnCompare; context.lParam = lParamSort; if (IsEx) diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 2163d1b14ee..1fbb62db044 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3100,7 +3100,7 @@ static void test_sort_order(void) prev_value = 0; }
- todo_wine ok(!prev_value, "ListView not sorted correctly.\n"); + ok(!prev_value, "ListView not sorted correctly.\n");
DestroyWindow(hwndListView); }
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147440
Your paranoid android.
=== debian11b (64 bit WoW report) ===
Report validation errors: dxgi:dxgi has unaccounted for todo messages dxgi:dxgi has unaccounted for skip messages The report seems to have been truncated
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
DestroyWindow(hwnd);}
+static INT WINAPI test_CallBackCompareEx(LPARAM first, LPARAM second, LPARAM lParam) +{
- HWND hwnd_List_view = (HWND)lParam;
Change hwnd_List_view to list_view or hwnd to make it more consistent.
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
- ListView_GetItemTextA(hwnd_List_view, second, 0, buffer2, sizeof(buffer2));
- itm1 = atoi(buffer1);
- itm2 = atoi(buffer2);
- return (itm1 - itm2);
+}
+static void test_sort_order(void) +{
- LVITEMA lvi = {0};
- LV_COLUMNA lvc = {0};
- RECT rcClient;
- CHAR buffer[256];
- CHAR col_names[][2] = { "1", "2" };
- HWND hwndListView = create_listview_control(LVS_REPORT);
Please make the style consistent. We prefer the underscore style in the wine code.
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
- CHAR buffer[256];
- CHAR col_names[][2] = { "1", "2" };
- HWND hwndListView = create_listview_control(LVS_REPORT);
- int prev_value;
- GetClientRect(hwndparent, &rcClient);
- lvc.mask = LVCF_TEXT | LVCF_WIDTH | LVCF_SUBITEM;
- lvc.pszText = col_names[0];
- lvc.cx = rcClient.right / 2;
- lvc.iSubItem = 0;
- SendMessageA(hwndListView, LVM_INSERTCOLUMNA, 0, (LPARAM) &lvc);
- lvc.pszText = col_names[1];
- lvc.iSubItem = 1;
- lvc.cx = rcClient.right / 2;
You can remove this line.
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
lvi.iSubItem = 0;lvi.pszText = buffer;sprintf(buffer, "%d", rand() % 100);SendMessageA(hwndListView, LVM_INSERTITEMA, 0, (LPARAM) &lvi);lvi.iSubItem = 1;sprintf(buffer, "%d", rand() % 100);SendMessageA(hwndListView, LVM_SETITEMTEXTA, i, (LPARAM) &lvi);- }
- SendMessageA(hwndListView, LVM_SORTITEMSEX, (WPARAM) hwndListView, (LPARAM) test_CallBackCompareEx);
- memset(&lvi, 0, sizeof(lvi));
- for (int i = 1; i < 100; i++)
- {
ListView_GetItemTextA(hwndListView, i-1, 0, buffer, sizeof(buffer));
Add spaces before and after the minus sign.
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
- {
lvi.mask = LVIF_TEXT;lvi.iItem = i;lvi.iSubItem = 0;lvi.pszText = buffer;sprintf(buffer, "%d", rand() % 100);SendMessageA(hwndListView, LVM_INSERTITEMA, 0, (LPARAM) &lvi);lvi.iSubItem = 1;sprintf(buffer, "%d", rand() % 100);SendMessageA(hwndListView, LVM_SETITEMTEXTA, i, (LPARAM) &lvi);- }
- SendMessageA(hwndListView, LVM_SORTITEMSEX, (WPARAM) hwndListView, (LPARAM) test_CallBackCompareEx);
- memset(&lvi, 0, sizeof(lvi));
Is this needed?
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
SendMessageA(hwndListView, LVM_SETITEMTEXTA, i, (LPARAM) &lvi);- }
- SendMessageA(hwndListView, LVM_SORTITEMSEX, (WPARAM) hwndListView, (LPARAM) test_CallBackCompareEx);
- memset(&lvi, 0, sizeof(lvi));
- for (int i = 1; i < 100; i++)
- {
ListView_GetItemTextA(hwndListView, i-1, 0, buffer, sizeof(buffer));prev_value = atoi(buffer);ListView_GetItemTextA(hwndListView, i, 0, buffer, sizeof(buffer));if (atoi(buffer) < prev_value){prev_value = 1;
Using a different variable, for example, "sorted", would make it more intuitive.
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/listview.c:
ok(lstrcmpA(buff, names[3]) == 0, "Expected '%s', got '%s'\n", names[3], buff); DestroyWindow(hwnd);
- test_sort_order();
I would call it test_custom_sort() and call it from the main function.
-
Jacob Czekalla -
Jacob Czekalla (＠JacobCzekalla)
-
Marvin -
Zhiyi Zhang (＠zhiyi)