[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