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

[Nikolay Sivov]

On 11/21/18 1:12 PM, Gabriel Ivăncescu wrote:

> On Wed, Nov 21, 2018 at 12:39 AM Nikolay Sivov <nsivov at codeweavers.com> 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 condition will depend on it.
>
> 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.
Yes, but now it will be duplicated in two places.
>
>>> -    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.
It doesn't matter, for multiselect case you'll have to cleanup one way 
or another.
>
>>>    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.
 From what I can tell negative values are accepted.
>
>>>        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.
I don't see why NODATA would be special here.
>
>> 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.
It doesn't matter what is currently assumed, if we want to improve or 
optimize anything, we'll have to make sure it's correct.
>
>> 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).
I think it's a very small change, and yes, it's separate and unrelated 
to NODATA.