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

Huw Davies huw at codeweavers.com
Mon Feb 11 04:10:24 CST 2019


On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Iv─âncescu wrote:
> The LBS_NODATA style's purpose is to drastically improve performance and
> memory usage on very large lists, since they should function as virtual
> lists. Thus, don't store any data for single-selection listboxes (descr->items
> always NULL).
> 
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374
> Signed-off-by: Gabriel Iv─âncescu <gabrielopcode at gmail.com>
> ---
> 
> Most of the changes are just indentation in InsertItem/RemoveItem.
> 
>  dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++--------------
>  1 file changed, 90 insertions(+), 50 deletions(-)
> 
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index d115617..fd3bfa6 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -19,7 +19,7 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>   *
>   * TODO:
> - *    - LBS_NODATA
> + *    - LBS_NODATA for multi-selection listboxes
>   */
>  
>  #include <string.h>
> @@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index )
>      return descr->items[index].selected;
>  }
>  
> +static BOOL is_singlesel_NODATA(const LB_DESCR *descr)
> +{
> +    return (descr->style & LBS_NODATA) &&
> +           !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));
> +}
> +

There are way too many is_singlesel_NODATA() special cases in this patch.
Instead of that, we should add more helpers like is_item_selected().
I've given some examples below.

>  /***********************************************************************
>   *           LISTBOX_GetCurrentPageSize
>   *
> @@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
>          GetClientRect(descr->self, &r);
>          hrgn = set_control_clipping( hdc, &r );
>  
> +        if (is_singlesel_NODATA(descr)) item = NULL;

This is used to cover item->data (and item->str in the TRACE).
So introduce a get_item_data() and get_item_string() helper
that returns NULL in the no_data case.  Patches to add those
helpers obviously come before this patch.

>          dis.CtlType      = ODT_LISTBOX;
>          dis.CtlID        = GetWindowLongPtrW( descr->self, GWLP_ID );
>          dis.hwndItem     = descr->self;
> @@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
>  {
>      UINT new_size = descr->nb_items + nb_items;
>  
> -    if (new_size > descr->items_size && !resize_storage(descr, new_size))
> -        return LB_ERRSPACE;
> +    /* Windows keeps track of (unaligned) reserved space
> +       for LBS_NODATA, despite the fact it is useless */
> +    if (new_size > descr->items_size)
> +    {
> +        if (is_singlesel_NODATA(descr))
> +            descr->items_size = new_size;
> +        else if (!resize_storage(descr, new_size))
> +            return LB_ERRSPACE;
> +    }

Have resize_storage() handle no_data, it'll fix up ->items_size too, so there
should be no need to change this bit of code.

>      return descr->items_size;
>  }
>  
> @@ -1460,10 +1474,13 @@ static LRESULT LISTBOX_SetSelection( LB_DESCR *descr, INT index,
>      {
>          INT oldsel = descr->selected_item;
>          if (index == oldsel) return LB_OKAY;
> -        if (oldsel != -1) descr->items[oldsel].selected = FALSE;
> -        if (index != -1) descr->items[index].selected = TRUE;
> -        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
> +        if (!(descr->style & LBS_NODATA))
> +        {
> +            if (oldsel != -1) descr->items[oldsel].selected = FALSE;
> +            if (index != -1) descr->items[index].selected = TRUE;
> +        }

Add a helper to set an item's selection state.

>          descr->selected_item = index;
> +        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
>          if (index != -1) LISTBOX_RepaintItem( descr, index, ODA_SELECT );
>          if (send_notify && descr->nb_items) SEND_NOTIFICATION( descr,
>                                 (index != -1) ? LBN_SELCHANGE : LBN_SELCANCEL );
> @@ -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.

> +
> +        /* 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)
> +        {
> +            MEASUREITEMSTRUCT mis;
> +            UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
>  
> -        mis.CtlType    = ODT_LISTBOX;
> -        mis.CtlID      = id;
> -        mis.itemID     = index;
> -        mis.itemData   = data;
> -        mis.itemHeight = descr->item_height;
> -        SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
> -        item->height = mis.itemHeight ? mis.itemHeight : 1;
> -        TRACE("[%p]: measure item %d (%s) = %d\n",
> -              descr->self, index, str ? debugstr_w(str) : "", item->height );
> +            mis.CtlType    = ODT_LISTBOX;
> +            mis.CtlID      = id;
> +            mis.itemID     = index;
> +            mis.itemData   = data;
> +            mis.itemHeight = descr->item_height;
> +            SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
> +            item->height = mis.itemHeight ? mis.itemHeight : 1;
> +            TRACE("[%p]: measure item %d (%s) = %d\n",
> +                  descr->self, index, str ? debugstr_w(str) : "", item->height );
> +        }
>      }
>  
>      /* Repaint the items */
> @@ -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.

> -
> -    if (!descr->nb_items) return LB_OKAY;
> +    if (is_singlesel_NODATA(descr))
> +    {
> +        if (!descr->nb_items)
> +        {
> +            SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> +            return LB_OKAY;
> +        }
> +    }
> +    else
> +    {
> +        LISTBOX_DeleteItem( descr, index );
>  
> -    /* Remove the item */
> +        if (!descr->nb_items) return LB_OKAY;
>  
> -    item = &descr->items[index];
> -    if (index < descr->nb_items)
> -        RtlMoveMemory( item, item + 1,
> -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> -    if (descr->anchor_item == descr->nb_items) descr->anchor_item--;
> +        /* Remove the item */
> +        item = &descr->items[index];
> +        if (index < descr->nb_items)
> +            RtlMoveMemory( item, item + 1,
> +                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
>  
> -    resize_storage(descr, descr->nb_items);
> +        resize_storage(descr, descr->nb_items);
> +    }
> +    descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
>  
>      /* Repaint the items */
>  
> @@ -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.

>      HeapFree( GetProcessHeap(), 0, descr->items );
>      descr->nb_items      = 0;
>      descr->top_item      = 0;
> @@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
>  
>      if (count > orig_num)
>      {
> -        if (!resize_storage(descr, count))
> -            return LB_ERRSPACE;
> -        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> +        if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
> +        {
> +            if (!resize_storage(descr, count))
> +                return LB_ERRSPACE;
> +            memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> +        }
> +        else
> +            descr->items_size = max(descr->items_size, count);
>          descr->nb_items = count;
>  
>          LISTBOX_UpdateScroll(descr);
> @@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
>          {
>              descr->anchor_item = min(descr->anchor_item, count - 1);
>  
> -            resize_storage(descr, count);
> -            if (descr->selected_item >= count)
> +            if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
> +                resize_storage(descr, count);
> +            else if (descr->selected_item >= descr->nb_items)
>                  descr->selected_item = -1;
>  
>              LISTBOX_UpdateScroll(descr);

And again resize_storage() will handle this.

Huw.



More information about the wine-devel mailing list