[PATCH v3 0/4] MR11507: */listbox: Handle out-of-bounds SETTOPINDEX
If the index is negative or greater than or equal the number of items, we should return LB_ERR to match windows. If it is greater than the top index but less than the number of items, it still gets clamped. Discovered when investigating inconsistencies in test results between Windows and WINE in !11491. -- v3: user32/listbox: Handle out-of-bounds SETTOPINDEX comctl32/listbox: Handle out-of-bounds SETTOPINDEX user32/tests: Check listbox SETTOPINDEX errors comctl32/tests: Check listbox SETTOPINDEX errors https://gitlab.winehq.org/wine/wine/-/merge_requests/11507
From: Tobiasz Laskowski <tlaskowski@codeweavers.com> --- dlls/comctl32/tests/listbox.c | 70 +++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 8ac595a355b..557433ed481 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -2996,6 +2996,75 @@ static void test_integral_resize(void) DestroyWindow(parent); } +static void test_LB_SETTOPINDEX(void) { + HWND parent, listbox; + int i; + int ret; + + parent = create_parent(); + listbox = CreateWindowA(WC_LISTBOXA, "TestList", + WS_CHILD | WS_VISIBLE, 0, 0, 200, 100, parent, NULL, NULL, 0); + ok(!!listbox, "got error %lu\n", GetLastError()); + + for (i = 0; i < 10; i++) + { + SendMessageA(listbox, LB_ADDSTRING, 0, (LPARAM)"item"); + } + + /* Normal range */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 3, 0); + ok(!ret, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + ok(ret == 3, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_SETTOPINDEX, 0, 0); + ok(!ret, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + ok(ret == 0, "got %d\n", ret); + + /* Less than item count, but greater than max top index (to which it gets clamped) */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 5, 0); + ok(!ret, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + ok(ret == 4, "got %d\n", ret); + + /* Reset to a non-boundary top item */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 2, 0); + ok(!ret, "got %d\n", ret); + + /* Greater than or equal to item count (out of bounds) */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 10, 0); + todo_wine + ok(ret == LB_ERR, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + todo_wine + ok(ret == 2, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_SETTOPINDEX, 20, 0); + todo_wine + ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + todo_wine + ok(ret == 2, "got %d\n", ret); + + /* Negative (out of bounds) */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, -1, 0); + todo_wine + ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + todo_wine + ok(ret == 2, "got %d\n", ret); + + DestroyWindow(listbox); + DestroyWindow(parent); +} + START_TEST(listbox) { ULONG_PTR ctx_cookie; @@ -3030,6 +3099,7 @@ START_TEST(listbox) test_LB_FINDSTRING(); test_keypresses(); test_integral_resize(); + test_LB_SETTOPINDEX(); uninit_winevent_hook(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11507
From: Tobiasz Laskowski <tlaskowski@codeweavers.com> --- dlls/user32/tests/listbox.c | 72 ++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/dlls/user32/tests/listbox.c b/dlls/user32/tests/listbox.c index 3aba6a297bf..b2036b04525 100644 --- a/dlls/user32/tests/listbox.c +++ b/dlls/user32/tests/listbox.c @@ -188,7 +188,7 @@ check (DWORD style, const struct listbox_test test) free(txtw); free(txt); } - + /* Confirm the count of items, and that an invalid delete does not remove anything */ res = SendMessageA(hLB, LB_GETCOUNT, 0, 0); ok((res==4), "Expected 4 items, got %d\n", res); @@ -2579,6 +2579,75 @@ static void test_integral_resize(void) DestroyWindow(parent); } +static void test_LB_SETTOPINDEX(void) { + HWND parent, listbox; + int i; + int ret; + + parent = create_parent(); + listbox = CreateWindowA("listbox", "TestList", + WS_CHILD | WS_VISIBLE, 0, 0, 200, 100, parent, NULL, NULL, 0); + ok(!!listbox, "got error %lu\n", GetLastError()); + + for (i = 0; i < 10; i++) + { + SendMessageA(listbox, LB_ADDSTRING, 0, (LPARAM)"item"); + } + + /* Normal range */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 3, 0); + ok(!ret, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + ok(ret == 3, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_SETTOPINDEX, 0, 0); + ok(!ret, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + ok(ret == 0, "got %d\n", ret); + + /* Less than item count, but greater than max top index (to which it gets clamped) */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 5, 0); + ok(!ret, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + ok(ret == 4, "got %d\n", ret); + + /* Reset to a non-boundary top item */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 2, 0); + ok(!ret, "got %d\n", ret); + + /* Greater than or equal to item count (out of bounds) */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, 10, 0); + todo_wine + ok(ret == LB_ERR, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + todo_wine + ok(ret == 2, "got %d\n", ret); + + ret = SendMessageA(listbox, LB_SETTOPINDEX, 20, 0); + todo_wine + ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + todo_wine + ok(ret == 2, "got %d\n", ret); + + /* Negative (out of bounds) */ + ret = SendMessageA(listbox, LB_SETTOPINDEX, -1, 0); + todo_wine + ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); + + ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); + todo_wine + ok(ret == 2, "got %d\n", ret); + + DestroyWindow(listbox); + DestroyWindow(parent); +} + START_TEST(listbox) { const struct listbox_test SS = @@ -2680,4 +2749,5 @@ START_TEST(listbox) test_LBS_NODATA(); test_LB_FINDSTRING(); test_integral_resize(); + test_LB_SETTOPINDEX(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11507
From: Tobiasz Laskowski <tlaskowski@codeweavers.com> If the index is negative or greater than or equal the number of items, we should return LB_ERR to match windows. If it is greater than the top index but less than the number of items, it still gets clamped. --- dlls/comctl32/tests/listbox.c | 6 ------ dlls/comctl32_v6/listbox.c | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 557433ed481..bcef9abe423 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -3037,28 +3037,22 @@ static void test_LB_SETTOPINDEX(void) { /* Greater than or equal to item count (out of bounds) */ ret = SendMessageA(listbox, LB_SETTOPINDEX, 10, 0); - todo_wine ok(ret == LB_ERR, "got %d\n", ret); ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); - todo_wine ok(ret == 2, "got %d\n", ret); ret = SendMessageA(listbox, LB_SETTOPINDEX, 20, 0); - todo_wine ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); - todo_wine ok(ret == 2, "got %d\n", ret); /* Negative (out of bounds) */ ret = SendMessageA(listbox, LB_SETTOPINDEX, -1, 0); - todo_wine ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); - todo_wine ok(ret == 2, "got %d\n", ret); DestroyWindow(listbox); diff --git a/dlls/comctl32_v6/listbox.c b/dlls/comctl32_v6/listbox.c index 7dec0e2d999..fecbc61c649 100644 --- a/dlls/comctl32_v6/listbox.c +++ b/dlls/comctl32_v6/listbox.c @@ -386,8 +386,8 @@ static LRESULT LISTBOX_SetTopItem( LB_DESCR *descr, INT index, BOOL scroll ) TRACE("setting top item %d, scroll %d\n", index, scroll); + if (index < 0 || index >= descr->nb_items) return LB_ERR; if (index > max) index = max; - if (index < 0) index = 0; if (descr->style & LBS_MULTICOLUMN) index -= index % descr->page_size; if (descr->top_item == index) return LB_OKAY; if (scroll) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11507
From: Tobiasz Laskowski <tlaskowski@codeweavers.com> --- dlls/user32/listbox.c | 2 +- dlls/user32/tests/listbox.c | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index c569b296842..55514c3b0b2 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -395,8 +395,8 @@ static LRESULT LISTBOX_SetTopItem( LB_DESCR *descr, INT index, BOOL scroll ) TRACE("setting top item %d, scroll %d\n", index, scroll); + if (index < 0 || index >= descr->nb_items) return LB_ERR; if (index > max) index = max; - if (index < 0) index = 0; if (descr->style & LBS_MULTICOLUMN) index -= index % descr->page_size; if (descr->top_item == index) return LB_OKAY; if (scroll) diff --git a/dlls/user32/tests/listbox.c b/dlls/user32/tests/listbox.c index b2036b04525..697d66b2263 100644 --- a/dlls/user32/tests/listbox.c +++ b/dlls/user32/tests/listbox.c @@ -2620,28 +2620,22 @@ static void test_LB_SETTOPINDEX(void) { /* Greater than or equal to item count (out of bounds) */ ret = SendMessageA(listbox, LB_SETTOPINDEX, 10, 0); - todo_wine ok(ret == LB_ERR, "got %d\n", ret); ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); - todo_wine ok(ret == 2, "got %d\n", ret); ret = SendMessageA(listbox, LB_SETTOPINDEX, 20, 0); - todo_wine ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); - todo_wine ok(ret == 2, "got %d\n", ret); /* Negative (out of bounds) */ ret = SendMessageA(listbox, LB_SETTOPINDEX, -1, 0); - todo_wine ok(ret == LB_ERR, "Expected LB_ERR, got %d\n", ret); ret = SendMessageA(listbox, LB_GETTOPINDEX, 0, 0); - todo_wine ok(ret == 2, "got %d\n", ret); DestroyWindow(listbox); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11507
On Fri Jul 31 09:57:04 2026 +0000, Nikolay Sivov wrote:
The order was fine. It's good when you have tests first, because fixing commits will show which tests were fixed. That makes sense. I've updated again so the tests are added first with \`todo_wine\`, and then updated when the differences are corrected.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11507#note_147460
Sorry, I didn't notice it earlier. You are changing a helper that's used in a dozen of places. This new condition could break existing assumptions. That's why it's sometimes better to keep public interface with its own helper that can stay unchanged. Not sure what's the best way would be here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11507#note_147509
participants (3)
-
Nikolay Sivov (＠nsivov) -
Tobi Laskowski (＠tobil)
-
Tobiasz Laskowski