[PATCH 1/5] comctl32/listbox: Resize the entire item array at once in SetCount

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

This has been carefully done so that it matches the previous behavior when
LBS_NODATA was used, since SetCount only works with it (so that means it
also implies LBS_OWNERDRAWFIXED and so on).

count was changed to unsigned so that it passes the last test in the patch
series, apparently Windows treats it as unsigned.

 dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index c92c2a2..787fc88 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -1742,24 +1742,47 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
 /***********************************************************************
  *           LISTBOX_SetCount
  */
-static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
+static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
 {
-    LRESULT ret;
+    UINT orig_num = descr->nb_items;
 
     if (!(descr->style & LBS_NODATA)) return LB_ERR;
 
-    /* FIXME: this is far from optimal... */
-    if (count > descr->nb_items)
+    if (count > orig_num)
     {
-        while (count > descr->nb_items)
-            if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)
-                return ret;
+        if (!resize_storage(descr, count))
+            return LB_ERRSPACE;
+        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
+        descr->nb_items = count;
+
+        LISTBOX_UpdateScroll(descr);
+        LISTBOX_InvalidateItems(descr, orig_num);
+
+        /* If listbox was empty, set focus to the first item */
+        if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE);
     }
-    else if (count < descr->nb_items)
+    else if (count < orig_num)
     {
-        while (count < descr->nb_items)
-            if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)
-                return ret;
+        LISTBOX_InvalidateItems(descr, count);
+        descr->nb_items = count;
+
+        if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
+        else
+        {
+            descr->anchor_item = min(descr->anchor_item, count - 1);
+
+            resize_storage(descr, count);
+            if (descr->selected_item >= count)
+                descr->selected_item = -1;
+
+            LISTBOX_UpdateScroll(descr);
+
+            /* If we removed the scrollbar, reset the top of the list */
+            if (count <= descr->page_size && orig_num > descr->page_size)
+                LISTBOX_SetTopItem(descr, 0, TRUE);
+
+            descr->focus_item = min(descr->focus_item, count - 1);
+        }
     }
 
     InvalidateRect( descr->self, NULL, TRUE );
-- 
2.19.1

[PATCH 2/5] comctl32/listbox: Use descr->selected_item for single-selection listboxes in is_item_selected

[Gabriel Ivăncescu]

Needed for LBS_NODATA.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/comctl32/listbox.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 787fc88..d115617 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -149,6 +149,8 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
 
 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;
 }
 
-- 
2.19.1

[PATCH 3/5] comctl32/tests: Test changing the selection styles after listbox creation

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/comctl32/tests/listbox.c | 130 ++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
index 68e4073..0cdcdea 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -784,6 +784,135 @@ static void test_listbox_height(void)
     DestroyWindow( hList );
 }
 
+static void test_changing_selection_styles(void)
+{
+    static const DWORD styles[] =
+    {
+        0,
+        LBS_NODATA | LBS_OWNERDRAWFIXED
+    };
+    static const DWORD selstyles[] =
+    {
+        0,
+        LBS_MULTIPLESEL,
+        LBS_EXTENDEDSEL,
+        LBS_MULTIPLESEL | LBS_EXTENDEDSEL
+    };
+    static const LONG selexpect_single[]  = { 0, 0, 1 };
+    static const LONG selexpect_single2[] = { 1, 0, 0 };
+    static const LONG selexpect_multi[]   = { 1, 0, 1 };
+    static const LONG selexpect_multi2[]  = { 1, 1, 0 };
+
+    HWND parent, listbox;
+    DWORD style;
+    LONG ret;
+    UINT i, j, k;
+
+    parent = create_parent();
+    ok(parent != NULL, "Failed to create parent window.\n");
+    for (i = 0; i < ARRAY_SIZE(styles); i++)
+    {
+        /* Test if changing selection styles affects selection storage */
+        for (j = 0; j < ARRAY_SIZE(selstyles); j++)
+        {
+            LONG setcursel_expect, selitemrange_expect, getselcount_expect;
+            const LONG *selexpect;
+
+            listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | selstyles[j] | WS_CHILD | WS_VISIBLE,
+                                    0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0);
+            ok(listbox != NULL, "%u: Failed to create ListBox window.\n", j);
+
+            if (selstyles[j] & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
+            {
+                setcursel_expect = LB_ERR;
+                selitemrange_expect = LB_OKAY;
+                getselcount_expect = 2;
+                selexpect = selexpect_multi;
+            }
+            else
+            {
+                setcursel_expect = 2;
+                selitemrange_expect = LB_ERR;
+                getselcount_expect = LB_ERR;
+                selexpect = selexpect_single;
+            }
+
+            for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++)
+            {
+                ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"x");
+                ok(ret == k, "%u: Unexpected return value %d, expected %d.\n", j, ret, k);
+            }
+            ret = SendMessageA(listbox, LB_GETCOUNT, 0, 0);
+            ok(ret == ARRAY_SIZE(selexpect_multi), "%u: Unexpected count %d.\n", j, ret);
+
+            /* Select items with different methods */
+            ret = SendMessageA(listbox, LB_SETCURSEL, 2, 0);
+            ok(ret == setcursel_expect, "%u: Unexpected return value %d.\n", j, ret);
+            ret = SendMessageA(listbox, LB_SELITEMRANGE, TRUE, MAKELPARAM(0, 0));
+            ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret);
+            ret = SendMessageA(listbox, LB_SELITEMRANGE, TRUE, MAKELPARAM(2, 2));
+            ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret);
+
+            /* Verify that the proper items are selected */
+            for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++)
+            {
+                ret = SendMessageA(listbox, LB_GETSEL, k, 0);
+                ok(ret == selexpect[k], "%u: Unexpected selection state %d, expected %d.\n",
+                    j, ret, selexpect[k]);
+            }
+
+            /* Now change the selection style */
+            style = GetWindowLongA(listbox, GWL_STYLE);
+            ok((style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == selstyles[j],
+                "%u: unexpected window styles %#x.\n", j, style);
+            if (selstyles[j] & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
+                style &= ~selstyles[j];
+            else
+                style |= LBS_MULTIPLESEL | LBS_EXTENDEDSEL;
+            SetWindowLongA(listbox, GWL_STYLE, style);
+            style = GetWindowLongA(listbox, GWL_STYLE);
+            ok(!(style & selstyles[j]), "%u: unexpected window styles %#x.\n", j, style);
+
+            /* Verify that the same items are selected */
+            ret = SendMessageA(listbox, LB_GETSELCOUNT, 0, 0);
+            ok(ret == getselcount_expect, "%u: expected %d from LB_GETSELCOUNT, got %d\n",
+                j, getselcount_expect, ret);
+
+            for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++)
+            {
+                ret = SendMessageA(listbox, LB_GETSEL, k, 0);
+                ok(ret == selexpect[k], "%u: Unexpected selection state %d, expected %d.\n",
+                    j, ret, selexpect[k]);
+            }
+
+            /* Lastly see if we can still change the selection as before with old style */
+            if (setcursel_expect != LB_ERR) setcursel_expect = 0;
+            ret = SendMessageA(listbox, LB_SETCURSEL, 0, 0);
+            ok(ret == setcursel_expect, "%u: Unexpected return value %d.\n", j, ret);
+            ret = SendMessageA(listbox, LB_SELITEMRANGE, TRUE, MAKELPARAM(1, 1));
+            ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret);
+            ret = SendMessageA(listbox, LB_SELITEMRANGE, FALSE, MAKELPARAM(2, 2));
+            ok(ret == selitemrange_expect, "%u: Unexpected return value %d.\n", j, ret);
+
+            /* And verify the selections */
+            selexpect = (selstyles[j] & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) ? selexpect_multi2 : selexpect_single2;
+            ret = SendMessageA(listbox, LB_GETSELCOUNT, 0, 0);
+            ok(ret == getselcount_expect, "%u: expected %d from LB_GETSELCOUNT, got %d\n",
+                j, getselcount_expect, ret);
+
+            for (k = 0; k < ARRAY_SIZE(selexpect_multi); k++)
+            {
+                ret = SendMessageA(listbox, LB_GETSEL, k, 0);
+                ok(ret == selexpect[k], "%u: Unexpected selection state %d, expected %d.\n",
+                    j, ret, selexpect[k]);
+            }
+
+            DestroyWindow(listbox);
+        }
+    }
+    DestroyWindow(parent);
+}
+
 static void test_itemfrompoint(void)
 {
     /* WS_POPUP is required in order to have a more accurate size calculation (
@@ -2483,6 +2612,7 @@ START_TEST(listbox)
     test_LB_SELITEMRANGE();
     test_LB_SETCURSEL();
     test_listbox_height();
+    test_changing_selection_styles();
     test_itemfrompoint();
     test_listbox_item_data();
     test_listbox_LB_DIR();
-- 
2.19.1

[PATCH 4/5] 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 <gabrielopcode(a)gmail.com>
---

Most of the changes are just indentation in InsertItem/RemoveItem.

 dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++--------------
 1 file changed, 90 insertions(+), 50 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index d115617..fd3bfa6 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -19,7 +19,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
  *
  * TODO:
- *    - LBS_NODATA
+ *    - LBS_NODATA for multi-selection listboxes
  */
 
 #include <string.h>
@@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index )
     return descr->items[index].selected;
 }
 
+static BOOL is_singlesel_NODATA(const LB_DESCR *descr)
+{
+    return (descr->style & LBS_NODATA) &&
+           !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));
+}
+
 /***********************************************************************
  *           LISTBOX_GetCurrentPageSize
  *
@@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
         GetClientRect(descr->self, &r);
         hrgn = set_control_clipping( hdc, &r );
 
+        if (is_singlesel_NODATA(descr)) item = NULL;
         dis.CtlType      = ODT_LISTBOX;
         dis.CtlID        = GetWindowLongPtrW( descr->self, GWLP_ID );
         dis.hwndItem     = descr->self;
@@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
 {
     UINT new_size = descr->nb_items + nb_items;
 
-    if (new_size > descr->items_size && !resize_storage(descr, new_size))
-        return LB_ERRSPACE;
+    /* Windows keeps track of (unaligned) reserved space
+       for LBS_NODATA, despite the fact it is useless */
+    if (new_size > descr->items_size)
+    {
+        if (is_singlesel_NODATA(descr))
+            descr->items_size = new_size;
+        else if (!resize_storage(descr, new_size))
+            return LB_ERRSPACE;
+    }
     return descr->items_size;
 }
 
@@ -1460,10 +1474,13 @@ static LRESULT LISTBOX_SetSelection( LB_DESCR *descr, INT index,
     {
         INT oldsel = descr->selected_item;
         if (index == oldsel) return LB_OKAY;
-        if (oldsel != -1) descr->items[oldsel].selected = FALSE;
-        if (index != -1) descr->items[index].selected = TRUE;
-        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
+        if (!(descr->style & LBS_NODATA))
+        {
+            if (oldsel != -1) descr->items[oldsel].selected = FALSE;
+            if (index != -1) descr->items[index].selected = TRUE;
+        }
         descr->selected_item = index;
+        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
         if (index != -1) LISTBOX_RepaintItem( descr, index, ODA_SELECT );
         if (send_notify && descr->nb_items) SEND_NOTIFICATION( descr,
                                (index != -1) ? LBN_SELCHANGE : LBN_SELCANCEL );
@@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
 
     if (index == -1) index = descr->nb_items;
     else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
-    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
-
-    /* Insert the item structure */
-
-    item = &descr->items[index];
-    if (index < descr->nb_items)
-        RtlMoveMemory( item + 1, item,
-                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
-    item->str      = str;
-    item->data     = HAS_STRINGS(descr) ? 0 : data;
-    item->height   = 0;
-    item->selected = FALSE;
-    descr->nb_items++;
-
-    /* Get item height */
-
-    if (descr->style & LBS_OWNERDRAWVARIABLE)
+    if (is_singlesel_NODATA(descr))
     {
-        MEASUREITEMSTRUCT mis;
-        UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
+        descr->nb_items++;
+        descr->items_size = max(descr->items_size, descr->nb_items);
+    }
+    else
+    {
+        if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
+
+        /* Insert the item structure */
+        item = &descr->items[index];
+        if (index < descr->nb_items)
+            RtlMoveMemory( item + 1, item,
+                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
+        item->str      = str;
+        item->data     = HAS_STRINGS(descr) ? 0 : data;
+        item->height   = 0;
+        item->selected = FALSE;
+        descr->nb_items++;
+
+        /* Get item height */
+        if (descr->style & LBS_OWNERDRAWVARIABLE)
+        {
+            MEASUREITEMSTRUCT mis;
+            UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
 
-        mis.CtlType    = ODT_LISTBOX;
-        mis.CtlID      = id;
-        mis.itemID     = index;
-        mis.itemData   = data;
-        mis.itemHeight = descr->item_height;
-        SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
-        item->height = mis.itemHeight ? mis.itemHeight : 1;
-        TRACE("[%p]: measure item %d (%s) = %d\n",
-              descr->self, index, str ? debugstr_w(str) : "", item->height );
+            mis.CtlType    = ODT_LISTBOX;
+            mis.CtlID      = id;
+            mis.itemID     = index;
+            mis.itemData   = data;
+            mis.itemHeight = descr->item_height;
+            SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
+            item->height = mis.itemHeight ? mis.itemHeight : 1;
+            TRACE("[%p]: measure item %d (%s) = %d\n",
+                  descr->self, index, str ? debugstr_w(str) : "", item->height );
+        }
     }
 
     /* Repaint the items */
@@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
     LISTBOX_InvalidateItems( descr, index );
 
     descr->nb_items--;
-    LISTBOX_DeleteItem( descr, index );
-
-    if (!descr->nb_items) return LB_OKAY;
+    if (is_singlesel_NODATA(descr))
+    {
+        if (!descr->nb_items)
+        {
+            SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
+            return LB_OKAY;
+        }
+    }
+    else
+    {
+        LISTBOX_DeleteItem( descr, index );
 
-    /* Remove the item */
+        if (!descr->nb_items) return LB_OKAY;
 
-    item = &descr->items[index];
-    if (index < descr->nb_items)
-        RtlMoveMemory( item, item + 1,
-                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
-    if (descr->anchor_item == descr->nb_items) descr->anchor_item--;
+        /* Remove the item */
+        item = &descr->items[index];
+        if (index < descr->nb_items)
+            RtlMoveMemory( item, item + 1,
+                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
 
-    resize_storage(descr, descr->nb_items);
+        resize_storage(descr, descr->nb_items);
+    }
+    descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
 
     /* Repaint the items */
 
@@ -1729,7 +1762,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
 {
     INT i;
 
-    for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
+    if (descr->items)
+        for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
     HeapFree( GetProcessHeap(), 0, descr->items );
     descr->nb_items      = 0;
     descr->top_item      = 0;
@@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
 
     if (count > orig_num)
     {
-        if (!resize_storage(descr, count))
-            return LB_ERRSPACE;
-        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
+        if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
+        {
+            if (!resize_storage(descr, count))
+                return LB_ERRSPACE;
+            memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
+        }
+        else
+            descr->items_size = max(descr->items_size, count);
         descr->nb_items = count;
 
         LISTBOX_UpdateScroll(descr);
@@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
         {
             descr->anchor_item = min(descr->anchor_item, count - 1);
 
-            resize_storage(descr, count);
-            if (descr->selected_item >= count)
+            if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
+                resize_storage(descr, count);
+            else if (descr->selected_item >= descr->nb_items)
                 descr->selected_item = -1;
 
             LISTBOX_UpdateScroll(descr);
-- 
2.19.1

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

[Huw Davies]

On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
> The LBS_NODATA style's purpose is to drastically improve performance and
> memory usage on very large lists, since they should function as virtual
> lists. Thus, don't store any data for single-selection listboxes (descr->items
> always NULL).
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>
> Most of the changes are just indentation in InsertItem/RemoveItem.
>
> dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++--------------
>  1 file changed, 90 insertions(+), 50 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index d115617..fd3bfa6 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -19,7 +19,7 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>   *
>   * TODO:
> - *    - LBS_NODATA
> + *    - LBS_NODATA for multi-selection listboxes
>   */
>
> #include <string.h>
> @@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index )
>      return descr->items[index].selected;
>  }
>
> +static BOOL is_singlesel_NODATA(const LB_DESCR *descr)
> +{
> +    return (descr->style & LBS_NODATA) &&
> +           !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));
> +}
> +

There are way too many is_singlesel_NODATA() special cases in this patch.
Instead of that, we should add more helpers like is_item_selected().
I've given some examples below.
> /***********************************************************************
>   *           LISTBOX_GetCurrentPageSize
>   *
> @@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
>          GetClientRect(descr->self, &r);
>          hrgn = set_control_clipping( hdc, &r );
>
> +        if (is_singlesel_NODATA(descr)) item = NULL;

This is used to cover item->data (and item->str in the TRACE).
So introduce a get_item_data() and get_item_string() helper
that returns NULL in the no_data case.  Patches to add those
helpers obviously come before this patch.
> dis.CtlType      = ODT_LISTBOX;
>          dis.CtlID        = GetWindowLongPtrW( descr->self, GWLP_ID );
>          dis.hwndItem     = descr->self;
> @@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
>  {
>      UINT new_size = descr->nb_items + nb_items;
>
> -    if (new_size > descr->items_size && !resize_storage(descr, new_size))
> -        return LB_ERRSPACE;
> +    /* Windows keeps track of (unaligned) reserved space
> +       for LBS_NODATA, despite the fact it is useless */
> +    if (new_size > descr->items_size)
> +    {
> +        if (is_singlesel_NODATA(descr))
> +            descr->items_size = new_size;
> +        else if (!resize_storage(descr, new_size))
> +            return LB_ERRSPACE;
> +    }

Have resize_storage() handle no_data, it'll fix up ->items_size too, so there
should be no need to change this bit of code.
> return descr->items_size;
>  }
>
> @@ -1460,10 +1474,13 @@ static LRESULT LISTBOX_SetSelection( LB_DESCR *descr, INT index,
>      {
>          INT oldsel = descr->selected_item;
>          if (index == oldsel) return LB_OKAY;
> -        if (oldsel != -1) descr->items[oldsel].selected = FALSE;
> -        if (index != -1) descr->items[index].selected = TRUE;
> -        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
> +        if (!(descr->style & LBS_NODATA))
> +        {
> +            if (oldsel != -1) descr->items[oldsel].selected = FALSE;
> +            if (index != -1) descr->items[index].selected = TRUE;
> +        }

Add a helper to set an item's selection state.
> descr->selected_item = index;
> +        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
>          if (index != -1) LISTBOX_RepaintItem( descr, index, ODA_SELECT );
>          if (send_notify && descr->nb_items) SEND_NOTIFICATION( descr,
>                                 (index != -1) ? LBN_SELCHANGE : LBN_SELCANCEL );
> @@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
>
> if (index == -1) index = descr->nb_items;
>      else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
> -    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> -
> -    /* Insert the item structure */
> -
> -    item = &descr->items[index];
> -    if (index < descr->nb_items)
> -        RtlMoveMemory( item + 1, item,
> -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> -    item->str      = str;
> -    item->data     = HAS_STRINGS(descr) ? 0 : data;
> -    item->height   = 0;
> -    item->selected = FALSE;
> -    descr->nb_items++;
> -
> -    /* Get item height */
> -
> -    if (descr->style & LBS_OWNERDRAWVARIABLE)
> +    if (is_singlesel_NODATA(descr))
>      {
> -        MEASUREITEMSTRUCT mis;
> -        UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
> +        descr->nb_items++;
> +        descr->items_size = max(descr->items_size, descr->nb_items);
> +    }
> +    else
> +    {
> +        if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;

Again, resize_storage will handle the no_data case.  Then add a helper to
actually insert the item.
> +
> +        /* Insert the item structure */
> +        item = &descr->items[index];
> +        if (index < descr->nb_items)
> +            RtlMoveMemory( item + 1, item,
> +                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> +        item->str      = str;
> +        item->data     = HAS_STRINGS(descr) ? 0 : data;
> +        item->height   = 0;
> +        item->selected = FALSE;
> +        descr->nb_items++;
> +
> +        /* Get item height */
> +        if (descr->style & LBS_OWNERDRAWVARIABLE)
> +        {
> +            MEASUREITEMSTRUCT mis;
> +            UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
>
> -        mis.CtlType    = ODT_LISTBOX;
> -        mis.CtlID      = id;
> -        mis.itemID     = index;
> -        mis.itemData   = data;
> -        mis.itemHeight = descr->item_height;
> -        SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
> -        item->height = mis.itemHeight ? mis.itemHeight : 1;
> -        TRACE("[%p]: measure item %d (%s) = %d\n",
> -              descr->self, index, str ? debugstr_w(str) : "", item->height );
> +            mis.CtlType    = ODT_LISTBOX;
> +            mis.CtlID      = id;
> +            mis.itemID     = index;
> +            mis.itemData   = data;
> +            mis.itemHeight = descr->item_height;
> +            SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
> +            item->height = mis.itemHeight ? mis.itemHeight : 1;
> +            TRACE("[%p]: measure item %d (%s) = %d\n",
> +                  descr->self, index, str ? debugstr_w(str) : "", item->height );
> +        }
>      }
>
> /* Repaint the items */
> @@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
>      LISTBOX_InvalidateItems( descr, index );
>
> descr->nb_items--;
> -    LISTBOX_DeleteItem( descr, index );

LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT
as required.
> -
> -    if (!descr->nb_items) return LB_OKAY;
> +    if (is_singlesel_NODATA(descr))
> +    {
> +        if (!descr->nb_items)
> +        {
> +            SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> +            return LB_OKAY;
> +        }
> +    }
> +    else
> +    {
> +        LISTBOX_DeleteItem( descr, index );
>
> -    /* Remove the item */
> +        if (!descr->nb_items) return LB_OKAY;
>
> -    item = &descr->items[index];
> -    if (index < descr->nb_items)
> -        RtlMoveMemory( item, item + 1,
> -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> -    if (descr->anchor_item == descr->nb_items) descr->anchor_item--;
> +        /* Remove the item */
> +        item = &descr->items[index];
> +        if (index < descr->nb_items)
> +            RtlMoveMemory( item, item + 1,
> +                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
>
> -    resize_storage(descr, descr->nb_items);
> +        resize_storage(descr, descr->nb_items);
> +    }
> +    descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
>
> /* Repaint the items */
>
> @@ -1729,7 +1762,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
>  {
>      INT i;
>
> -    for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
> +    if (descr->items)
> +        for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);

LB_DeleteItems() will cope.
> HeapFree( GetProcessHeap(), 0, descr->items );
>      descr->nb_items      = 0;
>      descr->top_item      = 0;
> @@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
>
> if (count > orig_num)
>      {
> -        if (!resize_storage(descr, count))
> -            return LB_ERRSPACE;
> -        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> +        if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
> +        {
> +            if (!resize_storage(descr, count))
> +                return LB_ERRSPACE;
> +            memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> +        }
> +        else
> +            descr->items_size = max(descr->items_size, count);
>          descr->nb_items = count;
>
> LISTBOX_UpdateScroll(descr);
> @@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
>          {
>              descr->anchor_item = min(descr->anchor_item, count - 1);
>
> -            resize_storage(descr, count);
> -            if (descr->selected_item >= count)
> +            if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
> +                resize_storage(descr, count);
> +            else if (descr->selected_item >= descr->nb_items)
>                  descr->selected_item = -1;
>
> LISTBOX_UpdateScroll(descr);

And again resize_storage() will handle this.

Huw.

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

[Gabriel Ivăncescu]

On 2/11/19 12:10 PM, Huw Davies wrote:
> On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
> > The LBS_NODATA style's purpose is to drastically improve performance and
> > memory usage on very large lists, since they should function as virtual
> > lists. Thus, don't store any data for single-selection listboxes (descr->items
> > always NULL).
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > ---
> >
> > Most of the changes are just indentation in InsertItem/RemoveItem.
> >
> > dlls/comctl32/listbox.c | 140 ++++++++++++++++++++++++++--------------
> >   1 file changed, 90 insertions(+), 50 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index d115617..fd3bfa6 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -19,7 +19,7 @@
> >    * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> >    *
> >    * TODO:
> > - *    - LBS_NODATA
> > + *    - LBS_NODATA for multi-selection listboxes
> >    */
> >
> > #include <string.h>
> > @@ -154,6 +154,12 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index )
> >       return descr->items[index].selected;
> >   }
> >
> > +static BOOL is_singlesel_NODATA(const LB_DESCR *descr)
> > +{
> > +    return (descr->style & LBS_NODATA) &&
> > +           !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));
> > +}
> > +
>
> There are way too many is_singlesel_NODATA() special cases in this patch.
> Instead of that, we should add more helpers like is_item_selected().
> I've given some examples below.
> > /***********************************************************************
> >    *           LISTBOX_GetCurrentPageSize
> >    *
> > @@ -547,6 +553,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> >           GetClientRect(descr->self, &r);
> >           hrgn = set_control_clipping( hdc, &r );
> >
> > +        if (is_singlesel_NODATA(descr)) item = NULL;
>
> This is used to cover item->data (and item->str in the TRACE).
> So introduce a get_item_data() and get_item_string() helper
> that returns NULL in the no_data case.  Patches to add those
> helpers obviously come before this patch.

Okay will do that.
> > dis.CtlType      = ODT_LISTBOX;
> >           dis.CtlID        = GetWindowLongPtrW( descr->self, GWLP_ID );
> >           dis.hwndItem     = descr->self;
> > @@ -709,8 +716,15 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
> >   {
> >       UINT new_size = descr->nb_items + nb_items;
> >
> > -    if (new_size > descr->items_size && !resize_storage(descr, new_size))
> > -        return LB_ERRSPACE;
> > +    /* Windows keeps track of (unaligned) reserved space
> > +       for LBS_NODATA, despite the fact it is useless */
> > +    if (new_size > descr->items_size)
> > +    {
> > +        if (is_singlesel_NODATA(descr))
> > +            descr->items_size = new_size;
> > +        else if (!resize_storage(descr, new_size))
> > +            return LB_ERRSPACE;
> > +    }
>
> Have resize_storage() handle no_data, it'll fix up ->items_size too, so there
> should be no need to change this bit of code.
> > return descr->items_size;
> >   }
> >
> > @@ -1460,10 +1474,13 @@ static LRESULT LISTBOX_SetSelection( LB_DESCR *descr, INT index,
> >       {
> >           INT oldsel = descr->selected_item;
> >           if (index == oldsel) return LB_OKAY;
> > -        if (oldsel != -1) descr->items[oldsel].selected = FALSE;
> > -        if (index != -1) descr->items[index].selected = TRUE;
> > -        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
> > +        if (!(descr->style & LBS_NODATA))
> > +        {
> > +            if (oldsel != -1) descr->items[oldsel].selected = FALSE;
> > +            if (index != -1) descr->items[index].selected = TRUE;
> > +        }
>
> Add a helper to set an item's selection state.
> > descr->selected_item = index;
> > +        if (oldsel != -1) LISTBOX_RepaintItem( descr, oldsel, ODA_SELECT );
> >           if (index != -1) LISTBOX_RepaintItem( descr, index, ODA_SELECT );
> >           if (send_notify && descr->nb_items) SEND_NOTIFICATION( descr,
> >                                  (index != -1) ? LBN_SELCHANGE : LBN_SELCANCEL );
> > @@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
> >
> > if (index == -1) index = descr->nb_items;
> >       else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
> > -    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> > -
> > -    /* Insert the item structure */
> > -
> > -    item = &descr->items[index];
> > -    if (index < descr->nb_items)
> > -        RtlMoveMemory( item + 1, item,
> > -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> > -    item->str      = str;
> > -    item->data     = HAS_STRINGS(descr) ? 0 : data;
> > -    item->height   = 0;
> > -    item->selected = FALSE;
> > -    descr->nb_items++;
> > -
> > -    /* Get item height */
> > -
> > -    if (descr->style & LBS_OWNERDRAWVARIABLE)
> > +    if (is_singlesel_NODATA(descr))
> >       {
> > -        MEASUREITEMSTRUCT mis;
> > -        UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
> > +        descr->nb_items++;
> > +        descr->items_size = max(descr->items_size, descr->nb_items);
> > +    }
> > +    else
> > +    {
> > +        if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
>
> Again, resize_storage will handle the no_data case.  Then add a helper to
> actually insert the item.

But in the insert helper, I'd have to ignore everything if it's 
single-selection LBS_NODATA (because there's nothing to do), so there is 
no difference to currently except just moving most of the function into 
a helper (instead of indenting it).

Is that what you had in mind?
> > +
> > +        /* Insert the item structure */
> > +        item = &descr->items[index];
> > +        if (index < descr->nb_items)
> > +            RtlMoveMemory( item + 1, item,
> > +                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> > +        item->str      = str;
> > +        item->data     = HAS_STRINGS(descr) ? 0 : data;
> > +        item->height   = 0;
> > +        item->selected = FALSE;
> > +        descr->nb_items++;
> > +
> > +        /* Get item height */
> > +        if (descr->style & LBS_OWNERDRAWVARIABLE)
> > +        {
> > +            MEASUREITEMSTRUCT mis;
> > +            UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
> >
> > -        mis.CtlType    = ODT_LISTBOX;
> > -        mis.CtlID      = id;
> > -        mis.itemID     = index;
> > -        mis.itemData   = data;
> > -        mis.itemHeight = descr->item_height;
> > -        SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
> > -        item->height = mis.itemHeight ? mis.itemHeight : 1;
> > -        TRACE("[%p]: measure item %d (%s) = %d\n",
> > -              descr->self, index, str ? debugstr_w(str) : "", item->height );
> > +            mis.CtlType    = ODT_LISTBOX;
> > +            mis.CtlID      = id;
> > +            mis.itemID     = index;
> > +            mis.itemData   = data;
> > +            mis.itemHeight = descr->item_height;
> > +            SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis );
> > +            item->height = mis.itemHeight ? mis.itemHeight : 1;
> > +            TRACE("[%p]: measure item %d (%s) = %d\n",
> > +                  descr->self, index, str ? debugstr_w(str) : "", item->height );
> > +        }
> >       }
> >
> > /* Repaint the items */
> > @@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
> >       LISTBOX_InvalidateItems( descr, index );
> >
> > descr->nb_items--;
> > -    LISTBOX_DeleteItem( descr, index );
>
> LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT
> as required.

Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called 
with LBS_NODATA (unless you simply mean an early exit if so) nor send 
any messages at all.
> > -
> > -    if (!descr->nb_items) return LB_OKAY;
> > +    if (is_singlesel_NODATA(descr))
> > +    {
> > +        if (!descr->nb_items)
> > +        {
> > +            SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> > +            return LB_OKAY;
> > +        }
> > +    }
> > +    else
> > +    {
> > +        LISTBOX_DeleteItem( descr, index );
> >
> > -    /* Remove the item */
> > +        if (!descr->nb_items) return LB_OKAY;
> >
> > -    item = &descr->items[index];
> > -    if (index < descr->nb_items)
> > -        RtlMoveMemory( item, item + 1,
> > -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> > -    if (descr->anchor_item == descr->nb_items) descr->anchor_item--;
> > +        /* Remove the item */
> > +        item = &descr->items[index];
> > +        if (index < descr->nb_items)
> > +            RtlMoveMemory( item, item + 1,
> > +                           (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> >
> > -    resize_storage(descr, descr->nb_items);
> > +        resize_storage(descr, descr->nb_items);
> > +    }
> > +    descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1);
> >
> > /* Repaint the items */
> >
> > @@ -1729,7 +1762,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
> >   {
> >       INT i;
> >
> > -    for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
> > +    if (descr->items)
> > +        for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
>
> LB_DeleteItems() will cope.

I don't like this solution. That would be slower than needed for large 
lists (which LBS_NODATA is for) when resetting them, for an operation 
that should basically be instant and not dependent on the list's size. 
i.e. from O(1) to O(n)
> > HeapFree( GetProcessHeap(), 0, descr->items );
> >       descr->nb_items      = 0;
> >       descr->top_item      = 0;
> > @@ -1752,9 +1786,14 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
> >
> > if (count > orig_num)
> >       {
> > -        if (!resize_storage(descr, count))
> > -            return LB_ERRSPACE;
> > -        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> > +        if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
> > +        {
> > +            if (!resize_storage(descr, count))
> > +                return LB_ERRSPACE;
> > +            memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> > +        }
> > +        else
> > +            descr->items_size = max(descr->items_size, count);
> >           descr->nb_items = count;
> >
> > LISTBOX_UpdateScroll(descr);
> > @@ -1773,8 +1812,9 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
> >           {
> >               descr->anchor_item = min(descr->anchor_item, count - 1);
> >
> > -            resize_storage(descr, count);
> > -            if (descr->selected_item >= count)
> > +            if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL))
> > +                resize_storage(descr, count);
> > +            else if (descr->selected_item >= descr->nb_items)
> >                   descr->selected_item = -1;
> >
> > LISTBOX_UpdateScroll(descr);
>
> And again resize_storage() will handle this.
>
> Huw.

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

[Huw Davies]

On Mon, Feb 11, 2019 at 01:09:14PM +0200, Gabriel Ivăncescu wrote:
> On 2/11/19 12:10 PM, Huw Davies wrote:
> > On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
> > > @@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
> > >       if (index == -1) index = descr->nb_items;
> > >       else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
> > > -    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> > > -
> > > -    /* Insert the item structure */
> > > -
> > > -    item = &descr->items[index];
> > > -    if (index < descr->nb_items)
> > > -        RtlMoveMemory( item + 1, item,
> > > -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> > > -    item->str      = str;
> > > -    item->data     = HAS_STRINGS(descr) ? 0 : data;
> > > -    item->height   = 0;
> > > -    item->selected = FALSE;
> > > -    descr->nb_items++;
> > > -
> > > -    /* Get item height */
> > > -
> > > -    if (descr->style & LBS_OWNERDRAWVARIABLE)
> > > +    if (is_singlesel_NODATA(descr))
> > >       {
> > > -        MEASUREITEMSTRUCT mis;
> > > -        UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
> > > +        descr->nb_items++;
> > > +        descr->items_size = max(descr->items_size, descr->nb_items);
> > > +    }
> > > +    else
> > > +    {
> > > +        if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> >
> > Again, resize_storage will handle the no_data case.  Then add a helper to
> > actually insert the item.
>
> But in the insert helper, I'd have to ignore everything if it's
> single-selection LBS_NODATA (because there's nothing to do), so there is no
> difference to currently except just moving most of the function into a
> helper (instead of indenting it).
>
> Is that what you had in mind?

Yes.
> > > @@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
> > >       LISTBOX_InvalidateItems( descr, index );
> > >       descr->nb_items--;
> > > -    LISTBOX_DeleteItem( descr, index );
> >
> > LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT
> > as required.
>
> Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called with
> LBS_NODATA (unless you simply mean an early exit if so) nor send any
> messages at all.

It's an earlish exit after sending the LB_RESETCONTENT message if appropriate.
> > > @@ -1729,7 +1762,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
> > >   {
> > >       INT i;
> > > -    for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
> > > +    if (descr->items)
> > > +        for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
> >
> > LB_DeleteItems() will cope.
>
> I don't like this solution. That would be slower than needed for large lists
> (which LBS_NODATA is for) when resetting them, for an operation that should
> basically be instant and not dependent on the list's size. i.e. from O(1) to
> O(n)

We'll revisit it if performance turns out to be an issue.

Huw.

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

[Gabriel Ivăncescu]

On 2/11/19 1:22 PM, Huw Davies wrote:
> On Mon, Feb 11, 2019 at 01:09:14PM +0200, Gabriel Ivăncescu wrote:
> > On 2/11/19 12:10 PM, Huw Davies wrote:
> > > On Fri, Feb 08, 2019 at 02:41:45PM +0200, Gabriel Ivăncescu wrote:
> > > > @@ -1535,36 +1552,42 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
> > > >        if (index == -1) index = descr->nb_items;
> > > >        else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
> > > > -    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> > > > -
> > > > -    /* Insert the item structure */
> > > > -
> > > > -    item = &descr->items[index];
> > > > -    if (index < descr->nb_items)
> > > > -        RtlMoveMemory( item + 1, item,
> > > > -                       (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
> > > > -    item->str      = str;
> > > > -    item->data     = HAS_STRINGS(descr) ? 0 : data;
> > > > -    item->height   = 0;
> > > > -    item->selected = FALSE;
> > > > -    descr->nb_items++;
> > > > -
> > > > -    /* Get item height */
> > > > -
> > > > -    if (descr->style & LBS_OWNERDRAWVARIABLE)
> > > > +    if (is_singlesel_NODATA(descr))
> > > >        {
> > > > -        MEASUREITEMSTRUCT mis;
> > > > -        UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID );
> > > > +        descr->nb_items++;
> > > > +        descr->items_size = max(descr->items_size, descr->nb_items);
> > > > +    }
> > > > +    else
> > > > +    {
> > > > +        if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;
> > >
> > > Again, resize_storage will handle the no_data case.  Then add a helper to
> > > actually insert the item.
> >
> > But in the insert helper, I'd have to ignore everything if it's
> > single-selection LBS_NODATA (because there's nothing to do), so there is no
> > difference to currently except just moving most of the function into a
> > helper (instead of indenting it).
> >
> > Is that what you had in mind?
>
> Yes.
> > > > @@ -1678,19 +1701,29 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
> > > >        LISTBOX_InvalidateItems( descr, index );
> > > >        descr->nb_items--;
> > > > -    LISTBOX_DeleteItem( descr, index );
> > >
> > > LB_DeleteItem should again handle the no_data case, it'll send LB_RESETCONTENT
> > > as required.
> >
> > Shouldn't that be RemoveItem? DeleteItem, AFAIK, has to not be called with
> > LBS_NODATA (unless you simply mean an early exit if so) nor send any
> > messages at all.
>
> It's an earlish exit after sending the LB_RESETCONTENT message if appropriate.
> > > > @@ -1729,7 +1762,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
> > > >    {
> > > >        INT i;
> > > > -    for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
> > > > +    if (descr->items)
> > > > +        for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
> > >
> > > LB_DeleteItems() will cope.
> >
> > I don't like this solution. That would be slower than needed for large lists
> > (which LBS_NODATA is for) when resetting them, for an operation that should
> > basically be instant and not dependent on the list's size. i.e. from O(1) to
> > O(n)
>
> We'll revisit it if performance turns out to be an issue.
>
> Huw.

Well, I find it awkward that sending SetCount to zero first would be far 
faster than LB_RESETCONTENT directly (that's what this will turn out to 
be), or that "growing" the list is instant while ResetContent is not. 
And it's just one line of code, I think when reducing O(n) to O(1) it 
should be worth it, given the point of LBS_NODATA.

(also, a variant of that line of code will also help with 
multi-selection listboxes later, which can also be instant)

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

[Gabriel Ivăncescu]

Ok, I have a question on how to proceed now, after looking through and 
finishing the rework.

DeleteItem saves the item data & string at the beginning, *before* 
ResetContent. It has to.

If I special-case LBS_NODATA inside of it, I'll have to do it *twice* 
(one before saving, one after ResetContent), or duplicate ResetContent, 
which is pretty ugly.

So, with this in mind, should I still special case it inside of 
DeleteItem instead of RemoveItem? (or a helper inside RemoveItem, as per 
InsertItem).

This will also fix the O(n) issue. And in the end there will be the same 
amount of checks in the code. I think this is how I should proceed but I 
want to be sure before I send it in later if you don't want it.

Summary: there will be one check in RemoveItem's helper (like 
InsertItem) and one in ResetContent. Instead of having two checks in 
DeleteItem (and the O(n) thing).

Thoughts?

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

[Huw Davies]

On Mon, Feb 11, 2019 at 05:29:58PM +0200, Gabriel Ivăncescu wrote:
> Ok, I have a question on how to proceed now, after looking through and
> finishing the rework.
>
> DeleteItem saves the item data & string at the beginning, *before*
> ResetContent. It has to.
>
> If I special-case LBS_NODATA inside of it, I'll have to do it *twice* (one
> before saving, one after ResetContent), or duplicate ResetContent, which is
> pretty ugly.
>
> So, with this in mind, should I still special case it inside of DeleteItem
> instead of RemoveItem? (or a helper inside RemoveItem, as per InsertItem).
>
> This will also fix the O(n) issue. And in the end there will be the same
> amount of checks in the code. I think this is how I should proceed but I
> want to be sure before I send it in later if you don't want it.
>
> Summary: there will be one check in RemoveItem's helper (like InsertItem)
> and one in ResetContent. Instead of having two checks in DeleteItem (and the
> O(n) thing).

It's going to be easier to review some actual code.  So do it in the way
you think best follows the spirit of what I was asking and we can take it
from there.

Huw.

[PATCH 5/5] comctl32/tests: Test negative SetCount for listboxes

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 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)
-- 
2.19.1

[Huw Davies]

On Fri, Feb 08, 2019 at 02:41:42PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>
> This has been carefully done so that it matches the previous behavior when
> LBS_NODATA was used, since SetCount only works with it (so that means it
> also implies LBS_OWNERDRAWFIXED and so on).
>
> count was changed to unsigned so that it passes the last test in the patch
> series, apparently Windows treats it as unsigned.
>
> dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index c92c2a2..787fc88 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -1742,24 +1742,47 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
>  /***********************************************************************
>   *           LISTBOX_SetCount
>   */
> -static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
> +static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
>  {
> -    LRESULT ret;
> +    UINT orig_num = descr->nb_items;
>
> if (!(descr->style & LBS_NODATA)) return LB_ERR;
>
> -    /* FIXME: this is far from optimal... */
> -    if (count > descr->nb_items)
> +    if (count > orig_num)
>      {
> -        while (count > descr->nb_items)
> -            if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)
> -                return ret;
> +        if (!resize_storage(descr, count))
> +            return LB_ERRSPACE;
> +        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> +        descr->nb_items = count;
> +
> +        LISTBOX_UpdateScroll(descr);
> +        LISTBOX_InvalidateItems(descr, orig_num);
> +
> +        /* If listbox was empty, set focus to the first item */
> +        if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE);
>      }
> -    else if (count < descr->nb_items)
> +    else if (count < orig_num)
>      {
> -        while (count < descr->nb_items)
> -            if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)
> -                return ret;
> +        LISTBOX_InvalidateItems(descr, count);
> +        descr->nb_items = count;
> +
> +        if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> +        else
> +        {
> +            descr->anchor_item = min(descr->anchor_item, count - 1);
> +
> +            resize_storage(descr, count);
> +            if (descr->selected_item >= count)
> +                descr->selected_item = -1;
> +
> +            LISTBOX_UpdateScroll(descr);
> +
> +            /* If we removed the scrollbar, reset the top of the list */
> +            if (count <= descr->page_size && orig_num > descr->page_size)
> +                LISTBOX_SetTopItem(descr, 0, TRUE);
> +
> +            descr->focus_item = min(descr->focus_item, count - 1);
> +        }
>      }
>
> InvalidateRect( descr->self, NULL, TRUE );

This could be a lot simpler.  For a start, we don't need the calls to
LB_InvalidateItems() since we have the InvalidateRect() call at the
end.  Also, much of the code in the two cases (shrink or grow) is the
same and so could be merged.  (I realise you've basically inlined the
code that was called before, but we can do better than that).

It probably also makes sense to have resize_storage() perform the
zero-init.

Huw.

[Gabriel Ivăncescu]

On 2/11/19 11:54 AM, Huw Davies wrote:
> On Fri, Feb 08, 2019 at 02:41:42PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > ---
> >
> > This has been carefully done so that it matches the previous behavior when
> > LBS_NODATA was used, since SetCount only works with it (so that means it
> > also implies LBS_OWNERDRAWFIXED and so on).
> >
> > count was changed to unsigned so that it passes the last test in the patch
> > series, apparently Windows treats it as unsigned.
> >
> > dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++----------
> >   1 file changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index c92c2a2..787fc88 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -1742,24 +1742,47 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
> >   /***********************************************************************
> >    *           LISTBOX_SetCount
> >    */
> > -static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
> > +static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
> >   {
> > -    LRESULT ret;
> > +    UINT orig_num = descr->nb_items;
> >
> > if (!(descr->style & LBS_NODATA)) return LB_ERR;
> >
> > -    /* FIXME: this is far from optimal... */
> > -    if (count > descr->nb_items)
> > +    if (count > orig_num)
> >       {
> > -        while (count > descr->nb_items)
> > -            if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)
> > -                return ret;
> > +        if (!resize_storage(descr, count))
> > +            return LB_ERRSPACE;
> > +        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> > +        descr->nb_items = count;
> > +
> > +        LISTBOX_UpdateScroll(descr);
> > +        LISTBOX_InvalidateItems(descr, orig_num);
> > +
> > +        /* If listbox was empty, set focus to the first item */
> > +        if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE);
> >       }
> > -    else if (count < descr->nb_items)
> > +    else if (count < orig_num)
> >       {
> > -        while (count < descr->nb_items)
> > -            if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)
> > -                return ret;
> > +        LISTBOX_InvalidateItems(descr, count);
> > +        descr->nb_items = count;
> > +
> > +        if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> > +        else
> > +        {
> > +            descr->anchor_item = min(descr->anchor_item, count - 1);
> > +
> > +            resize_storage(descr, count);
> > +            if (descr->selected_item >= count)
> > +                descr->selected_item = -1;
> > +
> > +            LISTBOX_UpdateScroll(descr);
> > +
> > +            /* If we removed the scrollbar, reset the top of the list */
> > +            if (count <= descr->page_size && orig_num > descr->page_size)
> > +                LISTBOX_SetTopItem(descr, 0, TRUE);
> > +
> > +            descr->focus_item = min(descr->focus_item, count - 1);
> > +        }
> >       }
> >
> > InvalidateRect( descr->self, NULL, TRUE );
>
> This could be a lot simpler.  For a start, we don't need the calls to
> LB_InvalidateItems() since we have the InvalidateRect() call at the
> end.  Also, much of the code in the two cases (shrink or grow) is the
> same and so could be merged.  (I realise you've basically inlined the
> code that was called before, but we can do better than that).
>
> It probably also makes sense to have resize_storage() perform the
> zero-init.
>
> Huw.

I'll see what I can do about minimizing the code, but I'm not sure if it 
will complicate things later with multi-selection listbox (I guess such 
changes should be done later, if needed).

But, the zero-init would be redundant for all other listboxes though, so 
it will slow down inserting items for "normal" listboxes. Should I do it 
just for LBS_NODATA in resize_storage?

[Huw Davies]

On Mon, Feb 11, 2019 at 01:00:46PM +0200, Gabriel Ivăncescu wrote:
> On 2/11/19 11:54 AM, Huw Davies wrote:
> > On Fri, Feb 08, 2019 at 02:41:42PM +0200, Gabriel Ivăncescu wrote:
> > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > > ---
> > >
> > > This has been carefully done so that it matches the previous behavior when
> > > LBS_NODATA was used, since SetCount only works with it (so that means it
> > > also implies LBS_OWNERDRAWFIXED and so on).
> > >
> > > count was changed to unsigned so that it passes the last test in the patch
> > > series, apparently Windows treats it as unsigned.
> > >
> > > dlls/comctl32/listbox.c | 45 +++++++++++++++++++++++++++++++----------
> > >   1 file changed, 34 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > index c92c2a2..787fc88 100644
> > > --- a/dlls/comctl32/listbox.c
> > > +++ b/dlls/comctl32/listbox.c
> > > @@ -1742,24 +1742,47 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
> > >   /***********************************************************************
> > >    *           LISTBOX_SetCount
> > >    */
> > > -static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
> > > +static LRESULT LISTBOX_SetCount( LB_DESCR *descr, UINT count )
> > >   {
> > > -    LRESULT ret;
> > > +    UINT orig_num = descr->nb_items;
> > >       if (!(descr->style & LBS_NODATA)) return LB_ERR;
> > > -    /* FIXME: this is far from optimal... */
> > > -    if (count > descr->nb_items)
> > > +    if (count > orig_num)
> > >       {
> > > -        while (count > descr->nb_items)
> > > -            if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0)
> > > -                return ret;
> > > +        if (!resize_storage(descr, count))
> > > +            return LB_ERRSPACE;
> > > +        memset(&descr->items[orig_num], 0, (count - orig_num) * sizeof(LB_ITEMDATA));
> > > +        descr->nb_items = count;
> > > +
> > > +        LISTBOX_UpdateScroll(descr);
> > > +        LISTBOX_InvalidateItems(descr, orig_num);
> > > +
> > > +        /* If listbox was empty, set focus to the first item */
> > > +        if (orig_num == 0) LISTBOX_SetCaretIndex(descr, 0, FALSE);
> > >       }
> > > -    else if (count < descr->nb_items)
> > > +    else if (count < orig_num)
> > >       {
> > > -        while (count < descr->nb_items)
> > > -            if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0)
> > > -                return ret;
> > > +        LISTBOX_InvalidateItems(descr, count);
> > > +        descr->nb_items = count;
> > > +
> > > +        if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);
> > > +        else
> > > +        {
> > > +            descr->anchor_item = min(descr->anchor_item, count - 1);
> > > +
> > > +            resize_storage(descr, count);
> > > +            if (descr->selected_item >= count)
> > > +                descr->selected_item = -1;
> > > +
> > > +            LISTBOX_UpdateScroll(descr);
> > > +
> > > +            /* If we removed the scrollbar, reset the top of the list */
> > > +            if (count <= descr->page_size && orig_num > descr->page_size)
> > > +                LISTBOX_SetTopItem(descr, 0, TRUE);
> > > +
> > > +            descr->focus_item = min(descr->focus_item, count - 1);
> > > +        }
> > >       }
> > >       InvalidateRect( descr->self, NULL, TRUE );
> >
> > This could be a lot simpler.  For a start, we don't need the calls to
> > LB_InvalidateItems() since we have the InvalidateRect() call at the
> > end.  Also, much of the code in the two cases (shrink or grow) is the
> > same and so could be merged.  (I realise you've basically inlined the
> > code that was called before, but we can do better than that).
> >
> > It probably also makes sense to have resize_storage() perform the
> > zero-init.
> >
> > Huw.
>
> I'll see what I can do about minimizing the code, but I'm not sure if it
> will complicate things later with multi-selection listbox (I guess such
> changes should be done later, if needed).

Well ideally this won't change much this mutli-select no-data.  If it does,
you'll probably need to add more helpers.
> But, the zero-init would be redundant for all other listboxes though, so it
> will slow down inserting items for "normal" listboxes. Should I do it just
> for LBS_NODATA in resize_storage?

Yes, that's what I meant.  resize_storage() is allowed to be different for
the no-data case.

Huw.