[PATCH v4 1/7] comctl32/listbox: Resize the entire item array at once in SetCount
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v4: Dumb mistake with get_item_string in patch 5, sorry for noise. dlls/comctl32/listbox.c | 43 ++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c92c2a2..db7d474 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -144,6 +144,12 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) descr->items_size = items_size; descr->items = items; } + + if ((descr->style & LBS_NODATA) && items_size > descr->nb_items) + { + memset(&descr->items[descr->nb_items], 0, + (items_size - descr->nb_items) * sizeof(LB_ITEMDATA)); + } return TRUE; } @@ -1742,25 +1748,36 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) /*********************************************************************** * LISTBOX_SetCount */ -static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) +static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count ) { - LRESULT ret; + UINT orig_num = descr->nb_items; if (!(descr->style & LBS_NODATA)) return LB_ERR; - /* FIXME: this is far from optimal... */ - if (count > descr->nb_items) - { - while (count > descr->nb_items) - if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0) - return ret; - } - else if (count < descr->nb_items) + if (!resize_storage(descr, count)) + return LB_ERRSPACE; + descr->nb_items = count; + + if (count) { - while (count < descr->nb_items) - if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0) - return ret; + LISTBOX_UpdateScroll(descr); + if (count < orig_num) + { + descr->anchor_item = min(descr->anchor_item, count - 1); + if (descr->selected_item >= count) + descr->selected_item = -1; + + /* If we removed the scrollbar, reset the top of the list */ + if (count <= descr->page_size && orig_num > descr->page_size) + LISTBOX_SetTopItem(descr, 0, TRUE); + + descr->focus_item = min(descr->focus_item, count - 1); + } + + /* If it was empty before growing, set focus to the first item */ + else if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE); } + else SendMessageW(descr->self, LB_RESETCONTENT, 0, 0); InvalidateRect( descr->self, NULL, TRUE ); return LB_OKAY; -- 2.20.1
Needed for LBS_NODATA. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index db7d474..c84d317 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -155,6 +155,8 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) { + if (!(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))) + return index == descr->selected_item; return descr->items[index].selected; } -- 2.20.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/tests/listbox.c | 130 ++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 68e4073..0cdcdea 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -784,6 +784,135 @@ static void test_listbox_height(void) DestroyWindow( hList ); } +static void test_changing_selection_styles(void) +{ + static const DWORD styles[] = + { + 0, + LBS_NODATA | LBS_OWNERDRAWFIXED + }; + static const DWORD selstyles[] = + { + 0, + LBS_MULTIPLESEL, + LBS_EXTENDEDSEL, + LBS_MULTIPLESEL | LBS_EXTENDEDSEL + }; + static const LONG selexpect_single[] = { 0, 0, 1 }; + static const LONG selexpect_single2[] = { 1, 0, 0 }; + static const LONG selexpect_multi[] = { 1, 0, 1 }; + static const LONG selexpect_multi2[] = { 1, 1, 0 }; + + HWND parent, listbox; + DWORD style; + LONG ret; + UINT i, j, k; + + parent = create_parent(); + ok(parent != NULL, "Failed to create parent window.\n"); + for (i = 0; i < ARRAY_SIZE(styles); i++) + { + /* Test if changing selection styles affects selection storage */ + for (j = 0; j < ARRAY_SIZE(selstyles); j++) + { + LONG setcursel_expect, selitemrange_expect, getselcount_expect; + const LONG *selexpect; + + listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | selstyles[j] | WS_CHILD | WS_VISIBLE, + 0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0); + ok(listbox != NULL, "%u: Failed to create ListBox window.\n", j); + + if (selstyles[j] & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) + { + setcursel_expect = LB_ERR; + selitemrange_expect = LB_OKAY; + getselcount_expect = 2; + selexpect = selexpect_multi; + } + else + { + setcursel_expect = 2; + selitemrange_expect = LB_ERR; + getselcount_expect = LB_ERR; + selexpect = selexpect_single; + } + + for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++) + { + ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"x"); + ok(ret == k, "%u: Unexpected return value %d, expected %d.\n", j, ret, k); + } + ret = SendMessageA(listbox, LB_GETCOUNT, 0, 0); + ok(ret == ARRAY_SIZE(selexpect_multi), "%u: Unexpected count %d.\n", j, ret); + + /* Select items with different methods */ + ret = SendMessageA(listbox, LB_SETCURSEL, 2, 0); + ok(ret == setcursel_expect, "%u: Unexpected return value %d.\n", j, ret); + ret = SendMessageA(listbox, LB_SELITEMRANGE, TRUE, MAKELPARAM(0, 0)); + ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret); + ret = SendMessageA(listbox, LB_SELITEMRANGE, TRUE, MAKELPARAM(2, 2)); + ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret); + + /* Verify that the proper items are selected */ + for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++) + { + ret = SendMessageA(listbox, LB_GETSEL, k, 0); + ok(ret == selexpect[k], "%u: Unexpected selection state %d, expected %d.\n", + j, ret, selexpect[k]); + } + + /* Now change the selection style */ + style = GetWindowLongA(listbox, GWL_STYLE); + ok((style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == selstyles[j], + "%u: unexpected window styles %#x.\n", j, style); + if (selstyles[j] & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) + style &= ~selstyles[j]; + else + style |= LBS_MULTIPLESEL | LBS_EXTENDEDSEL; + SetWindowLongA(listbox, GWL_STYLE, style); + style = GetWindowLongA(listbox, GWL_STYLE); + ok(!(style & selstyles[j]), "%u: unexpected window styles %#x.\n", j, style); + + /* Verify that the same items are selected */ + ret = SendMessageA(listbox, LB_GETSELCOUNT, 0, 0); + ok(ret == getselcount_expect, "%u: expected %d from LB_GETSELCOUNT, got %d\n", + j, getselcount_expect, ret); + + for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++) + { + ret = SendMessageA(listbox, LB_GETSEL, k, 0); + ok(ret == selexpect[k], "%u: Unexpected selection state %d, expected %d.\n", + j, ret, selexpect[k]); + } + + /* Lastly see if we can still change the selection as before with old style */ + if (setcursel_expect != LB_ERR) setcursel_expect = 0; + ret = SendMessageA(listbox, LB_SETCURSEL, 0, 0); + ok(ret == setcursel_expect, "%u: Unexpected return value %d.\n", j, ret); + ret = SendMessageA(listbox, LB_SELITEMRANGE, TRUE, MAKELPARAM(1, 1)); + ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret); + ret = SendMessageA(listbox, LB_SELITEMRANGE, FALSE, MAKELPARAM(2, 2)); + ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret); + + /* And verify the selections */ + selexpect = (selstyles[j] & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) ? selexpect_multi2 : selexpect_single2; + ret = SendMessageA(listbox, LB_GETSELCOUNT, 0, 0); + ok(ret == getselcount_expect, "%u: expected %d from LB_GETSELCOUNT, got %d\n", + j, getselcount_expect, ret); + + for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++) + { + ret = SendMessageA(listbox, LB_GETSEL, k, 0); + ok(ret == selexpect[k], "%u: Unexpected selection state %d, expected %d.\n", + j, ret, selexpect[k]); + } + + DestroyWindow(listbox); + } + } + DestroyWindow(parent); +} + static void test_itemfrompoint(void) { /* WS_POPUP is required in order to have a more accurate size calculation ( @@ -2483,6 +2612,7 @@ START_TEST(listbox) test_LB_SELITEMRANGE(); test_LB_SETCURSEL(); test_listbox_height(); + test_changing_selection_styles(); test_itemfrompoint(); test_listbox_item_data(); test_listbox_LB_DIR(); -- 2.20.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c84d317..34d555a 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) +{ + return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; +} + static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -565,7 +570,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, if (focused) dis.itemState |= ODS_FOCUS; if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED; - dis.itemData = item ? item->data : 0; + dis.itemData = get_item_data(descr, index); dis.rcItem = *rect; TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n", descr->self, index, item ? debugstr_w(item->str) : "", action, @@ -788,8 +793,7 @@ static LRESULT LISTBOX_GetText( LB_DESCR *descr, INT index, LPWSTR buffer, BOOL } else { if (buffer) - *((ULONG_PTR *)buffer) = (descr->style & LBS_NODATA) - ? 0 : descr->items[index].data; + *((ULONG_PTR *)buffer) = get_item_data(descr, index); len = sizeof(ULONG_PTR); } return len; @@ -837,7 +841,7 @@ static INT LISTBOX_FindStringPos( LB_DESCR *descr, LPCWSTR str, BOOL exact ) /* note that some application (MetaStock) expects the second item * to be in the listbox */ cis.itemID1 = index; - cis.itemData1 = descr->items[index].data; + cis.itemData1 = get_item_data(descr, index); cis.itemID2 = -1; cis.itemData2 = (ULONG_PTR)str; cis.dwLocaleId = descr->locale; @@ -1641,7 +1645,7 @@ static LRESULT LISTBOX_InsertString( LB_DESCR *descr, INT index, LPCWSTR str ) static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) { /* save the item data before it gets freed by LB_RESETCONTENT */ - ULONG_PTR item_data = descr->items[index].data; + ULONG_PTR item_data = get_item_data(descr, index); LPWSTR item_str = descr->items[index].str; if (!descr->nb_items) @@ -2643,7 +2647,7 @@ static LRESULT CALLBACK LISTBOX_WindowProc( HWND hwnd, UINT msg, WPARAM wParam, SetLastError(ERROR_INVALID_INDEX); return LB_ERR; } - return (descr->style & LBS_NODATA) ? 0 : descr->items[wParam].data; + return get_item_data(descr, wParam); case LB_SETITEMDATA: if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items)) -- 2.20.1
On Tue, Feb 12, 2019 at 03:47:00PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c84d317..34d555a 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) +{ + return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; +} +
nit-pick (since I've commented on the next patch), could you group all of the item helpers with is_item_selected()? Huw.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v4: Dumb mistake with get_item_string, sorry for noise. I've left FindStringPos alone because I'm not 100% sure if a listbox with strings is allowed to have a NULL str (which would give a different code path than currently, if I were to check for NULL from get_item_string instead). LISTBOX_lstrcmpiW ends up using CompareStringW which does check for NULL, so I'm not entirely certain about it. dlls/comctl32/listbox.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 34d555a..f99406a 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -132,6 +132,11 @@ static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; } +static WCHAR *get_item_string( const LB_DESCR *descr, UINT index ) +{ + return HAS_STRINGS(descr) ? descr->items[index].str : NULL; +} + static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -573,7 +578,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, dis.itemData = get_item_data(descr, index); dis.rcItem = *rect; TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n", - descr->self, index, item ? debugstr_w(item->str) : "", action, + descr->self, index, debugstr_w(get_item_string(descr, index)), action, dis.itemState, wine_dbgstr_rect(rect) ); SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis); SelectClipRgn( hdc, hrgn ); @@ -1646,7 +1651,7 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) { /* save the item data before it gets freed by LB_RESETCONTENT */ ULONG_PTR item_data = get_item_data(descr, index); - LPWSTR item_str = descr->items[index].str; + LPWSTR item_str = get_item_string(descr, index); if (!descr->nb_items) SendMessageW( descr->self, LB_RESETCONTENT, 0, 0 ); @@ -1668,8 +1673,7 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) dis.itemData = item_data; SendMessageW( descr->owner, WM_DELETEITEM, id, (LPARAM)&dis ); } - if (HAS_STRINGS(descr)) - HeapFree( GetProcessHeap(), 0, item_str ); + HeapFree( GetProcessHeap(), 0, item_str ); } -- 2.20.1
On Tue, Feb 12, 2019 at 03:47:01PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
v4: Dumb mistake with get_item_string, sorry for noise.
I've left FindStringPos alone because I'm not 100% sure if a listbox with strings is allowed to have a NULL str (which would give a different code path than currently, if I were to check for NULL from get_item_string instead). LISTBOX_lstrcmpiW ends up using CompareStringW which does check for NULL, so I'm not entirely certain about it.
_InsertString inserts an empty string in this case, but I don't find any tests for this. Could you add one? There are also many over places (e.g. the 2nd half of _PaintItem) that could use this helper. The goal is to have all reads of ->str go through this helper, even in cases where the code is inside a HAS_STRINGS() block. Huw.
On 2/13/19 11:50 AM, Huw Davies wrote:
On Tue, Feb 12, 2019 at 03:47:01PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
v4: Dumb mistake with get_item_string, sorry for noise.
I've left FindStringPos alone because I'm not 100% sure if a listbox with strings is allowed to have a NULL str (which would give a different code path than currently, if I were to check for NULL from get_item_string instead). LISTBOX_lstrcmpiW ends up using CompareStringW which does check for NULL, so I'm not entirely certain about it.
_InsertString inserts an empty string in this case, but I don't find any tests for this. Could you add one?
There are also many over places (e.g. the 2nd half of _PaintItem) that could use this helper. The goal is to have all reads of ->str go through this helper, even in cases where the code is inside a HAS_STRINGS() block.
Huw.
Ok, I'll see about the test. I didn't want to mess with PaintItem because it uses the item pointer directly, so presumably it's valid already. Since it's used in so many places, I'll use a local variable to hold it at the beginning (if valid), and get rid of the "item" variable, since I can just check for index instead. Does that sound fine?
On Wed, Feb 13, 2019 at 12:45:52PM +0200, Gabriel Ivăncescu wrote:
On 2/13/19 11:50 AM, Huw Davies wrote:
On Tue, Feb 12, 2019 at 03:47:01PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
v4: Dumb mistake with get_item_string, sorry for noise.
I've left FindStringPos alone because I'm not 100% sure if a listbox with strings is allowed to have a NULL str (which would give a different code path than currently, if I were to check for NULL from get_item_string instead). LISTBOX_lstrcmpiW ends up using CompareStringW which does check for NULL, so I'm not entirely certain about it.
_InsertString inserts an empty string in this case, but I don't find any tests for this. Could you add one?
There are also many over places (e.g. the 2nd half of _PaintItem) that could use this helper. The goal is to have all reads of ->str go through this helper, even in cases where the code is inside a HAS_STRINGS() block.
Huw.
Ok, I'll see about the test.
Thanks.
I didn't want to mess with PaintItem because it uses the item pointer directly, so presumably it's valid already.
The item variable should start to disappear from most of these functions when they start using the helpers.
Since it's used in so many places, I'll use a local variable to hold it at the beginning (if valid), and get rid of the "item" variable, since I can just check for index instead. Does that sound fine?
Yes, using a local variable to hold the string is absolutely fine. Huw.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- is_singlesel_NODATA is needed otherwise it fails the tests for multi-selection nodata, because it's not implemented yet. dlls/comctl32/listbox.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index f99406a..706a0b8 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == LBS_NODATA; +} + static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; @@ -170,6 +175,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; } +static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) +{ + if (!is_singlesel_NODATA(descr)) descr->items[index].selected = state; +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -1433,7 +1443,7 @@ static LRESULT LISTBOX_SelectItemRange( LB_DESCR *descr, INT first, for (i = first; i <= last; i++) { if (descr->items[i].selected) continue; - descr->items[i].selected = TRUE; + set_item_selected_state(descr, i, TRUE); LISTBOX_InvalidateItemRect(descr, i); } } @@ -1442,7 +1452,7 @@ static LRESULT LISTBOX_SelectItemRange( LB_DESCR *descr, INT first, for (i = first; i <= last; i++) { if (!descr->items[i].selected) continue; - descr->items[i].selected = FALSE; + set_item_selected_state(descr, i, FALSE); LISTBOX_InvalidateItemRect(descr, i); } } @@ -1475,10 +1485,10 @@ static LRESULT LISTBOX_SetSelection( LB_DESCR *descr, INT index, { INT oldsel = descr->selected_item; if (index == oldsel) return LB_OKAY; - if (oldsel != -1) descr->items[oldsel].selected = FALSE; - if (index != -1) descr->items[index].selected = TRUE; - if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT ); + if (oldsel != -1) set_item_selected_state(descr, oldsel, FALSE); + if (index != -1) set_item_selected_state(descr, index, TRUE); descr->selected_item = index; + if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT ); if (index != -1) LISTBOX_RepaintItem( descr, index, ODA_SELECT ); if (send_notify && descr->nb_items) SEND_NOTIFICATION( descr, (index != -1) ? LBN_SELCHANGE : LBN_SELCANCEL ); -- 2.20.1
On Tue, Feb 12, 2019 at 03:47:02PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
is_singlesel_NODATA is needed otherwise it fails the tests for multi-selection nodata, because it's not implemented yet.
dlls/comctl32/listbox.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index f99406a..706a0b8 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == LBS_NODATA; +} + static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; @@ -170,6 +175,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) +{ + if (!is_singlesel_NODATA(descr)) descr->items[index].selected = state; +}
To mirror is_item_selected() I'd suggest only setting selected in the multiselect case, so this would become: if (IS_MULTISELECT(descr)) descr->items[index].selected = state; Huw.
On 2/13/19 12:00 PM, Huw Davies wrote:
On Tue, Feb 12, 2019 at 03:47:02PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
is_singlesel_NODATA is needed otherwise it fails the tests for multi-selection nodata, because it's not implemented yet.
dlls/comctl32/listbox.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index f99406a..706a0b8 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == LBS_NODATA; +} + static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; @@ -170,6 +175,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) +{ + if (!is_singlesel_NODATA(descr)) descr->items[index].selected = state; +}
To mirror is_item_selected() I'd suggest only setting selected in the multiselect case, so this would become:
if (IS_MULTISELECT(descr)) descr->items[index].selected = state;
Huw.
I just realized that this would become a no-op in _SetSelection, and a redundant check in the other (because it's always multi-select there). So my question is, should I just get rid of the calls entirely in the _SetSelection function, and keep the code as-is (for now) in the _SelectItemRange? (the latter will change when multi-select nodata listbox gets implemented, so this helper is unneeded for now)
On Wed, Feb 13, 2019 at 01:11:35PM +0200, Gabriel Ivăncescu wrote:
On 2/13/19 12:00 PM, Huw Davies wrote:
On Tue, Feb 12, 2019 at 03:47:02PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
is_singlesel_NODATA is needed otherwise it fails the tests for multi-selection nodata, because it's not implemented yet.
dlls/comctl32/listbox.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index f99406a..706a0b8 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == LBS_NODATA; +} + static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; @@ -170,6 +175,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; } +static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) +{ + if (!is_singlesel_NODATA(descr)) descr->items[index].selected = state; +}
To mirror is_item_selected() I'd suggest only setting selected in the multiselect case, so this would become:
if (IS_MULTISELECT(descr)) descr->items[index].selected = state;
Huw.
I just realized that this would become a no-op in _SetSelection, and a redundant check in the other (because it's always multi-select there).
So my question is, should I just get rid of the calls entirely in the _SetSelection function.
Let's still call them.
and keep the code as-is (for now) in the _SelectItemRange? (the latter will change when multi-select nodata listbox gets implemented, so this helper is unneeded for now)
And use the helpers here. Note also you should use is_item_selected() instead of reading ->items[i].selected directly. Huw.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 706a0b8..f1cc1ea 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1659,19 +1659,12 @@ static LRESULT LISTBOX_InsertString( LB_DESCR *descr, INT index, LPCWSTR str ) */ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) { - /* save the item data before it gets freed by LB_RESETCONTENT */ - ULONG_PTR item_data = get_item_data(descr, index); - LPWSTR item_str = get_item_string(descr, index); - - if (!descr->nb_items) - SendMessageW( descr->self, LB_RESETCONTENT, 0, 0 ); - /* Note: Win 3.1 only sends DELETEITEM on owner-draw items, * while Win95 sends it for all items with user data. * It's probably better to send it too often than not * often enough, so this is what we do here. */ - if (IS_OWNERDRAW(descr) || item_data) + if (IS_OWNERDRAW(descr) || get_item_data(descr, index)) { DELETEITEMSTRUCT dis; UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); @@ -1680,10 +1673,10 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) dis.CtlID = id; dis.itemID = index; dis.hwndItem = descr->self; - dis.itemData = item_data; + dis.itemData = get_item_data(descr, index); SendMessageW( descr->owner, WM_DELETEITEM, id, (LPARAM)&dis ); } - HeapFree( GetProcessHeap(), 0, item_str ); + HeapFree( GetProcessHeap(), 0, get_item_string(descr, index) ); } @@ -1701,11 +1694,14 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) /* We need to invalidate the original rect instead of the updated one. */ LISTBOX_InvalidateItems( descr, index ); + if (descr->nb_items == 1) + { + SendMessageW(descr->self, LB_RESETCONTENT, 0, 0); + return LB_OKAY; + } descr->nb_items--; LISTBOX_DeleteItem( descr, index ); - if (!descr->nb_items) return LB_OKAY; - /* Remove the item */ item = &descr->items[index]; -- 2.20.1
participants (2)
-
Gabriel Ivăncescu -
Huw Davies