[PATCH 1/5] comctl32/listbox: Resize the entire item array at once in SetCount

[Gabriel Ivăncescu]

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