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

[Gabriel Ivăncescu]

On 2/11/19 1:22 PM, Huw Davies wrote:
> 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.
> 

Well, I find it awkward that sending SetCount to zero first would be far 
faster than LB_RESETCONTENT directly (that's what this will turn out to 
be), or that "growing" the list is instant while ResetContent is not. 
And it's just one line of code, I think when reducing O(n) to O(1) it 
should be worth it, given the point of LBS_NODATA.

(also, a variant of that line of code will also help with 
multi-selection listboxes later, which can also be instant)