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

[Gabriel Ivăncescu]

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

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.