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

[Nikolay Sivov]

On 11/20/18 4:40 PM, Gabriel Ivăncescu wrote:

> I reduced the patch to just single-selection changes so that it still
> works. After these are in, the multi-selection patches should be smaller.

This should be reduced further.

>
> Please note that the new helper functions (such as NODATA_is_sel) should
> remain as-is because they will be extended later for multi-selection
> listboxes, even if they seem redundant for now, so that I can really trim
> down the size of the multi-selection patches.:-)
>
> By the way, this patch really looks larger than it is. Most of the changes
> are in SetCount and the helper functions. InsertItem and RemoveItem seem
> large, but the changes are minor -- it's mostly just indentation, so it
> should be simple to review.

It is large, and again for no good reason.

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.

>   	    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.

>           if (!ignoreFocus && (descr->focus_item == index) &&
>               (descr->caret_on) &&
>               (descr->in_focus)) dis.itemState |= ODS_FOCUS;
>           if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED;
> -        dis.itemData     = item->data;
> +        dis.itemData     = (descr->style & LBS_NODATA) ? 0 : item->data;
>           dis.rcItem       = *rect;
>           TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n",
> -              descr->self, index, debugstr_w(item->str), action,
> -              dis.itemState, wine_dbgstr_rect(rect) );
> +              descr->self, index, debugstr_w((descr->style & LBS_NODATA) ? NULL : item->str),
> +              action, dis.itemState, wine_dbgstr_rect(rect) );
>           SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis);
>           SelectClipRgn( hdc, hrgn );
>           if (hrgn) DeleteObject( hrgn );

Instead of spreading style checks just make sure 'item' is unset correctly.

> @@ -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.

> -    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?

>   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?

>       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.

Regarding item storage, it's important to test what happens when 
multiselection styles are changed after window creation.

Speaking of optimizations, having additional field for a number of 
selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.