Re: [PATCH v2 1/2] comctl32/listbox: Implement LBS_NODATA for single-selection listboxes

[Gabriel Ivăncescu]

On Wed, Nov 21, 2018 at 12:39 AM Nikolay Sivov  wrote:
> For example painting part:
> > @@ -494,7 +559,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> >           RECT r;
> >           HRGN hrgn;
> > -
> > if (!item)
> > -
> > if (index >= descr->nb_items)
> > {
>
> 'index' should be made unsigned throughout, unless there is a reason not to.

I don't mind making it unsigned but I don't see what it has to do with
the patch and didn't want to make more changes than necessary.

This check is changed because checking for NULL item will not work if
(1) index is zero (valid) and (2) we have single-selection LBS_NODATA.
In this case it will erroneously assume it's out of bounds (NULL + 0 =
NULL) when it's a perfectly valid index (zero). So it's related to
LBS_NODATA that's why it's in this patch.
> >       if (action == ODA_FOCUS)
> >           DrawFocusRect( hdc, rect );
> >
> > @@ -517,16 +582,19 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> >           dis.hDC          = hdc;
> >           dis.itemID       = index;
> >           dis.itemState    = 0;
> > -
> >    if (item->selected) dis.itemState |= ODS_SELECTED;
> > -
> >    if ( ( is_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) ||
> > -
> >         (!is_singlesel_NODATA(descr) && item->selected) )
> > -
> >        dis.itemState |= ODS_SELECTED;
> > -
>
> This is really ugly.

Right, I'll just set item to NULL and if so assume it's LBS_NODATA.
> > @@ -1678,28 +1757,38 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
> >       LISTBOX_InvalidateItems( descr, index );
> >   descr->nb_items--;
> > - LISTBOX_DeleteItem( descr, index );
> > -
> > - if (!descr->nb_items) return LB_OKAY;
> > -
> > - /* Remove the item */
> > - if (is_singlesel_NODATA(descr))
> > - {
> > -
> >    if (!descr->nb_items)
> > -
> >    {
> > -
> >        SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> > -
> >        return LB_OKAY;
> > -
> >    }
> > - }
>
> We'll need additional test for that.

This is not new behavior though. I did it because that's what happens
with DeleteItem when it removes the last item, so that it keeps the
same behavior as before. If this isn't based on a test then the normal
listbox code is wrong too.
> > - for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
> > - if (!is_singlesel_NODATA(descr))
> > -
> >    for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
>
> Is it the same as testing for null items array?

For now yes since we only have single-selection listboxes. I'll change
it to NULL check then.
> > static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
> >   {
> > - LRESULT ret;
> > -
> >
> > INT orig_num;
> >
> > if (!(descr->style & LBS_NODATA)) return LB_ERR;
> > -
> >
> > count = max(count, 0);
>
> Do we have tests for that?

Ah no, I'll see what I can add here as a test. I just did it to
prevent it from blowing up on negative values.
> >   case LB_GETSEL:
> >       if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items))
> >           return LB_ERR;
> > -
> >    if (is_singlesel_NODATA(descr))
> > -
> >        return NODATA_is_sel(descr, wParam);
> >     return descr->items[wParam].selected;
>
> Do you need this? For single selection case you'll only need to check
> wParam == selected_item, regardless of LBS_NODATA presence.

But, as I mentioned in the comments of the patch, this will make the
multi-selection patch smaller, since it will share this code path.
This will become a check for NODATA rather than just single-selection
and anything else will be identical (the helper itself will be
extended).

If I change it for single-selection, I then have to add *another*
check for LBS_NODATA in the next patch, and also keep the normal code
path. IMO let's just keep it like this.
> Regarding item storage, it's important to test what happens when
> multiselection styles are changed after window creation.

Well, the current code assumes they can't be changed since it never
updates them. I don't see why LBS_NODATA has to be special here, and I
really didn't want to complicate this more than it is, but to keep the
existing behavior where possible.

I'll see what I can do about tests though.
> Speaking of optimizations, having additional field for a number of
> selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.

Yeah, but it will require some changes in existing cases (not just
LBS_NODATA) to keep track of them (like when setting the range of
selections), I think we should leave that for later. I don't know if
it's very worth it since getting the count should be fast anyway on
multi-selection listboxes (and usually it's not spammed by apps).