[PATCH v2 1/4] comctl32/listbox: Use a helper to set item data by index
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 2b6702a..954866a 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -161,6 +161,11 @@ static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; } +static void set_item_data( LB_DESCR *descr, UINT index, ULONG_PTR data ) +{ + if (!(descr->style & LBS_NODATA)) descr->items[index].data = data; +} + static WCHAR *get_item_string( const LB_DESCR *descr, UINT index ) { return HAS_STRINGS(descr) ? descr->items[index].str : NULL; @@ -2684,7 +2689,7 @@ static LRESULT CALLBACK LISTBOX_WindowProc( HWND hwnd, UINT msg, WPARAM wParam, SetLastError(ERROR_INVALID_INDEX); return LB_ERR; } - if (!(descr->style & LBS_NODATA)) descr->items[wParam].data = lParam; + set_item_data(descr, wParam, lParam); /* undocumented: returns TRUE, not LB_OKAY (0) */ return TRUE; -- 2.20.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- no-op patch. This has been cleaned up so now all the comparison code (for each of the 3 variants) is only written once because the outer loop will wrap around, which also avoids the ugly macro. Of course it also uses the helpers and avoids having a pointless item variable (especially since we already tracked the index to begin with). dlls/comctl32/listbox.c | 66 ++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 954866a..dd29dc6 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -965,46 +965,48 @@ static INT LISTBOX_FindFileStrPos( LB_DESCR *descr, LPCWSTR str ) */ static INT LISTBOX_FindString( LB_DESCR *descr, INT start, LPCWSTR str, BOOL exact ) { - INT i; - LB_ITEMDATA *item; + INT i, end = descr->nb_items; if (descr->style & LBS_NODATA) return LB_ERR; - if (start >= descr->nb_items) start = -1; - item = descr->items + start + 1; + start++; + if (start >= descr->nb_items) start = 0; if (HAS_STRINGS(descr)) { if (!str || ! str[0] ) return LB_ERR; if (exact) { - for (i = start + 1; i < descr->nb_items; i++, item++) - if (!LISTBOX_lstrcmpiW( descr->locale, str, item->str )) return i; - for (i = 0, item = descr->items; i <= start; i++, item++) - if (!LISTBOX_lstrcmpiW( descr->locale, str, item->str )) return i; + for (;;) + { + for (i = start; i < end; i++) + if (!LISTBOX_lstrcmpiW(descr->locale, str, get_item_string(descr, i))) + return i; + if (!start) break; + end = start; + start = 0; + } } else { - /* Special case for drives and directories: ignore prefix */ -#define CHECK_DRIVE(item) \ - if ((item)->str[0] == '[') \ - { \ - if (!strncmpiW( str, (item)->str+1, len )) return i; \ - if (((item)->str[1] == '-') && !strncmpiW(str, (item)->str+2, len)) \ - return i; \ - } - + /* Special case for drives and directories: ignore prefix */ INT len = strlenW(str); - for (i = start + 1; i < descr->nb_items; i++, item++) + for (;;) { - if (!strncmpiW( str, item->str, len )) return i; - CHECK_DRIVE(item); - } - for (i = 0, item = descr->items; i <= start; i++, item++) - { - if (!strncmpiW( str, item->str, len )) return i; - CHECK_DRIVE(item); + for (i = start; i < end; i++) + { + WCHAR *item_str = get_item_string(descr, i); + + if (!strncmpiW(str, item_str, len)) return i; + if (item_str[0] == '[') + { + if (!strncmpiW(str, item_str + 1, len)) return i; + if (item_str[1] == '-' && !strncmpiW(str, item_str + 2, len)) return i; + } + } + if (!start) break; + end = start; + start = 0; } -#undef CHECK_DRIVE } } else @@ -1014,10 +1016,14 @@ static INT LISTBOX_FindString( LB_DESCR *descr, INT start, LPCWSTR str, BOOL exa return LISTBOX_FindStringPos( descr, str, TRUE ); /* Otherwise use a linear search */ - for (i = start + 1; i < descr->nb_items; i++, item++) - if (item->data == (ULONG_PTR)str) return i; - for (i = 0, item = descr->items; i <= start; i++, item++) - if (item->data == (ULONG_PTR)str) return i; + for (;;) + { + for (i = start; i < end; i++) + if (get_item_data(descr, i) == (ULONG_PTR)str) return i; + if (!start) break; + end = start; + start = 0; + } } return LB_ERR; } -- 2.20.1
On Tue, Feb 26, 2019 at 02:53:36PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
no-op patch.
This has been cleaned up so now all the comparison code (for each of the 3 variants) is only written once because the outer loop will wrap around, which also avoids the ugly macro. Of course it also uses the helpers and avoids having a pointless item variable (especially since we already tracked the index to begin with).
dlls/comctl32/listbox.c | 66 ++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 954866a..dd29dc6 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -965,46 +965,48 @@ static INT LISTBOX_FindFileStrPos( LB_DESCR *descr, LPCWSTR str ) */ static INT LISTBOX_FindString( LB_DESCR *descr, INT start, LPCWSTR str, BOOL exact ) { - INT i; - LB_ITEMDATA *item; + INT i, end = descr->nb_items;
if (descr->style & LBS_NODATA) return LB_ERR;
- if (start >= descr->nb_items) start = -1; - item = descr->items + start + 1; + start++; + if (start >= descr->nb_items) start = 0; if (HAS_STRINGS(descr)) { if (!str || ! str[0] ) return LB_ERR; if (exact) { - for (i = start + 1; i < descr->nb_items; i++, item++) - if (!LISTBOX_lstrcmpiW( descr->locale, str, item->str )) return i; - for (i = 0, item = descr->items; i <= start; i++, item++) - if (!LISTBOX_lstrcmpiW( descr->locale, str, item->str )) return i; + for (;;) + { + for (i = start; i < end; i++) + if (!LISTBOX_lstrcmpiW(descr->locale, str, get_item_string(descr, i))) + return i; + if (!start) break; + end = start; + start = 0; + } }
These nested for loops are ugly. What about something like this (having set up start as you've already done): for (i = 0, item = start; i < descr->nb_items; i++, item++) { if (item == descr->nb_items) item = 0; if (!LISTBOX_lstrcmpiW(descr->locale, str, get_item_string(descr, item))) return item; } Huw.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index dd29dc6..66297e0 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static size_t get_item_size( const LB_DESCR *descr ) +{ + return sizeof(LB_ITEMDATA); +} + static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -137,7 +142,7 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1); if ((descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) != LBS_NODATA) { - items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA)); + items = heap_realloc(descr->items, items_size * get_item_size(descr)); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -151,7 +156,7 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) if ((descr->style & LBS_NODATA) && descr->items && items_size > descr->nb_items) { memset(&descr->items[descr->nb_items], 0, - (items_size - descr->nb_items) * sizeof(LB_ITEMDATA)); + (items_size - descr->nb_items) * get_item_size(descr)); } return TRUE; } @@ -202,7 +207,7 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR item = descr->items + index; if (index < descr->nb_items) - memmove(item + 1, item, (descr->nb_items - index) * sizeof(LB_ITEMDATA)); + memmove(item + 1, item, (descr->nb_items - index) * get_item_size(descr)); item->str = str; item->data = HAS_STRINGS(descr) ? 0 : data; @@ -218,7 +223,7 @@ static void remove_item_data(LB_DESCR *descr, UINT index) item = descr->items + index; if (index < descr->nb_items) - memmove(item, item + 1, (descr->nb_items - index) * sizeof(LB_ITEMDATA)); + memmove(item, item + 1, (descr->nb_items - index) * get_item_size(descr)); } /*********************************************************************** -- 2.20.1
Use a byte array to store selection state of items, since we don't need any other data for LBS_NODATA multi-selection listboxes. This improves memory usage dramatically for large lists, and performance boosts are nice too. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 84 +++++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 28 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 66297e0..f8b09c8 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -71,7 +71,11 @@ typedef struct UINT style; /* Window style */ INT width; /* Window width */ INT height; /* Window height */ - LB_ITEMDATA *items; /* Array of items */ + union + { + LB_ITEMDATA *items; /* Array of items */ + BYTE *items_nodata; /* For multi-selection LBS_NODATA */ + } u; INT nb_items; /* Number of items */ UINT items_size; /* Total number of allocated items in the array */ INT top_item; /* Top visible item */ @@ -127,9 +131,19 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +/* + For listboxes without LBS_NODATA, an array of LB_ITEMDATA is allocated + to store the states of each item into descr->u.items. + + For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->u.items_nodata will always be NULL. + + For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state into descr->u.items_nodata. +*/ static size_t get_item_size( const LB_DESCR *descr ) { - return sizeof(LB_ITEMDATA); + return (descr->style & LBS_NODATA) ? sizeof(BYTE) : sizeof(LB_ITEMDATA); } static BOOL resize_storage(LB_DESCR *descr, UINT items_size) @@ -142,20 +156,20 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1); if ((descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) != LBS_NODATA) { - items = heap_realloc(descr->items, items_size * get_item_size(descr)); + items = heap_realloc(descr->u.items, items_size * get_item_size(descr)); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); return FALSE; } - descr->items = items; + descr->u.items = items; } descr->items_size = items_size; } - if ((descr->style & LBS_NODATA) && descr->items && items_size > descr->nb_items) + if ((descr->style & LBS_NODATA) && descr->u.items_nodata && items_size > descr->nb_items) { - memset(&descr->items[descr->nb_items], 0, + memset(&descr->u.items_nodata[descr->nb_items], 0, (items_size - descr->nb_items) * get_item_size(descr)); } return TRUE; @@ -163,67 +177,81 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index ) { - return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data; + return (descr->style & LBS_NODATA) ? 0 : descr->u.items[index].data; } static void set_item_data( LB_DESCR *descr, UINT index, ULONG_PTR data ) { - if (!(descr->style & LBS_NODATA)) descr->items[index].data = data; + if (!(descr->style & LBS_NODATA)) descr->u.items[index].data = data; } static WCHAR *get_item_string( const LB_DESCR *descr, UINT index ) { - return HAS_STRINGS(descr) ? descr->items[index].str : NULL; + return HAS_STRINGS(descr) ? descr->u.items[index].str : NULL; } static UINT get_item_height( const LB_DESCR *descr, UINT index ) { - return (descr->style & LBS_NODATA) ? 0 : descr->items[index].height; + return (descr->style & LBS_NODATA) ? 0 : descr->u.items[index].height; } static void set_item_height( LB_DESCR *descr, UINT index, UINT height ) { - if (!(descr->style & LBS_NODATA)) descr->items[index].height = height; + if (!(descr->style & LBS_NODATA)) descr->u.items[index].height = height; } static BOOL is_item_selected( const LB_DESCR *descr, UINT index ) { if (!(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))) return index == descr->selected_item; - return descr->items[index].selected; + if (descr->style & LBS_NODATA) + return descr->u.items_nodata[index]; + else + return descr->u.items[index].selected; } static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) { if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) - descr->items[index].selected = state; + { + if (descr->style & LBS_NODATA) + descr->u.items_nodata[index] = state; + else + descr->u.items[index].selected = state; + } } static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) { - LB_ITEMDATA *item; + size_t size = get_item_size(descr); + BYTE *p = descr->u.items_nodata + index * size; - if (!descr->items) return; + if (!descr->u.items) return; - item = descr->items + index; if (index < descr->nb_items) - memmove(item + 1, item, (descr->nb_items - index) * get_item_size(descr)); + memmove(p + size, p, (descr->nb_items - index) * size); - item->str = str; - item->data = HAS_STRINGS(descr) ? 0 : data; - item->height = 0; - item->selected = FALSE; + if (descr->style & LBS_NODATA) + descr->u.items_nodata[index] = FALSE; + else + { + LB_ITEMDATA *item = descr->u.items + index; + item->str = str; + item->data = HAS_STRINGS(descr) ? 0 : data; + item->height = 0; + item->selected = FALSE; + } } static void remove_item_data(LB_DESCR *descr, UINT index) { - LB_ITEMDATA *item; + size_t size = get_item_size(descr); + BYTE *p = descr->u.items_nodata + index * size; - if (!descr->items) return; + if (!descr->u.items) return; - item = descr->items + index; if (index < descr->nb_items) - memmove(item, item + 1, (descr->nb_items - index) * get_item_size(descr)); + memmove(p, p + size, (descr->nb_items - index) * size); } /*********************************************************************** @@ -1785,14 +1813,14 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) if (!(descr->style & LBS_NODATA)) for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i); - HeapFree( GetProcessHeap(), 0, descr->items ); + HeapFree( GetProcessHeap(), 0, descr->u.items ); descr->nb_items = 0; descr->top_item = 0; descr->selected_item = -1; descr->focus_item = 0; descr->anchor_item = -1; descr->items_size = 0; - descr->items = NULL; + descr->u.items = NULL; } @@ -2539,7 +2567,7 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc ) descr->style = GetWindowLongW( descr->self, GWL_STYLE ); descr->width = rect.right - rect.left; descr->height = rect.bottom - rect.top; - descr->items = NULL; + descr->u.items = NULL; descr->items_size = 0; descr->nb_items = 0; descr->top_item = 0; -- 2.20.1
I forgot to remove the TODO: at the top of the file; if this patchset is okay, please remove it before committing.
participants (2)
-
Gabriel Ivăncescu -
Huw Davies