Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
This has been carefully done so that it matches the previous behavior when LBS_NODATA was used, since SetCount only works with it (so that means it also implies LBS_OWNERDRAWFIXED and so on).
count was changed to unsigned so that it passes the last test in the patch series, apparently Windows treats it as unsigned.
dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c92c2a2..787fc88 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1742,24 +1742,47 @@ 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) + if (count > orig_num) { - while (count > descr->nb_items) - if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0) - return ret; + if (!resize_storage(descr, count)) + return LB_ERRSPACE; + memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA)); + descr->nb_items = count; + + LISTBOX_UpdateScroll(descr); + LISTBOX_InvalidateItems(descr, orig_num); + + /* If listbox was empty, set focus to the first item */ + if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE); } - else if (count < descr->nb_items) + else if (count < orig_num) { - while (count < descr->nb_items) - if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0) - return ret; + LISTBOX_InvalidateItems(descr, count); + descr->nb_items = count; + + if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0); + else + { + descr->anchor_item = min(descr->anchor_item, count - 1); + + resize_storage(descr, count); + if (descr->selected_item >= count) + descr->selected_item = -1; + + LISTBOX_UpdateScroll(descr); + + /* 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); + } }
InvalidateRect( descr->self, NULL, TRUE );
Needed for LBS_NODATA.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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 787fc88..d115617 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -149,6 +149,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; }
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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();
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@gmail.com ---
Most of the changes are just indentation in InsertItem/RemoveItem.
dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++-------------- 1 file changed, 90 insertions(+), 50 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index d115617..fd3bfa6 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> @@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & LBS_NODATA) && + !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)); +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, GetClientRect(descr->self, &r); hrgn = set_control_clipping( hdc, &r );
+ if (is_singlesel_NODATA(descr)) item = NULL; dis.CtlType = ODT_LISTBOX; dis.CtlID = GetWindowLongPtrW( descr->self, GWLP_ID ); dis.hwndItem = descr->self; @@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items ) { UINT new_size = descr->nb_items + nb_items;
- if (new_size > descr->items_size && !resize_storage(descr, new_size)) - return LB_ERRSPACE; + /* Windows keeps track of (unaligned) reserved space + for LBS_NODATA, despite the fact it is useless */ + if (new_size > descr->items_size) + { + if (is_singlesel_NODATA(descr)) + descr->items_size = new_size; + else if (!resize_storage(descr, new_size)) + return LB_ERRSPACE; + } return descr->items_size; }
@@ -1460,10 +1474,13 @@ 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 (!(descr->style & LBS_NODATA)) + { + if (oldsel != -1) descr->items[oldsel].selected = FALSE; + if (index != -1) descr->items[index].selected = 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 ); @@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
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) + if (is_singlesel_NODATA(descr)) { - MEASUREITEMSTRUCT mis; - UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + descr->nb_items++; + descr->items_size = max(descr->items_size, descr->nb_items); + } + else + { + 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 ); + 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 ); + } }
/* Repaint the items */ @@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) LISTBOX_InvalidateItems( descr, index );
descr->nb_items--; - LISTBOX_DeleteItem( descr, index ); - - if (!descr->nb_items) return LB_OKAY; + if (is_singlesel_NODATA(descr)) + { + if (!descr->nb_items) + { + SendMessageW(descr->self, LB_RESETCONTENT, 0, 0); + return LB_OKAY; + } + } + else + { + LISTBOX_DeleteItem( descr, index );
- /* Remove the item */ + if (!descr->nb_items) return LB_OKAY;
- 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 the item */ + item = &descr->items[index]; + if (index < descr->nb_items) + RtlMoveMemory( item, item + 1, + (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
- resize_storage(descr, descr->nb_items); + resize_storage(descr, descr->nb_items); + } + descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
/* Repaint the items */
@@ -1729,7 +1762,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->items) + 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; @@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
if (count > orig_num) { - if (!resize_storage(descr, count)) - return LB_ERRSPACE; - memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA)); + if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) + { + if (!resize_storage(descr, count)) + return LB_ERRSPACE; + memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA)); + } + else + descr->items_size = max(descr->items_size, count); descr->nb_items = count;
LISTBOX_UpdateScroll(descr); @@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count ) { descr->anchor_item = min(descr->anchor_item, count - 1);
- resize_storage(descr, count); - if (descr->selected_item >= count) + if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) + resize_storage(descr, count); + else if (descr->selected_item >= descr->nb_items) descr->selected_item = -1;
LISTBOX_UpdateScroll(descr);
On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
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@gmail.com
Most of the changes are just indentation in InsertItem/RemoveItem.
dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++-------------- 1 file changed, 90 insertions(+), 50 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index d115617..fd3bfa6 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> @@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{
- return (descr->style & LBS_NODATA) &&
!(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));+}
There are way too many is_singlesel_NODATA() special cases in this patch. Instead of that, we should add more helpers like is_item_selected(). I've given some examples below.
/***********************************************************************
LISTBOX_GetCurrentPageSize@@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, GetClientRect(descr->self, &r); hrgn = set_control_clipping( hdc, &r );
if (is_singlesel_NODATA(descr)) item = NULL;
This is used to cover item->data (and item->str in the TRACE). So introduce a get_item_data() and get_item_string() helper that returns NULL in the no_data case. Patches to add those helpers obviously come before this patch.
dis.CtlType = ODT_LISTBOX; dis.CtlID = GetWindowLongPtrW( descr->self, GWLP_ID ); dis.hwndItem = descr->self;@@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items ) { UINT new_size = descr->nb_items + nb_items;
- if (new_size > descr->items_size && !resize_storage(descr, new_size))
return LB_ERRSPACE;
- /* Windows keeps track of (unaligned) reserved space
for LBS_NODATA, despite the fact it is useless */- if (new_size > descr->items_size)
- {
if (is_singlesel_NODATA(descr))descr->items_size = new_size;else if (!resize_storage(descr, new_size))return LB_ERRSPACE;- }
Have resize_storage() handle no_data, it'll fix up ->items_size too, so there should be no need to change this bit of code.
return descr->items_size;}
@@ -1460,10 +1474,13 @@ 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 (!(descr->style & LBS_NODATA)){if (oldsel != -1) descr->items[oldsel].selected = FALSE;if (index != -1) descr->items[index].selected = TRUE;}
Add a helper to set an item's selection state.
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 );@@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
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)
- if (is_singlesel_NODATA(descr)) {
MEASUREITEMSTRUCT mis;UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
descr->nb_items++;descr->items_size = max(descr->items_size, descr->nb_items);- }
- else
- {
if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
Again, resize_storage will handle the no_data case. Then add a helper to actually insert the item.
/* 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 );
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 );}}
/* Repaint the items */
@@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) LISTBOX_InvalidateItems( descr, index );
descr->nb_items--;
- LISTBOX_DeleteItem( descr, index );
LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT as required.
- if (!descr->nb_items) return LB_OKAY;
- if (is_singlesel_NODATA(descr))
- {
if (!descr->nb_items){SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);return LB_OKAY;}- }
- else
- {
LISTBOX_DeleteItem( descr, index );
- /* Remove the item */
if (!descr->nb_items) return LB_OKAY;
- 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 the item */item = &descr->items[index];if (index < descr->nb_items)RtlMoveMemory( item, item + 1,(descr->nb_items - index) * sizeof(LB_ITEMDATA) );
- resize_storage(descr, descr->nb_items);
resize_storage(descr, descr->nb_items);}
descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
/* Repaint the items */
@@ -1729,7 +1762,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->items)
for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
LB_DeleteItems() will cope.
HeapFree( GetProcessHeap(), 0, descr->items ); descr->nb_items = 0; descr->top_item = 0;@@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
if (count > orig_num) {
if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)){if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));}elsedescr->items_size = max(descr->items_size, count); descr->nb_items = count; LISTBOX_UpdateScroll(descr);@@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count ) { descr->anchor_item = min(descr->anchor_item, count - 1);
resize_storage(descr, count);if (descr->selected_item >= count)
if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))resize_storage(descr, count);else if (descr->selected_item >= descr->nb_items) descr->selected_item = -1; LISTBOX_UpdateScroll(descr);
And again resize_storage() will handle this.
Huw.
On 2/11/19 12:10 PM, Huw Davies wrote:
On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
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@gmail.com
Most of the changes are just indentation in InsertItem/RemoveItem.
dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++-------------- 1 file changed, 90 insertions(+), 50 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index d115617..fd3bfa6 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>
@@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) return descr->items[index].selected; }
+static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{
- return (descr->style & LBS_NODATA) &&
!(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));+}
There are way too many is_singlesel_NODATA() special cases in this patch. Instead of that, we should add more helpers like is_item_selected(). I've given some examples below.
/***********************************************************************
LISTBOX_GetCurrentPageSize@@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, GetClientRect(descr->self, &r); hrgn = set_control_clipping( hdc, &r );
if (is_singlesel_NODATA(descr)) item = NULL;This is used to cover item->data (and item->str in the TRACE). So introduce a get_item_data() and get_item_string() helper that returns NULL in the no_data case. Patches to add those helpers obviously come before this patch.
Okay will do that.
dis.CtlType = ODT_LISTBOX; dis.CtlID = GetWindowLongPtrW( descr->self, GWLP_ID ); dis.hwndItem = descr->self;@@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items ) { UINT new_size = descr->nb_items + nb_items;
- if (new_size > descr->items_size && !resize_storage(descr, new_size))
return LB_ERRSPACE;
- /* Windows keeps track of (unaligned) reserved space
for LBS_NODATA, despite the fact it is useless */- if (new_size > descr->items_size)
- {
if (is_singlesel_NODATA(descr))descr->items_size = new_size;else if (!resize_storage(descr, new_size))return LB_ERRSPACE;- }
Have resize_storage() handle no_data, it'll fix up ->items_size too, so there should be no need to change this bit of code.
return descr->items_size;}
@@ -1460,10 +1474,13 @@ 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 (!(descr->style & LBS_NODATA)){if (oldsel != -1) descr->items[oldsel].selected = FALSE;if (index != -1) descr->items[index].selected = TRUE;}Add a helper to set an item's selection state.
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 );@@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
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)
- if (is_singlesel_NODATA(descr)) {
MEASUREITEMSTRUCT mis;UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
descr->nb_items++;descr->items_size = max(descr->items_size, descr->nb_items);- }
- else
- {
if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;Again, resize_storage will handle the no_data case. Then add a helper to actually insert the item.
But in the insert helper, I'd have to ignore everything if it's single-selection LBS_NODATA (because there's nothing to do), so there is no difference to currently except just moving most of the function into a helper (instead of indenting it).
Is that what you had in mind?
/* 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 );
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 );} } /* Repaint the items */@@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) LISTBOX_InvalidateItems( descr, index );
descr->nb_items--;
- LISTBOX_DeleteItem( descr, index );
LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT as required.
Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called with LBS_NODATA (unless you simply mean an early exit if so) nor send any messages at all.
- if (!descr->nb_items) return LB_OKAY;
- if (is_singlesel_NODATA(descr))
- {
if (!descr->nb_items){SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);return LB_OKAY;}- }
- else
- {
LISTBOX_DeleteItem( descr, index );
- /* Remove the item */
if (!descr->nb_items) return LB_OKAY;
- 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 the item */item = &descr->items[index];if (index < descr->nb_items)RtlMoveMemory( item, item + 1,(descr->nb_items - index) * sizeof(LB_ITEMDATA) );
- resize_storage(descr, descr->nb_items);
resize_storage(descr, descr->nb_items);}
descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
/* Repaint the items */
@@ -1729,7 +1762,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->items)
for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);LB_DeleteItems() will cope.
I don't like this solution. That would be slower than needed for large lists (which LBS_NODATA is for) when resetting them, for an operation that should basically be instant and not dependent on the list's size. i.e. from O(1) to O(n)
HeapFree( GetProcessHeap(), 0, descr->items ); descr->nb_items = 0; descr->top_item = 0;@@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
if (count > orig_num) {
if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)){if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));}elsedescr->items_size = max(descr->items_size, count); descr->nb_items = count; LISTBOX_UpdateScroll(descr);@@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count ) { descr->anchor_item = min(descr->anchor_item, count - 1);
resize_storage(descr, count);if (descr->selected_item >= count)
if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))resize_storage(descr, count);else if (descr->selected_item >= descr->nb_items) descr->selected_item = -1; LISTBOX_UpdateScroll(descr);And again resize_storage() will handle this.
Huw.
On Mon, Feb 11, 2019 at 01:09:14PM +0200, Gabriel Ivăncescu wrote:
On 2/11/19 12:10 PM, Huw Davies wrote:
On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
@@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index, 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)
- if (is_singlesel_NODATA(descr)) {
MEASUREITEMSTRUCT mis;UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
descr->nb_items++;descr->items_size = max(descr->items_size, descr->nb_items);- }
- else
- {
if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;Again, resize_storage will handle the no_data case. Then add a helper to actually insert the item.
But in the insert helper, I'd have to ignore everything if it's single-selection LBS_NODATA (because there's nothing to do), so there is no difference to currently except just moving most of the function into a helper (instead of indenting it).
Is that what you had in mind?
Yes.
@@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) LISTBOX_InvalidateItems( descr, index ); descr->nb_items--;
- LISTBOX_DeleteItem( descr, index );
LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT as required.
Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called with LBS_NODATA (unless you simply mean an early exit if so) nor send any messages at all.
It's an earlish exit after sending the LB_RESETCONTENT message if appropriate.
@@ -1729,7 +1762,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->items)
for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);LB_DeleteItems() will cope.
I don't like this solution. That would be slower than needed for large lists (which LBS_NODATA is for) when resetting them, for an operation that should basically be instant and not dependent on the list's size. i.e. from O(1) to O(n)
We'll revisit it if performance turns out to be an issue.
Huw.
On 2/11/19 1:22 PM, Huw Davies wrote:
On Mon, Feb 11, 2019 at 01:09:14PM +0200, Gabriel Ivăncescu wrote:
On 2/11/19 12:10 PM, Huw Davies wrote:
On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
@@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index, 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)
- if (is_singlesel_NODATA(descr)) {
MEASUREITEMSTRUCT mis;UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
descr->nb_items++;descr->items_size = max(descr->items_size, descr->nb_items);- }
- else
- {
if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;Again, resize_storage will handle the no_data case. Then add a helper to actually insert the item.
But in the insert helper, I'd have to ignore everything if it's single-selection LBS_NODATA (because there's nothing to do), so there is no difference to currently except just moving most of the function into a helper (instead of indenting it).
Is that what you had in mind?
Yes.
@@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) LISTBOX_InvalidateItems( descr, index ); descr->nb_items--;
- LISTBOX_DeleteItem( descr, index );
LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT as required.
Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called with LBS_NODATA (unless you simply mean an early exit if so) nor send any messages at all.
It's an earlish exit after sending the LB_RESETCONTENT message if appropriate.
@@ -1729,7 +1762,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->items)
for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);LB_DeleteItems() will cope.
I don't like this solution. That would be slower than needed for large lists (which LBS_NODATA is for) when resetting them, for an operation that should basically be instant and not dependent on the list's size. i.e. from O(1) to O(n)
We'll revisit it if performance turns out to be an issue.
Huw.
Well, I find it awkward that sending SetCount to zero first would be far faster than LB_RESETCONTENT directly (that's what this will turn out to be), or that "growing" the list is instant while ResetContent is not. And it's just one line of code, I think when reducing O(n) to O(1) it should be worth it, given the point of LBS_NODATA.
(also, a variant of that line of code will also help with multi-selection listboxes later, which can also be instant)
Ok, I have a question on how to proceed now, after looking through and finishing the rework.
DeleteItem saves the item data & string at the beginning, *before* ResetContent. It has to.
If I special-case LBS_NODATA inside of it, I'll have to do it *twice* (one before saving, one after ResetContent), or duplicate ResetContent, which is pretty ugly.
So, with this in mind, should I still special case it inside of DeleteItem instead of RemoveItem? (or a helper inside RemoveItem, as per InsertItem).
This will also fix the O(n) issue. And in the end there will be the same amount of checks in the code. I think this is how I should proceed but I want to be sure before I send it in later if you don't want it.
Summary: there will be one check in RemoveItem's helper (like InsertItem) and one in ResetContent. Instead of having two checks in DeleteItem (and the O(n) thing).
Thoughts?
On Mon, Feb 11, 2019 at 05:29:58PM +0200, Gabriel Ivăncescu wrote:
Ok, I have a question on how to proceed now, after looking through and finishing the rework.
DeleteItem saves the item data & string at the beginning, *before* ResetContent. It has to.
If I special-case LBS_NODATA inside of it, I'll have to do it *twice* (one before saving, one after ResetContent), or duplicate ResetContent, which is pretty ugly.
So, with this in mind, should I still special case it inside of DeleteItem instead of RemoveItem? (or a helper inside RemoveItem, as per InsertItem).
This will also fix the O(n) issue. And in the end there will be the same amount of checks in the code. I think this is how I should proceed but I want to be sure before I send it in later if you don't want it.
Summary: there will be one check in RemoveItem's helper (like InsertItem) and one in ResetContent. Instead of having two checks in DeleteItem (and the O(n) thing).
It's going to be easier to review some actual code. So do it in the way you think best follows the spirit of what I was asking and we can take it from there.
Huw.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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 0cdcdea..de73ad2 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -1996,6 +1996,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)
On Fri, Feb 08, 2019 at 02:41:42PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
This has been carefully done so that it matches the previous behavior when LBS_NODATA was used, since SetCount only works with it (so that means it also implies LBS_OWNERDRAWFIXED and so on).
count was changed to unsigned so that it passes the last test in the patch series, apparently Windows treats it as unsigned.
dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c92c2a2..787fc88 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1742,24 +1742,47 @@ 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)
- if (count > orig_num) {
while (count > descr->nb_items)if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)return ret;
if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));descr->nb_items = count;LISTBOX_UpdateScroll(descr);LISTBOX_InvalidateItems(descr, orig_num);/* If listbox was empty, set focus to the first item */if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE);
- else if (count < descr->nb_items)
- else if (count < orig_num) {
while (count < descr->nb_items)if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)return ret;
LISTBOX_InvalidateItems(descr, count);descr->nb_items = count;if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);else{descr->anchor_item = min(descr->anchor_item, count - 1);resize_storage(descr, count);if (descr->selected_item >= count)descr->selected_item = -1;LISTBOX_UpdateScroll(descr);/* 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);}}
InvalidateRect( descr->self, NULL, TRUE );
This could be a lot simpler. For a start, we don't need the calls to LB_InvalidateItems() since we have the InvalidateRect() call at the end. Also, much of the code in the two cases (shrink or grow) is the same and so could be merged. (I realise you've basically inlined the code that was called before, but we can do better than that).
It probably also makes sense to have resize_storage() perform the zero-init.
Huw.
On 2/11/19 11:54 AM, Huw Davies wrote:
On Fri, Feb 08, 2019 at 02:41:42PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
This has been carefully done so that it matches the previous behavior when LBS_NODATA was used, since SetCount only works with it (so that means it also implies LBS_OWNERDRAWFIXED and so on).
count was changed to unsigned so that it passes the last test in the patch series, apparently Windows treats it as unsigned.
dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c92c2a2..787fc88 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1742,24 +1742,47 @@ 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)
- if (count > orig_num) {
while (count > descr->nb_items)if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)return ret;
if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));descr->nb_items = count;LISTBOX_UpdateScroll(descr);LISTBOX_InvalidateItems(descr, orig_num);/* If listbox was empty, set focus to the first item */if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE); }
- else if (count < descr->nb_items)
- else if (count < orig_num) {
while (count < descr->nb_items)if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)return ret;
LISTBOX_InvalidateItems(descr, count);descr->nb_items = count;if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);else{descr->anchor_item = min(descr->anchor_item, count - 1);resize_storage(descr, count);if (descr->selected_item >= count)descr->selected_item = -1;LISTBOX_UpdateScroll(descr);/* 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);} } InvalidateRect( descr->self, NULL, TRUE );This could be a lot simpler. For a start, we don't need the calls to LB_InvalidateItems() since we have the InvalidateRect() call at the end. Also, much of the code in the two cases (shrink or grow) is the same and so could be merged. (I realise you've basically inlined the code that was called before, but we can do better than that).
It probably also makes sense to have resize_storage() perform the zero-init.
Huw.
I'll see what I can do about minimizing the code, but I'm not sure if it will complicate things later with multi-selection listbox (I guess such changes should be done later, if needed).
But, the zero-init would be redundant for all other listboxes though, so it will slow down inserting items for "normal" listboxes. Should I do it just for LBS_NODATA in resize_storage?
On Mon, Feb 11, 2019 at 01:00:46PM +0200, Gabriel Ivăncescu wrote:
On 2/11/19 11:54 AM, Huw Davies wrote:
On Fri, Feb 08, 2019 at 02:41:42PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
This has been carefully done so that it matches the previous behavior when LBS_NODATA was used, since SetCount only works with it (so that means it also implies LBS_OWNERDRAWFIXED and so on).
count was changed to unsigned so that it passes the last test in the patch series, apparently Windows treats it as unsigned.
dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index c92c2a2..787fc88 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1742,24 +1742,47 @@ 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)
- if (count > orig_num) {
while (count > descr->nb_items)if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)return ret;
if (!resize_storage(descr, count))return LB_ERRSPACE;memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));descr->nb_items = count;LISTBOX_UpdateScroll(descr);LISTBOX_InvalidateItems(descr, orig_num);/* If listbox was empty, set focus to the first item */if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE); }
- else if (count < descr->nb_items)
- else if (count < orig_num) {
while (count < descr->nb_items)if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)return ret;
LISTBOX_InvalidateItems(descr, count);descr->nb_items = count;if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);else{descr->anchor_item = min(descr->anchor_item, count - 1);resize_storage(descr, count);if (descr->selected_item >= count)descr->selected_item = -1;LISTBOX_UpdateScroll(descr);/* 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);} } InvalidateRect( descr->self, NULL, TRUE );This could be a lot simpler. For a start, we don't need the calls to LB_InvalidateItems() since we have the InvalidateRect() call at the end. Also, much of the code in the two cases (shrink or grow) is the same and so could be merged. (I realise you've basically inlined the code that was called before, but we can do better than that).
It probably also makes sense to have resize_storage() perform the zero-init.
Huw.
I'll see what I can do about minimizing the code, but I'm not sure if it will complicate things later with multi-selection listbox (I guess such changes should be done later, if needed).
Well ideally this won't change much this mutli-select no-data. If it does, you'll probably need to add more helpers.
But, the zero-init would be redundant for all other listboxes though, so it will slow down inserting items for "normal" listboxes. Should I do it just for LBS_NODATA in resize_storage?
Yes, that's what I meant. resize_storage() is allowed to be different for the no-data case.
Huw.
-
Gabriel Ivăncescu -
Huw Davies