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

[Huw Davies]

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.