[PATCH v2 01/10] comctl32/listbox: Resize the entire item array at once in SetCount
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- count was changed to unsigned so that it passes the test in the next patch, apparently Windows treats it as unsigned. 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
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/tests/listbox.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 68e4073..58de614 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -1867,6 +1867,11 @@ static void test_set_count( void ) GetUpdateRect( listbox, &r, TRUE ); ok( !IsRectEmpty( &r ), "got empty rect\n"); + ret = SendMessageA( listbox, LB_SETCOUNT, -5, 0 ); + ok( ret == 0, "got %d\n", ret ); + ret = SendMessageA( listbox, LB_GETCOUNT, 0, 0 ); + ok( ret == -5, "got %d\n", ret ); + DestroyWindow( listbox ); for (i = 0; i < ARRAY_SIZE(styles); ++i) -- 2.20.1
Damn I just realized this after I sent it that it may fail because we don't have LBS_NODATA implemented yet, and I shouldn't have re-ordered it. My bad :-/ If nothing else is wrong with the patch series, can you please just move this patch to the end of the series? On 2/11/19 7:03 PM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/tests/listbox.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 68e4073..58de614 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -1867,6 +1867,11 @@ static void test_set_count( void ) GetUpdateRect( listbox, &r, TRUE ); ok( !IsRectEmpty( &r ), "got empty rect\n");
+ ret = SendMessageA( listbox, LB_SETCOUNT, -5, 0 ); + ok( ret == 0, "got %d\n", ret ); + ret = SendMessageA( listbox, LB_GETCOUNT, 0, 0 ); + ok( ret == -5, "got %d\n", ret ); + DestroyWindow( listbox );
for (i = 0; i < ARRAY_SIZE(styles); ++i)
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 58de614..de73ad2 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 ( @@ -2488,6 +2617,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 | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c84d317..ec6a169 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -160,6 +160,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; } +static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) +{ + return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -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, -- 2.20.1
On Mon, Feb 11, 2019 at 07:03:08PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c84d317..ec6a169 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -160,6 +160,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) +{ + return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -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,
This helper should now be used throughout the code, for example LISTBOX_GetText(), _FindStringPos(), _DeleteItem(), handler for LB_GETITEMDATA and ideally _FindString() (but I'll let you off _FindString() since that will require a bit of rewriting). You'll see that doing this will further reduce the _NODATA special cases. The same then goes for the following patch which introduces get_item_string(). Huw.
On 2/12/19 10:59 AM, Huw Davies wrote:
On Mon, Feb 11, 2019 at 07:03:08PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c84d317..ec6a169 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -160,6 +160,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) +{ + return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -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,
This helper should now be used throughout the code, for example LISTBOX_GetText(), _FindStringPos(), _DeleteItem(), handler for LB_GETITEMDATA and ideally _FindString() (but I'll let you off _FindString() since that will require a bit of rewriting).
You'll see that doing this will further reduce the _NODATA special cases.
The same then goes for the following patch which introduces get_item_string().
Huw.
I think FindStringPos also fails with NODATA, should I still use it there or leave it as it is to keep the patch smaller?
On Tue, Feb 12, 2019 at 12:51:28PM +0200, Gabriel Ivăncescu wrote:
On 2/12/19 10:59 AM, Huw Davies wrote:
On Mon, Feb 11, 2019 at 07:03:08PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c84d317..ec6a169 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -160,6 +160,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; } +static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) +{ + return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -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,
This helper should now be used throughout the code, for example LISTBOX_GetText(), _FindStringPos(), _DeleteItem(), handler for LB_GETITEMDATA and ideally _FindString() (but I'll let you off _FindString() since that will require a bit of rewriting).
You'll see that doing this will further reduce the _NODATA special cases.
The same then goes for the following patch which introduces get_item_string().
Huw.
I think FindStringPos also fails with NODATA, should I still use it there or leave it as it is to keep the patch smaller?
Ideally every read of ->data/->str would be replaced by the appropriate helper. I realise that _FineStringPos will require a little bit of work, but, since you ask, I think it's worth it. This should also answer your question about DeleteItem. Huw.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index ec6a169..48b5b78 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -165,6 +165,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 (descr->style & LBS_NODATA) ? NULL : descr->items[index].str; +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -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 ); -- 2.20.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 48b5b78..633e7c6 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -160,6 +160,18 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; } +static void set_item_selected( LB_DESCR *descr, UINT index ) +{ + INT oldsel = descr->selected_item; + + if (!(descr->style & LBS_NODATA)) + { + if (oldsel != -1) descr->items[oldsel].selected = FALSE; + if (index != -1) descr->items[index].selected = TRUE; + } + descr->selected_item = index; +} + static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; @@ -1476,10 +1488,8 @@ 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; + set_item_selected(descr, index); if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT ); - descr->selected_item = index; 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 Mon, Feb 11, 2019 at 07:03:10PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 48b5b78..633e7c6 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -160,6 +160,18 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static void set_item_selected( LB_DESCR *descr, UINT index ) +{ + INT oldsel = descr->selected_item; + + if (!(descr->style & LBS_NODATA)) + { + if (oldsel != -1) descr->items[oldsel].selected = FALSE; + if (index != -1) descr->items[index].selected = TRUE; + } + descr->selected_item = index; +}
I had more in mind: static void set_item_selected_state( LB_DESCR *descr, UINT index, BOOL state ) { if (!(descr->style & LBS_NODATA)) descr->items[index].selected = state; } The idea is that the helpers are simple getters/setters for the item 'object'. It's in these helpers that you'll hide any storage differences.
+ static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; @@ -1476,10 +1488,8 @@ 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; + set_item_selected(descr, index); if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT ); - descr->selected_item = index; if (index != -1) LISTBOX_RepaintItem( descr, index, ODA_SELECT ); if (send_notify && descr->nb_items) SEND_NOTIFICATION( descr, (index != -1) ? LBN_SELCHANGE : LBN_SELCANCEL );
So it would get called twice in the above code, and the above would still set ->selected_item itself. This should also be used in _SelectItemRange() which will help when you get to nodata multi-select. Huw.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Ok, this is how I ended up making the code simpler later. The reason being that it really makes no sense at all to send LB_RESETCONTENT from within DeleteItem: that function is called *from* ResetContent itself and it only happened to work since it updated nb_items afterward. It was too fragile to begin with. Note that LB_RESETCONTENT already calls DeleteItem, so we can simply re-use that, and IMO this makes much more sense and will simplify the code in the last patch. dlls/comctl32/listbox.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 633e7c6..0f74e80 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1660,13 +1660,9 @@ 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; LPWSTR item_str = descr->items[index].str; - 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 @@ -1703,11 +1699,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
On Mon, Feb 11, 2019 at 07:03:11PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
Ok, this is how I ended up making the code simpler later. The reason being that it really makes no sense at all to send LB_RESETCONTENT from within DeleteItem: that function is called *from* ResetContent itself and it only happened to work since it updated nb_items afterward. It was too fragile to begin with.
Note that LB_RESETCONTENT already calls DeleteItem, so we can simply re-use that, and IMO this makes much more sense and will simplify the code in the last patch.
Good, this is clearly how it should be done.
dlls/comctl32/listbox.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 633e7c6..0f74e80 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1660,13 +1660,9 @@ 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; LPWSTR item_str = descr->items[index].str;
As a nit-pick, there's no need to have these temp variables anymore, you can just call the getter helpers as needed. (Yes, we'll call get_item_data() twice, but it'll almost certainly get inlined anyway). Huw.
On 2/12/19 11:32 AM, Huw Davies wrote:
On Mon, Feb 11, 2019 at 07:03:11PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
Ok, this is how I ended up making the code simpler later. The reason being that it really makes no sense at all to send LB_RESETCONTENT from within DeleteItem: that function is called *from* ResetContent itself and it only happened to work since it updated nb_items afterward. It was too fragile to begin with.
Note that LB_RESETCONTENT already calls DeleteItem, so we can simply re-use that, and IMO this makes much more sense and will simplify the code in the last patch.
Good, this is clearly how it should be done.
dlls/comctl32/listbox.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 633e7c6..0f74e80 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1660,13 +1660,9 @@ 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; LPWSTR item_str = descr->items[index].str;
As a nit-pick, there's no need to have these temp variables anymore, you can just call the getter helpers as needed. (Yes, we'll call get_item_data() twice, but it'll almost certainly get inlined anyway).
Huw.
I have one question about this. DeleteItem shouldn't do anything for LBS_NODATA (not send any messages, etc). Should I still use the helpers and change the code, or leave it as it is (perhaps just getting rid of temporaries and using directly)? We don't have tests for this yet in the Wine tree, because of multi-select nodata listboxes not being implemented, I wanted to send the tests after that because then I won't have to special-case "todo" wines for multi-select cases, etc.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This is a no-op patch, but note how nb_items is incremented before and this is taken care of in the helper function. This is for two reasons: 1) This simplifies the next patch (only adds a single line to insert_item). 2) This matches what RemoveItem does, so it's more consistent. dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 0f74e80..1f56c99 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -182,6 +182,38 @@ static WCHAR *get_item_string( const LB_DESCR *descr, UINT index ) return (descr->style & LBS_NODATA) ? NULL : descr->items[index].str; } +static void insert_item(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) +{ + LB_ITEMDATA *item; + + item = &descr->items[index]; + + if (index < descr->nb_items - 1) + RtlMoveMemory(item + 1, item, + (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA)); + item->str = str; + item->data = HAS_STRINGS(descr) ? 0 : data; + item->height = 0; + item->selected = FALSE; + + /* Get item height */ + if (descr->style & LBS_OWNERDRAWVARIABLE) + { + MEASUREITEMSTRUCT mis; + UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + + mis.CtlType = ODT_LISTBOX; + mis.CtlID = id; + mis.itemID = index; + mis.itemData = data; + mis.itemHeight = descr->item_height; + SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); + item->height = mis.itemHeight ? mis.itemHeight : 1; + TRACE("[%p]: measure item %d (%s) = %d\n", + descr->self, index, str ? debugstr_w(str) : "", item->height ); + } +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -1556,42 +1588,14 @@ static void LISTBOX_MoveCaret( LB_DESCR *descr, INT index, BOOL fully_visible ) static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index, LPWSTR str, ULONG_PTR data ) { - LB_ITEMDATA *item; INT oldfocus = descr->focus_item; if (index == -1) index = descr->nb_items; else if ((index < 0) || (index > descr->nb_items)) return LB_ERR; if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR; - /* Insert the item structure */ - - item = &descr->items[index]; - if (index < descr->nb_items) - RtlMoveMemory( item + 1, item, - (descr->nb_items - index) * sizeof(LB_ITEMDATA) ); - item->str = str; - item->data = HAS_STRINGS(descr) ? 0 : data; - item->height = 0; - item->selected = FALSE; descr->nb_items++; - - /* Get item height */ - - if (descr->style & LBS_OWNERDRAWVARIABLE) - { - MEASUREITEMSTRUCT mis; - UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); - - mis.CtlType = ODT_LISTBOX; - mis.CtlID = id; - mis.itemID = index; - mis.itemData = data; - mis.itemHeight = descr->item_height; - SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); - item->height = mis.itemHeight ? mis.itemHeight : 1; - TRACE("[%p]: measure item %d (%s) = %d\n", - descr->self, index, str ? debugstr_w(str) : "", item->height ); - } + insert_item(descr, index, str, data); /* Repaint the items */ -- 2.20.1
On Mon, Feb 11, 2019 at 07:03:12PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
This is a no-op patch, but note how nb_items is incremented before and this is taken care of in the helper function. This is for two reasons:
1) This simplifies the next patch (only adds a single line to insert_item). 2) This matches what RemoveItem does, so it's more consistent.
Let's get patches 1,3-8 in first. This series has now got rather long to review in one go. Huw.
The LBS_NODATA style's purpose is to drastically improve performance and memory usage on very large lists, since they should function as virtual lists. Thus, don't store any data for single-selection listboxes (descr->items always NULL). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- remove_item is part of this patch because it would otherwise have to be rewritten due to indentation, amounting to same amount of changes, and it's just moving the small part into a helper. The patch should be small enough now, hopefully. dlls/comctl32/listbox.c | 59 +++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 1f56c99..1b8f874 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -19,7 +19,7 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * * TODO: - * - LBS_NODATA + * - LBS_NODATA for multi-selection listboxes */ #include <string.h> @@ -126,6 +126,17 @@ typedef enum static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ); + +static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == LBS_NODATA; +} + +static BOOL is_multisel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & LBS_NODATA) && (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)); +} static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { @@ -135,17 +146,20 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) items_size + LB_ARRAY_GRANULARITY * 2 < descr->items_size) { items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1); - items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA)); - if (!items) + if (!is_singlesel_NODATA(descr)) { - SEND_NOTIFICATION(descr, LBN_ERRSPACE); - return FALSE; + items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA)); + if (!items) + { + SEND_NOTIFICATION(descr, LBN_ERRSPACE); + return FALSE; + } + descr->items = items; } descr->items_size = items_size; - descr->items = items; } - if ((descr->style & LBS_NODATA) && items_size > descr->nb_items) + if (is_multisel_NODATA(descr) && items_size > descr->nb_items) { memset(&descr->items[descr->nb_items], 0, (items_size - descr->nb_items) * sizeof(LB_ITEMDATA)); @@ -186,6 +200,7 @@ static void insert_item(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) { LB_ITEMDATA *item; + if (is_singlesel_NODATA(descr)) return; item = &descr->items[index]; if (index < descr->nb_items - 1) @@ -214,6 +229,20 @@ static void insert_item(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) } } +static void remove_item(LB_DESCR *descr, INT index) +{ + if (!is_singlesel_NODATA(descr)) + { + LB_ITEMDATA *item = &descr->items[index]; + + LISTBOX_DeleteItem(descr, index); + if (index < descr->nb_items) + RtlMoveMemory(item, item + 1, + (descr->nb_items - index) * sizeof(LB_ITEMDATA)); + } + descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1); +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -1696,8 +1725,6 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) */ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) { - LB_ITEMDATA *item; - if ((index < 0) || (index >= descr->nb_items)) return LB_ERR; /* We need to invalidate the original rect instead of the updated one. */ @@ -1709,16 +1736,7 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) return LB_OKAY; } descr->nb_items--; - LISTBOX_DeleteItem( descr, index ); - - /* Remove the item */ - - item = &descr->items[index]; - if (index < descr->nb_items) - RtlMoveMemory( item, item + 1, - (descr->nb_items - index) * sizeof(LB_ITEMDATA) ); - if (descr->anchor_item == descr->nb_items) descr->anchor_item--; - + remove_item(descr, index); resize_storage(descr, descr->nb_items); /* Repaint the items */ @@ -1758,7 +1776,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) { INT i; - for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!(descr->style & LBS_NODATA)) + for(i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i); HeapFree( GetProcessHeap(), 0, descr->items ); descr->nb_items = 0; descr->top_item = 0; -- 2.20.1
participants (2)
-
Gabriel Ivăncescu -
Huw Davies