[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Gabriel Ivăncescu]

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 at 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.