[PATCH v4 1/4] comctl32/listbox: Move the item data insertion into a separate function

[Gabriel Ivăncescu]

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

This is a no-op patch. nb_items is incremented before the call and this is
taken care of in the helper function. This is for two reasons:

1) This simplifies the nodata patch (only adds a single line to
insert_item_data).
2) This matches what RemoveItem does, so it's more consistent.

dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 50ff5c5..330d3ae 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state)
         descr->items[index].selected = state;
 }

+static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data)
+{
+    LB_ITEMDATA *item;
+
+    item = &descr->items[index];
+
+    if (index < descr->nb_items - 1)
+        RtlMoveMemory(item + 1, item,
+                      (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA));
+    item->str      = str;
+    item->data     = HAS_STRINGS(descr) ? 0 : data;
+    item->height   = 0;
+    item->selected = FALSE;
+
+    /* 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 );
+    }
+}
+
 /***********************************************************************
  *           LISTBOX_GetCurrentPageSize
  *
@@ -1553,42 +1585,14 @@ static void LISTBOX_MoveCaret( LB_DESCR *descr, INT index, BOOL fully_visible )
 static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
                                    LPWSTR str, ULONG_PTR data )
 {
-    LB_ITEMDATA *item;
     INT oldfocus = descr->focus_item;

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)
-    {
-        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 );
-    }
+    insert_item_data(descr, index, str, data);

/* Repaint the items */

[PATCH v4 2/4] comctl32/listbox: Move the item data removal into a separate function

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/comctl32/listbox.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 330d3ae..214bfb0 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -126,6 +126,7 @@ typedef enum
 static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;

static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index );

static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
 {
@@ -208,6 +209,14 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d
     }
 }

+static void remove_item_data(LB_DESCR *descr, INT index)
+{
+    LISTBOX_DeleteItem(descr, index);
+    if (index < descr->nb_items)
+        RtlMoveMemory(&descr->items[index], &descr->items[index + 1],
+                      (descr->nb_items - index) * sizeof(LB_ITEMDATA));
+}
+
 /***********************************************************************
  *           LISTBOX_GetCurrentPageSize
  *
@@ -1689,8 +1698,6 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index )
  */
 static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
 {
-    LB_ITEMDATA *item;
-
     if ((index < 0) || (index >= descr->nb_items)) return LB_ERR;

/* We need to invalidate the original rect instead of the updated one. */
@@ -1702,16 +1709,9 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
         return LB_OKAY;
     }
     descr->nb_items--;
-    LISTBOX_DeleteItem( descr, index );
-
-    /* Remove the item */
-
-    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_item_data(descr, index);

+    descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
     resize_storage(descr, descr->nb_items);

/* Repaint the items */

[PATCH v4 2/4] comctl32/listbox: Move the item data removal into a separate function

[Huw Davies]

On Mon, Feb 18, 2019 at 03:47:52PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu
>
> dlls/comctl32/listbox.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index 330d3ae..214bfb0 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -126,6 +126,7 @@ typedef enum
>  static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
>
> static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
> +static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index );
>
> static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
>  {
> @@ -208,6 +209,14 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d
>      }
>  }
>
> +static void remove_item_data(LB_DESCR *descr, INT index)
> +{
> - LISTBOX_DeleteItem(descr, index);

Similarly let's leave LISTBOX_DeleteItem in RemoveItem().
> - if (index < descr->nb_items)
> -
>    RtlMoveMemory(&descr->items[index], &descr->items[index + 1],
> -
>                  (descr->nb_items - index) * sizeof(LB_ITEMDATA));
>
> +}
> -
>
> /***********************************************************************
> -
>       LISTBOX_GetCurrentPageSize
> -

[PATCH v4 3/4] comctl32/listbox: Implement LBS_NODATA for single-selection listboxes

[Gabriel Ivăncescu]

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

v4: Get rid of the nodata helpers and open code the checks.

I have kept the check in ResetContent because it reduces algorithm complexity
from O(n) to O(1) and does not depend on the list size. LBS_NODATA listboxes
can be large and possibly have millions of virtual items (that's what
they are for), so I think a single extra line check is really worth it
to avoid this scenario (it can reduce annoying stuttering when resetting
such a listbox to instant), and also to be consistent with SetCount(0) in
effect. The linked bug report is an indirect example of such case (an IDE
with a large database: https://forum.winehq.org/viewtopic.php?f=8&t=17812).

As a bonus, it also cleans up the formatting since it indents the line
anyway. This will also help with multi-selection nodata listboxes later.

dlls/comctl32/listbox.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 214bfb0..032a3b3 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>
@@ -136,17 +136,20 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
         items_size + LB_ARRAY_GRANULARITY * 2 < descr->items_size)
     {
         items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
-        items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA));
-        if (!items)
+        if ((descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) != LBS_NODATA)
         {
-            SEND_NOTIFICATION(descr, LBN_ERRSPACE);
-            return FALSE;
+            items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA));
+            if (!items)
+            {
+                SEND_NOTIFICATION(descr, LBN_ERRSPACE);
+                return FALSE;
+            }
+            descr->items = items;
         }
         descr->items_size = items_size;
-        descr->items = items;
     }

-    if ((descr->style & LBS_NODATA) && items_size > descr->nb_items)
+    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));
@@ -181,6 +184,7 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d
 {
     LB_ITEMDATA *item;

+    if (!descr->items) return;
     item = &descr->items[index];

if (index < descr->nb_items - 1)
@@ -211,6 +215,8 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d

static void remove_item_data(LB_DESCR *descr, INT index)
 {
+    if (!descr->items) return;
+
     LISTBOX_DeleteItem(descr, index);
     if (index < descr->nb_items)
         RtlMoveMemory(&descr->items[index], &descr->items[index + 1],
@@ -1751,7 +1757,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->style & LBS_NODATA))
+        for(i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
     HeapFree( GetProcessHeap(), 0, descr->items );
     descr->nb_items      = 0;
     descr->top_item      = 0;

[PATCH v4 4/4] comctl32/tests: Test negative counts in LB_SETCOUNT

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/comctl32/tests/listbox.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
index 0cdcdea..de73ad2 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -1996,6 +1996,11 @@ static void test_set_count( void )
     GetUpdateRect( listbox, &r, TRUE );
     ok( !IsRectEmpty( &r ), "got empty rect\n");

+    ret = SendMessageA( listbox, LB_SETCOUNT, -5, 0 );
+    ok( ret == 0, "got %d\n", ret );
+    ret = SendMessageA( listbox, LB_GETCOUNT, 0, 0 );
+    ok( ret == -5, "got %d\n", ret );
+
     DestroyWindow( listbox );

for (i = 0; i < ARRAY_SIZE(styles); ++i)

[Huw Davies]

On Mon, Feb 18, 2019 at 03:47:51PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu
>
> This is a no-op patch. nb_items is incremented before the call and this is
> taken care of in the helper function. This is for two reasons:
> - This simplifies the nodata patch (only adds a single line to
>
> insert_item_data).
> 2) This matches what RemoveItem does, so it's more consistent.
>
> dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index 50ff5c5..330d3ae 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state)
>          descr->items[index].selected = state;
>  }
>
> +static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data)
> +{
> - LB_ITEMDATA *item;
> -
> - item = &descr->items[index];
> -
> - if (index < descr->nb_items - 1)
> -
>    RtlMoveMemory(item + 1, item,
> -
>                  (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA));
> - item->str      = str;
> - item->data     = HAS_STRINGS(descr) ? 0 : data;
> - item->height   = 0;
> - item->selected = FALSE;
> -
> - /* 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 );
> - }

Let's leave the OWNERDRAWVARIABLE bit in InsertItem.  insert_item_data() should
just be about manipulating the items array.  You'll want to add a set_item_height()
helper first.

Huw.

[Gabriel Ivăncescu]

On 2/19/19 12:14 PM, Huw Davies wrote:
> On Mon, Feb 18, 2019 at 03:47:51PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu
> >
> > This is a no-op patch. nb_items is incremented before the call and this is
> > taken care of in the helper function. This is for two reasons:
> > - This simplifies the nodata patch (only adds a single line to
> >
> > insert_item_data).
> > 2) This matches what RemoveItem does, so it's more consistent.
> >
> > dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++-------------------
> >   1 file changed, 33 insertions(+), 29 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index 50ff5c5..330d3ae 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state)
> >           descr->items[index].selected = state;
> >   }
> >
> > +static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data)
> > +{
> > - LB_ITEMDATA *item;
> > -
> > - item = &descr->items[index];
> > -
> > - if (index < descr->nb_items - 1)
> > -
> >    RtlMoveMemory(item + 1, item,
> > -
> >                  (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA));
> > - item->str      = str;
> > - item->data     = HAS_STRINGS(descr) ? 0 : data;
> > - item->height   = 0;
> > - item->selected = FALSE;
> > -
> > - /* 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 );
> > - }
>
> Let's leave the OWNERDRAWVARIABLE bit in InsertItem.  insert_item_data() should
> just be about manipulating the items array.  You'll want to add a set_item_height()
> helper first.
>
> Huw.

LBS_OWNERDRAWVARIABLE is incompatible with LBS_NODATA (and disabled when 
listbox is created, due to OWNERDRAWFIXED which is mandatory), should I 
still add a set_item_height helper? It's not really needed in this case.

Besides, even if we assume that it's not incompatible, it sounds weird 
to send the WM_MEASUREITEM message and not do anything with the result 
in that situation (the helper would do nothing with nodata, right?).

Of course, it doesn't happen in practice anyway but... :-)

[Huw Davies]

On Tue, Feb 19, 2019 at 12:47:07PM +0200, Gabriel Ivăncescu wrote:
> On 2/19/19 12:14 PM, Huw Davies wrote:
> > On Mon, Feb 18, 2019 at 03:47:51PM +0200, Gabriel Ivăncescu wrote:
> > > Signed-off-by: Gabriel Ivăncescu
> > >
> > > This is a no-op patch. nb_items is incremented before the call and this is
> > > taken care of in the helper function. This is for two reasons:
> > > - This simplifies the nodata patch (only adds a single line to
> > >
> > > insert_item_data).
> > > 2) This matches what RemoveItem does, so it's more consistent.
> > >
> > > dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++-------------------
> > >   1 file changed, 33 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > index 50ff5c5..330d3ae 100644
> > > --- a/dlls/comctl32/listbox.c
> > > +++ b/dlls/comctl32/listbox.c
> > > @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state)
> > >           descr->items[index].selected = state;
> > >   }
> > > +static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data)
> > > +{
> > > - LB_ITEMDATA *item;
> > > -
> > > - item = &descr->items[index];
> > > -
> > > - if (index < descr->nb_items - 1)
> > > -
> > >    RtlMoveMemory(item + 1, item,
> > > -
> > >                  (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA));
> > > - item->str      = str;
> > > - item->data     = HAS_STRINGS(descr) ? 0 : data;
> > > - item->height   = 0;
> > > - item->selected = FALSE;
> > > -
> > > - /* 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 );
> > > - }
> >
> > Let's leave the OWNERDRAWVARIABLE bit in InsertItem.  insert_item_data() should
> > just be about manipulating the items array.  You'll want to add a set_item_height()
> > helper first.
> >
> > Huw.
>
> LBS_OWNERDRAWVARIABLE is incompatible with LBS_NODATA (and disabled when
> listbox is created, due to OWNERDRAWFIXED which is mandatory), should I
> still add a set_item_height helper? It's not really needed in this case.

Yes, I know this.

At the risk of repeating myself (but apparently I have to), the aim is
to get rid of direct accesses to item, outside of the item g/setters.

Huw.