[PATCH 1/4] comctl32/listbox: Use is_item_selected in GetSelCount and GetSelItems

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

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;
 }

[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Gabriel Ivăncescu]

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 
---

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)

[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Huw Davies]

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
>
> 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.

[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Gabriel Ivăncescu]

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
> >
> > 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.

[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Gabriel Ivăncescu]

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
> > >
> > > 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.

[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Huw Davies]

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.

[PATCH 2/4] comctl32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Gabriel Ivăncescu]

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 :-)

[PATCH 3/4] user32/listbox: Use is_item_selected in GetSelCount and GetSelItems

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 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;
 }

[PATCH 3/4] user32/listbox: Use is_item_selected in GetSelCount and GetSelItems

[Marvin]

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

[PATCH 4/4] user32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Gabriel Ivăncescu]

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 
---
 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)

[PATCH 4/4] user32/listbox: Implement LBS_NODATA for multi-selection listboxes

[Marvin]

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