On 2/25/19 1:15 PM, Gabriel Ivăncescu wrote:
On 2/25/19 12:37 PM, Huw Davies wrote:
On Fri, Feb 22, 2019 at 02:00:57PM +0200, Gabriel Ivăncescu wrote:
Use a byte array to store selection state of items, since we don't need any other data for LBS_NODATA multi-selection listboxes. This improves memory usage dramatically for large lists, and performance boosts are nice too.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
For now, this should be good enough (byte array). Unfortunately, using other storage representations in the future (like selection ranges, or bit arrays) would probably need the avoided NODATA helpers to split the functions (in order to be easier maintainable), so for now this will suffice.
dlls/comctl32/listbox.c | 48 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b6024c3..5afdf5c 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -18,8 +18,6 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA *
- TODO:
- * - LBS_NODATA for multi-selection listboxes
*/ #include <string.h> @@ -127,6 +125,15 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +/* + Resize the item storage array if needed.
+ For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->items will always be NULL.
+ For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state, instead of the usual LB_ITEMDATA. +*/ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -137,7 +144,10 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1); if ((descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) != LBS_NODATA) { - items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA)); + size_t size = items_size; + if (!(descr->style & LBS_NODATA)) size *= sizeof(LB_ITEMDATA);
+ items = heap_realloc(descr->items, size); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -150,8 +160,7 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) if ((descr->style & LBS_NODATA) && descr->items && items_size > descr->nb_items) { - memset(&descr->items[descr->nb_items], 0, - (items_size - descr->nb_items) * sizeof(LB_ITEMDATA)); + memset((BYTE*)descr->items + descr->nb_items, 0, items_size
- descr->nb_items);
} return TRUE; } @@ -180,13 +189,21 @@ 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; + if (descr->style & LBS_NODATA) + return ((BYTE*)descr->items)[index]; + else + return descr->items[index].selected;
Make descr->items a union of the two types, that will get rid of these horrible casts. If you find you need to change a lot of code to make this work, then you need to add more calls to item getter/setter helpers.
} static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) { if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) - descr->items[index].selected = state; + { + if (descr->style & LBS_NODATA) + ((BYTE*)descr->items)[index] = state; + else + descr->items[index].selected = state; + } } static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) @@ -194,6 +211,15 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR LB_ITEMDATA *item; if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index;
+ if (index < descr->nb_items) + memmove(item + 1, item, descr->nb_items - index); + *item = FALSE; + return; + }
This is essentially the same code as what follows for the !LBS_NODATA case. If you added a helper to return the size of an item then these could be merged. This helper could also be used in resize_storage().
Huw.
Yeah, I wanted to avoid too many changes (also because I made these casts inside the helpers only), it's too bad we can't use anonymous unions in Wine's C89. I'll see what I can do with the union and helpers.
Ok, I have a question now: can I use a small helper that uses the cast instead of a union? Something like:
static inline BYTE *get_nodata_items(const LB_DESCR *descr) { return (BYTE*)descr->items; }
And use it where needed? It would require less changes to the code and, IMO, keep it cleaner (other than the one helper above). Such helpers are frequent in other parts of the code (mostly dealing with COM stuff).
On a side note, I'll also rewrite FindString since it's a mess and would still be a mess even if I replaced it with the helpers only.