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@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));}elsedescr->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.