[PATCH 1/4] comctl32/listbox: Use is_item_selected in GetSelCount and GetSelItems
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Besides cleaning up the code, this is needed for multi-selection LBS_NODATA. dlls/comctl32/listbox.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index cbe57f2..b6024c3 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1024,13 +1024,12 @@ static INT LISTBOX_FindString( LB_DESCR *descr, INT start, LPCWSTR str, BOOL exa static LRESULT LISTBOX_GetSelCount( const LB_DESCR *descr ) { INT i, count; - const LB_ITEMDATA *item = descr->items; if (!(descr->style & LBS_MULTIPLESEL) || (descr->style & LBS_NOSEL)) return LB_ERR; - for (i = count = 0; i < descr->nb_items; i++, item++) - if (item->selected) count++; + for (i = count = 0; i < descr->nb_items; i++) + count += is_item_selected(descr, i); return count; } @@ -1041,11 +1040,10 @@ static LRESULT LISTBOX_GetSelCount( const LB_DESCR *descr ) static LRESULT LISTBOX_GetSelItems( const LB_DESCR *descr, INT max, LPINT array ) { INT i, count; - const LB_ITEMDATA *item = descr->items; if (!(descr->style & LBS_MULTIPLESEL)) return LB_ERR; - for (i = count = 0; (i < descr->nb_items) && (count < max); i++, item++) - if (item->selected) array[count++] = i; + for (i = count = 0; (i < descr->nb_items) && (count < max); i++) + if (is_item_selected(descr, i)) array[count++] = i; return count; } -- 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> --- For now, this should be good enough (byte array). Unfortunately, using other storage representations in the future (like selection ranges, or bit arrays) would probably need the avoided NODATA helpers to split the functions (in order to be easier maintainable), so for now this will suffice. dlls/comctl32/listbox.c | 48 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b6024c3..5afdf5c 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -18,8 +18,6 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * - * TODO: - * - LBS_NODATA for multi-selection listboxes */ #include <string.h> @@ -127,6 +125,15 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +/* + Resize the item storage array if needed. + + For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->items will always be NULL. + + For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state, instead of the usual LB_ITEMDATA. +*/ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -137,7 +144,10 @@ 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)); + size_t size = items_size; + if (!(descr->style & LBS_NODATA)) size *= sizeof(LB_ITEMDATA); + + items = heap_realloc(descr->items, size); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -150,8 +160,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)); + memset((BYTE*)descr->items + descr->nb_items, 0, items_size - descr->nb_items); } return TRUE; } @@ -180,13 +189,21 @@ 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 ((BYTE*)descr->items)[index]; + else + return descr->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) + ((BYTE*)descr->items)[index] = state; + else + descr->items[index].selected = state; + } } static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) @@ -194,6 +211,15 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR LB_ITEMDATA *item; if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item + 1, item, descr->nb_items - index); + *item = FALSE; + return; + } item = descr->items + index; if (index < descr->nb_items) @@ -210,6 +236,14 @@ static void remove_item_data(LB_DESCR *descr, UINT index) LB_ITEMDATA *item; if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item, item + 1, descr->nb_items - index); + return; + } item = descr->items + index; if (index < descr->nb_items) -- 2.20.1
On Fri, Feb 22, 2019 at 02:00:57PM +0200, Gabriel Ivăncescu wrote:
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> ---
For now, this should be good enough (byte array). Unfortunately, using other storage representations in the future (like selection ranges, or bit arrays) would probably need the avoided NODATA helpers to split the functions (in order to be easier maintainable), so for now this will suffice.
dlls/comctl32/listbox.c | 48 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b6024c3..5afdf5c 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -18,8 +18,6 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * - * TODO: - * - LBS_NODATA for multi-selection listboxes */
#include <string.h> @@ -127,6 +125,15 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+/* + Resize the item storage array if needed. + + For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->items will always be NULL. + + For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state, instead of the usual LB_ITEMDATA. +*/ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -137,7 +144,10 @@ 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)); + size_t size = items_size; + if (!(descr->style & LBS_NODATA)) size *= sizeof(LB_ITEMDATA); + + items = heap_realloc(descr->items, size); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -150,8 +160,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)); + memset((BYTE*)descr->items + descr->nb_items, 0, items_size - descr->nb_items); } return TRUE; } @@ -180,13 +189,21 @@ 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 ((BYTE*)descr->items)[index]; + else + return descr->items[index].selected;
Make descr->items a union of the two types, that will get rid of these horrible casts. If you find you need to change a lot of code to make this work, then you need to add more calls to item getter/setter helpers.
}
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) + ((BYTE*)descr->items)[index] = state; + else + descr->items[index].selected = state; + } }
static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) @@ -194,6 +211,15 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR LB_ITEMDATA *item;
if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item + 1, item, descr->nb_items - index); + *item = FALSE; + return; + }
This is essentially the same code as what follows for the !LBS_NODATA case. If you added a helper to return the size of an item then these could be merged. This helper could also be used in resize_storage(). Huw.
On 2/25/19 12:37 PM, Huw Davies wrote:
On Fri, Feb 22, 2019 at 02:00:57PM +0200, Gabriel Ivăncescu wrote:
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> ---
For now, this should be good enough (byte array). Unfortunately, using other storage representations in the future (like selection ranges, or bit arrays) would probably need the avoided NODATA helpers to split the functions (in order to be easier maintainable), so for now this will suffice.
dlls/comctl32/listbox.c | 48 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b6024c3..5afdf5c 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -18,8 +18,6 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * - * TODO: - * - LBS_NODATA for multi-selection listboxes */
#include <string.h> @@ -127,6 +125,15 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+/* + Resize the item storage array if needed. + + For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->items will always be NULL. + + For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state, instead of the usual LB_ITEMDATA. +*/ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -137,7 +144,10 @@ 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)); + size_t size = items_size; + if (!(descr->style & LBS_NODATA)) size *= sizeof(LB_ITEMDATA); + + items = heap_realloc(descr->items, size); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -150,8 +160,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)); + memset((BYTE*)descr->items + descr->nb_items, 0, items_size - descr->nb_items); } return TRUE; } @@ -180,13 +189,21 @@ 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 ((BYTE*)descr->items)[index]; + else + return descr->items[index].selected;
Make descr->items a union of the two types, that will get rid of these horrible casts. If you find you need to change a lot of code to make this work, then you need to add more calls to item getter/setter helpers.
}
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) + ((BYTE*)descr->items)[index] = state; + else + descr->items[index].selected = state; + } }
static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) @@ -194,6 +211,15 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR LB_ITEMDATA *item;
if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item + 1, item, descr->nb_items - index); + *item = FALSE; + return; + }
This is essentially the same code as what follows for the !LBS_NODATA case. If you added a helper to return the size of an item then these could be merged. This helper could also be used in resize_storage().
Huw.
Yeah, I wanted to avoid too many changes (also because I made these casts inside the helpers only), it's too bad we can't use anonymous unions in Wine's C89. I'll see what I can do with the union and helpers.
On 2/25/19 1:15 PM, Gabriel Ivăncescu wrote:
On 2/25/19 12:37 PM, Huw Davies wrote:
On Fri, Feb 22, 2019 at 02:00:57PM +0200, Gabriel Ivăncescu wrote:
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> ---
For now, this should be good enough (byte array). Unfortunately, using other storage representations in the future (like selection ranges, or bit arrays) would probably need the avoided NODATA helpers to split the functions (in order to be easier maintainable), so for now this will suffice.
dlls/comctl32/listbox.c | 48 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b6024c3..5afdf5c 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -18,8 +18,6 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * - * TODO: - * - LBS_NODATA for multi-selection listboxes */ #include <string.h> @@ -127,6 +125,15 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +/* + Resize the item storage array if needed. + + For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->items will always be NULL. + + For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state, instead of the usual LB_ITEMDATA. +*/ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -137,7 +144,10 @@ 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)); + size_t size = items_size; + if (!(descr->style & LBS_NODATA)) size *= sizeof(LB_ITEMDATA); + + items = heap_realloc(descr->items, size); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -150,8 +160,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)); + memset((BYTE*)descr->items + descr->nb_items, 0, items_size - descr->nb_items); } return TRUE; } @@ -180,13 +189,21 @@ 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 ((BYTE*)descr->items)[index]; + else + return descr->items[index].selected;
Make descr->items a union of the two types, that will get rid of these horrible casts. If you find you need to change a lot of code to make this work, then you need to add more calls to item getter/setter helpers.
} 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) + ((BYTE*)descr->items)[index] = state; + else + descr->items[index].selected = state; + } } static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) @@ -194,6 +211,15 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR LB_ITEMDATA *item; if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item + 1, item, descr->nb_items - index); + *item = FALSE; + return; + }
This is essentially the same code as what follows for the !LBS_NODATA case. If you added a helper to return the size of an item then these could be merged. This helper could also be used in resize_storage().
Huw.
Yeah, I wanted to avoid too many changes (also because I made these casts inside the helpers only), it's too bad we can't use anonymous unions in Wine's C89. I'll see what I can do with the union and helpers.
Ok, I have a question now: can I use a small helper that uses the cast instead of a union? Something like: static inline BYTE *get_nodata_items(const LB_DESCR *descr) { return (BYTE*)descr->items; } And use it where needed? It would require less changes to the code and, IMO, keep it cleaner (other than the one helper above). Such helpers are frequent in other parts of the code (mostly dealing with COM stuff). On a side note, I'll also rewrite FindString since it's a mess and would still be a mess even if I replaced it with the helpers only.
On Mon, Feb 25, 2019 at 03:39:14PM +0200, Gabriel Ivăncescu wrote:
Ok, I have a question now: can I use a small helper that uses the cast instead of a union? Something like:
static inline BYTE *get_nodata_items(const LB_DESCR *descr) { return (BYTE*)descr->items; }
And use it where needed? It would require less changes to the code and, IMO, keep it cleaner (other than the one helper above). Such helpers are frequent in other parts of the code (mostly dealing with COM stuff).
No. That's just hiding the hack. The issue is that regular storage and no date storage should have an equal footing. With a union it's clear. The only places you should need to change of the helpers and most of those don't do anything in the no data case, so it shouldn't be too bad.
On a side note, I'll also rewrite FindString since it's a mess and would still be a mess even if I replaced it with the helpers only.
Ok, good. Huw.
On 2/25/19 4:00 PM, Huw Davies wrote:
On Mon, Feb 25, 2019 at 03:39:14PM +0200, Gabriel Ivăncescu wrote:
Ok, I have a question now: can I use a small helper that uses the cast instead of a union? Something like:
static inline BYTE *get_nodata_items(const LB_DESCR *descr) { return (BYTE*)descr->items; }
And use it where needed? It would require less changes to the code and, IMO, keep it cleaner (other than the one helper above). Such helpers are frequent in other parts of the code (mostly dealing with COM stuff).
No. That's just hiding the hack. The issue is that regular storage and no date storage should have an equal footing. With a union it's clear. The only places you should need to change of the helpers and most of those don't do anything in the no data case, so it shouldn't be too bad.
Yes I just find the "descr->u.items" slightly uglier and wanted to avoid it even if it's a simple no-op (the u. part) since we don't have anon unions, but okay I'll use an union then :-)
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/listbox.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index 0be4867..0e9f97c 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -1066,13 +1066,12 @@ static INT LISTBOX_FindString( LB_DESCR *descr, INT start, LPCWSTR str, BOOL exa static LRESULT LISTBOX_GetSelCount( const LB_DESCR *descr ) { INT i, count; - const LB_ITEMDATA *item = descr->items; if (!(descr->style & LBS_MULTIPLESEL) || (descr->style & LBS_NOSEL)) return LB_ERR; - for (i = count = 0; i < descr->nb_items; i++, item++) - if (item->selected) count++; + for (i = count = 0; i < descr->nb_items; i++) + count += is_item_selected(descr, i); return count; } @@ -1083,11 +1082,10 @@ static LRESULT LISTBOX_GetSelCount( const LB_DESCR *descr ) static LRESULT LISTBOX_GetSelItems( const LB_DESCR *descr, INT max, LPINT array ) { INT i, count; - const LB_ITEMDATA *item = descr->items; if (!(descr->style & LBS_MULTIPLESEL)) return LB_ERR; - for (i = count = 0; (i < descr->nb_items) && (count < max); i++, item++) - if (item->selected) array[count++] = i; + for (i = count = 0; (i < descr->nb_items) && (count < max); i++) + if (is_item_selected(descr, i)) array[count++] = i; return count; } -- 2.20.1
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=48118 Your paranoid android. === debian9 (32 bit WoW report) === user32: msg.c:8713: Test failed: WaitForSingleObject failed 102 msg.c:8719: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8719: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8719: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000
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/user32/listbox.c | 48 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index 0e9f97c..cc06238 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -17,8 +17,6 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * - * TODO: - * - LBS_NODATA for multi-selection listboxes */ #include <string.h> @@ -122,6 +120,15 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +/* + Resize the item storage array if needed. + + For single-selection LBS_NODATA listboxes, no storage is allocated, + and thus descr->items will always be NULL. + + For multi-selection LBS_NODATA listboxes, one byte per item is stored + for the item's selection state, instead of the usual LB_ITEMDATA. +*/ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { LB_ITEMDATA *items; @@ -132,7 +139,10 @@ 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)); + size_t size = items_size; + if (!(descr->style & LBS_NODATA)) size *= sizeof(LB_ITEMDATA); + + items = heap_realloc(descr->items, size); if (!items) { SEND_NOTIFICATION(descr, LBN_ERRSPACE); @@ -145,8 +155,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)); + memset((BYTE*)descr->items + descr->nb_items, 0, items_size - descr->nb_items); } return TRUE; } @@ -175,13 +184,21 @@ 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 ((BYTE*)descr->items)[index]; + else + return descr->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) + ((BYTE*)descr->items)[index] = state; + else + descr->items[index].selected = state; + } } static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR data) @@ -189,6 +206,15 @@ static void insert_item_data(LB_DESCR *descr, UINT index, WCHAR *str, ULONG_PTR LB_ITEMDATA *item; if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item + 1, item, descr->nb_items - index); + *item = FALSE; + return; + } item = descr->items + index; if (index < descr->nb_items) @@ -205,6 +231,14 @@ static void remove_item_data(LB_DESCR *descr, UINT index) LB_ITEMDATA *item; if (!descr->items) return; + if (descr->style & LBS_NODATA) + { + BYTE *item = (BYTE*)descr->items + index; + + if (index < descr->nb_items) + memmove(item, item + 1, descr->nb_items - index); + return; + } item = descr->items + index; if (index < descr->nb_items) -- 2.20.1
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=48120 Your paranoid android. === debian9 (32 bit Chinese:China report) === user32: msg.c:8713: Test failed: WaitForSingleObject failed 102 msg.c:8719: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8719: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8719: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000 win.c:9871: Test failed: GetForegroundWindow() = (nil) === debian9 (32 bit WoW report) === user32: msg.c:8713: Test failed: WaitForSingleObject failed 102 msg.c:8719: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8719: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8719: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000
participants (3)
-
Gabriel Ivăncescu -
Huw Davies -
Marvin