[PATCH 4/5] comctl32/listbox: Implement LBS_NODATA for single-selection listboxes
Gabriel Ivăncescu
gabrielopcode at gmail.com
Mon Feb 11 05:09:14 CST 2019
On 2/11/19 12:10 PM, Huw Davies wrote:
> 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.
>
Okay will do that.
>> 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.
>
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?
>> +
>> + /* 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.
>
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.
>> -
>> - 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.
>
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)
>> 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