[PATCH 1/5] comctl32/listbox: Resize the entire item array at once in SetCount
Huw Davies
huw at codeweavers.com
Mon Feb 11 05:12:48 CST 2019
On Mon, Feb 11, 2019 at 01:00:46PM +0200, Gabriel Ivăncescu wrote:
> 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).
Well ideally this won't change much this mutli-select no-data. If it does,
you'll probably need to add more helpers.
> 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?
Yes, that's what I meant. resize_storage() is allowed to be different for
the no-data case.
Huw.
More information about the wine-devel
mailing list