Re: [PATCH 4/5] comctl32/listbox: Implement LBS_NODATA for single-selection listboxes

[Huw Davies]

On Mon, Feb 11, 2019 at 01:09:14PM +0200, Gabriel Ivăncescu wrote:
> On 2/11/19 12:10 PM, Huw Davies wrote:
> > On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
> > > @@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
> > >       if (index == -1) index = descr->nb_items;
> > >       else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
> > > - if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> > > -
> > > - /* Insert the item structure */
> > > -
> > > - item = &descr->items[index];
> > > - if (index < descr->nb_items)
> > > -
> > >    RtlMoveMemory( item + 1, item,
> > > -
> > >                   (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> > > - item->str      = str;
> > > - item->data     = HAS_STRINGS(descr) ? 0 : data;
> > > - item->height   = 0;
> > > - item->selected = FALSE;
> > > - descr->nb_items++;
> > > -
> > > - /* Get item height */
> > > -
> > > - if (descr->style & LBS_OWNERDRAWVARIABLE)
> > > - if (is_singlesel_NODATA(descr))
> > >  {
> > > -
> > >    MEASUREITEMSTRUCT mis;
> > > -
> > >    UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
> > > -
> > >    descr->nb_items++;
> > > -
> > >    descr->items_size = max(descr->items_size, descr->nb_items);
> > > - }
> > > - else
> > > - {
> > > -
> > >    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> >
> > Again, resize_storage will handle the no_data case.  Then add a helper to
> > actually insert the item.
>
> But in the insert helper, I'd have to ignore everything if it's
> single-selection LBS_NODATA (because there's nothing to do), so there is no
> difference to currently except just moving most of the function into a
> helper (instead of indenting it).
>
> Is that what you had in mind?

Yes.
> > > @@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
> > >       LISTBOX_InvalidateItems( descr, index );
> > >       descr->nb_items--;
> > > - LISTBOX_DeleteItem( descr, index );
> >
> > LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT
> > as required.
>
> Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called with
> LBS_NODATA (unless you simply mean an early exit if so) nor send any
> messages at all.

It's an earlish exit after sending the LB_RESETCONTENT message if appropriate.
> > > @@ -1729,7 +1762,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
> > >   {
> > >       INT i;
> > > - for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
> > > - if (descr->items)
> > > -
> > >    for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
> >
> > LB_DeleteItems() will cope.
>
> I don't like this solution. That would be slower than needed for large lists
> (which LBS_NODATA is for) when resetting them, for an operation that should
> basically be instant and not dependent on the list's size. i.e. from O(1) to
> O(n)

We'll revisit it if performance turns out to be an issue.

Huw.