[PATCH v5 1/8] comctl32/tests: Confirm that emptying the listbox sends a LB_RESETCONTENT to itself

[Gabriel Ivăncescu]

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

diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
index 43da3ca..9158e36 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -34,6 +34,7 @@
 
 enum seq_index
 {
+    LB_SEQ_INDEX,
     PARENT_SEQ_INDEX,
     NUM_MSG_SEQUENCES
 };
@@ -110,6 +111,43 @@ static HWND create_listbox(DWORD add_style, HWND parent)
     return handle;
 }
 
+static WNDPROC lb_test_subclass_proc_prev;
+static LRESULT WINAPI lb_test_subclass_proc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
+{
+    static LONG defwndproc_counter = 0;
+    struct message msg = { 0 };
+    LRESULT ret;
+
+    switch (message)
+    {
+        case WM_SIZE:
+        case WM_GETTEXT:
+        case WM_PAINT:
+        case WM_ERASEBKGND:
+        case WM_WINDOWPOSCHANGING:
+        case WM_WINDOWPOSCHANGED:
+        case WM_NCCALCSIZE:
+        case WM_NCPAINT:
+        case WM_NCHITTEST:
+        case WM_DEVICECHANGE:
+            break;
+
+        default:
+            msg.message = message;
+            msg.flags = sent|wparam|lparam;
+            if (defwndproc_counter) msg.flags |= defwinproc;
+            msg.wParam = wParam;
+            msg.lParam = lParam;
+            add_message(sequences, LB_SEQ_INDEX, &msg);
+    }
+
+    defwndproc_counter++;
+    ret = CallWindowProcA(lb_test_subclass_proc_prev, hwnd, message, wParam, lParam);
+    defwndproc_counter--;
+
+    return ret;
+}
+
 struct listbox_prop
 {
     DWORD add_style;
@@ -167,6 +205,15 @@ static void keypress(HWND handle, WPARAM keycode, BYTE scancode, BOOL extended)
 
 static void run_test(DWORD style, const struct listbox_test test)
 {
+    static const struct message delete_seq[] =
+    {
+        { LB_DELETESTRING, sent|wparam|lparam, 0, 0 },
+        { LB_DELETESTRING, sent|wparam|lparam, 0, 0 },
+        { LB_DELETESTRING, sent|wparam|lparam, 0, 0 },
+        { LB_DELETESTRING, sent|wparam|lparam, 0, 0 },
+        { LB_RESETCONTENT, sent|wparam|lparam|defwinproc, 0, 0 },
+        { 0 }
+    };
     struct listbox_stat answer;
     RECT second_item;
     int i, res;
@@ -227,6 +274,17 @@ static void run_test(DWORD style, const struct listbox_test test)
     res = SendMessageA(hLB, LB_GETCOUNT, 0, 0);
     ok(res == 4, "Expected 4 items, got %d\n", res);
 
+    /* Confirm that emptying the listbox sends a LB_RESETCONTENT to itself */
+    lb_test_subclass_proc_prev = (WNDPROC)SetWindowLongPtrW(hLB, GWLP_WNDPROC, (LONG_PTR)lb_test_subclass_proc);
+
+    flush_sequence(sequences, LB_SEQ_INDEX);
+    for (i = 4; i--;)
+    {
+        res = SendMessageA(hLB, LB_DELETESTRING, 0, 0);
+        ok(res == i, "Expected %d items, got %d\n", i, res);
+    }
+    ok_sequence(sequences, LB_SEQ_INDEX, delete_seq, "Emptying listbox", FALSE);
+
     DestroyWindow(hLB);
 }
 
@@ -1763,6 +1821,12 @@ static void test_listbox_dlgdir(void)
 
 static void test_set_count( void )
 {
+    static const struct message setcount0_seq[] =
+    {
+        { LB_SETCOUNT, sent|wparam|lparam, 0, 0 },
+        { LB_RESETCONTENT, sent|wparam|lparam|defwinproc, 0, 0 },
+        { 0 }
+    };
     static const DWORD styles[] =
     {
         LBS_OWNERDRAWFIXED,
@@ -1798,6 +1862,14 @@ static void test_set_count( void )
     GetUpdateRect( listbox, &r, TRUE );
     ok( !IsRectEmpty( &r ), "got empty rect\n");
 
+    /* Confirm that emptying the listbox sends a LB_RESETCONTENT to itself */
+    lb_test_subclass_proc_prev = (WNDPROC)SetWindowLongPtrW(listbox, GWLP_WNDPROC, (LONG_PTR)lb_test_subclass_proc);
+
+    flush_sequence(sequences, LB_SEQ_INDEX);
+    ret = SendMessageA(listbox, LB_SETCOUNT, 0, 0);
+    ok(ret == 0, "got %d\n", ret);
+    ok_sequence(sequences, LB_SEQ_INDEX, setcount0_seq, "LB_SETCOUNT 0", FALSE);
+
     DestroyWindow( listbox );
 
     for (i = 0; i < ARRAY_SIZE(styles); ++i)
-- 
2.19.1

[PATCH v5 2/8] 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 9158e36..f586565 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -773,6 +773,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 (
@@ -2434,6 +2563,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 v5 3/8] comctl32/listbox: Check for out of bounds index using an unsigned index

[Gabriel Ivăncescu]

Needed for LBS_NODATA.

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

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index dbc7b4a..a380b59 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -483,7 +483,7 @@ static INT LISTBOX_GetItemFromPoint( const LB_DESCR *descr, INT x, INT y )
  * Paint an item.
  */
 static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
-			       INT index, UINT action, BOOL ignoreFocus )
+			       UINT index, UINT action, BOOL ignoreFocus )
 {
     LB_ITEMDATA *item = NULL;
     if (index < descr->nb_items) item = &descr->items[index];
@@ -494,7 +494,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
         RECT r;
         HRGN hrgn;
 
-	if (!item)
+	if (index >= descr->nb_items)
 	{
 	    if (action == ODA_FOCUS)
 		DrawFocusRect( hdc, rect );
-- 
2.19.1

[PATCH v5 4/8] comctl32/listbox: Check for NULL item when populating the WM_DRAWITEM struct or tracing the string

[Gabriel Ivăncescu]

Needed for LBS_NODATA.

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

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index a380b59..a77de49 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -522,10 +522,10 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
             (descr->caret_on) &&
             (descr->in_focus)) dis.itemState |= ODS_FOCUS;
         if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED;
-        dis.itemData     = item->data;
+        dis.itemData     = item ? item->data : 0;
         dis.rcItem       = *rect;
         TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n",
-              descr->self, index, debugstr_w(item->str), action,
+              descr->self, index, debugstr_w(item ? item->str : NULL), action,
               dis.itemState, wine_dbgstr_rect(rect) );
         SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis);
         SelectClipRgn( hdc, hrgn );
-- 
2.19.1

[PATCH v5 5/8] comctl32/listbox: Use a helper function to check the selection state of an item

[Gabriel Ivăncescu]

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

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index a77de49..0638cdb 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -125,6 +125,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
 
 static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
 
+static BOOL is_item_selected(LB_DESCR *descr, UINT index)
+{
+    return descr->items[index].selected;
+}
+
 /***********************************************************************
  *           LISTBOX_GetCurrentPageSize
  *
@@ -517,7 +522,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
         dis.hDC          = hdc;
         dis.itemID       = index;
         dis.itemState    = 0;
-        if (item->selected) dis.itemState |= ODS_SELECTED;
+        if (is_item_selected(descr, index)) dis.itemState |= ODS_SELECTED;
         if (!ignoreFocus && (descr->focus_item == index) &&
             (descr->caret_on) &&
             (descr->in_focus)) dis.itemState |= ODS_FOCUS;
@@ -2763,7 +2768,7 @@ static LRESULT CALLBACK LISTBOX_WindowProc( HWND hwnd, UINT msg, WPARAM wParam,
     case LB_GETSEL:
         if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items))
             return LB_ERR;
-        return descr->items[wParam].selected;
+        return is_item_selected(descr, wParam);
 
     case LB_SETSEL:
         ret = LISTBOX_SetSelection( descr, lParam, wParam, FALSE );
-- 
2.19.1

[PATCH v5 6/8] comctl32/listbox: Store the items array size in a field instead of using HeapSize

[Gabriel Ivăncescu]

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

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 0638cdb..dbcbf4c 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -70,6 +70,7 @@ typedef struct
     UINT        style;          /* Window style */
     INT         width;          /* Window width */
     INT         height;         /* Window height */
+    UINT        array_size;     /* Total number of allocated items in the array */
     LB_ITEMDATA  *items;        /* Array of items */
     INT         nb_items;       /* Number of items */
     INT         top_item;       /* Top visible item */
@@ -681,7 +682,7 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
     nb_items += LB_ARRAY_GRANULARITY - 1;
     nb_items -= (nb_items % LB_ARRAY_GRANULARITY);
     if (descr->items) {
-        nb_items += HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(*item);
+        nb_items += descr->array_size;
 	item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
                               nb_items * sizeof(LB_ITEMDATA));
     }
@@ -695,6 +696,7 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
         SEND_NOTIFICATION( descr, LBN_ERRSPACE );
         return LB_ERRSPACE;
     }
+    descr->array_size = nb_items;
     descr->items = item;
     return LB_OKAY;
 }
@@ -1521,12 +1523,10 @@ 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 (!descr->items) max_items = 0;
-    else max_items = HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(*item);
-    if (descr->nb_items == max_items)
+    if (descr->nb_items == descr->array_size)
     {
         /* We need to grow the array */
-        max_items += LB_ARRAY_GRANULARITY;
+        max_items = descr->array_size + LB_ARRAY_GRANULARITY;
 	if (descr->items)
 	    item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
                                   max_items * sizeof(LB_ITEMDATA) );
@@ -1538,6 +1538,7 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
             SEND_NOTIFICATION( descr, LBN_ERRSPACE );
             return LB_ERRSPACE;
         }
+        descr->array_size = max_items;
         descr->items = item;
     }
 
@@ -1697,13 +1698,17 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
 
     /* Shrink the item array if possible */
 
-    max_items = HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(LB_ITEMDATA);
+    max_items = descr->array_size;
     if (descr->nb_items < max_items - 2*LB_ARRAY_GRANULARITY)
     {
         max_items -= LB_ARRAY_GRANULARITY;
         item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
                             max_items * sizeof(LB_ITEMDATA) );
-        if (item) descr->items = item;
+        if (item)
+        {
+            descr->array_size = max_items;
+            descr->items = item;
+        }
     }
     /* Repaint the items */
 
@@ -1749,6 +1754,7 @@ static void LISTBOX_ResetContent( LB_DESCR *descr )
     descr->selected_item = -1;
     descr->focus_item    = 0;
     descr->anchor_item   = -1;
+    descr->array_size    = 0;
     descr->items         = NULL;
 }
 
@@ -2486,6 +2492,7 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc )
     descr->width         = rect.right - rect.left;
     descr->height        = rect.bottom - rect.top;
     descr->items         = NULL;
+    descr->array_size    = 0;
     descr->nb_items      = 0;
     descr->top_item      = 0;
     descr->selected_item = -1;
-- 
2.19.1

[PATCH v5 7/8] comctl32/listbox: Expand the item array in a helper function

[Gabriel Ivăncescu]

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

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index dbcbf4c..3b76e19 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -36,6 +36,7 @@
 #include "wine/unicode.h"
 #include "wine/exception.h"
 #include "wine/debug.h"
+#include "wine/heap.h"
 
 #include "comctl32.h"
 
@@ -126,6 +127,27 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
 
 static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
 
+static BOOL expand_storage(LB_DESCR *descr, UINT amount)
+{
+    LB_ITEMDATA *p = descr->items;
+    UINT num = descr->nb_items + amount;
+
+    if (num > descr->array_size)
+    {
+        num += LB_ARRAY_GRANULARITY - 1;
+        num -= num % LB_ARRAY_GRANULARITY;
+        p = heap_realloc(p, num * sizeof(LB_ITEMDATA));
+        if (!p)
+        {
+            SEND_NOTIFICATION(descr, LBN_ERRSPACE);
+            return FALSE;
+        }
+        descr->array_size = num;
+        descr->items = p;
+    }
+    return TRUE;
+}
+
 static BOOL is_item_selected(LB_DESCR *descr, UINT index)
 {
     return descr->items[index].selected;
@@ -677,27 +699,8 @@ static void LISTBOX_DrawFocusRect( LB_DESCR *descr, BOOL on )
  */
 static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
 {
-    LB_ITEMDATA *item;
-
-    nb_items += LB_ARRAY_GRANULARITY - 1;
-    nb_items -= (nb_items % LB_ARRAY_GRANULARITY);
-    if (descr->items) {
-        nb_items += descr->array_size;
-	item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                              nb_items * sizeof(LB_ITEMDATA));
-    }
-    else {
-	item = HeapAlloc( GetProcessHeap(), 0,
-                              nb_items * sizeof(LB_ITEMDATA));
-    }
-
-    if (!item)
-    {
-        SEND_NOTIFICATION( descr, LBN_ERRSPACE );
+    if (!expand_storage(descr, nb_items))
         return LB_ERRSPACE;
-    }
-    descr->array_size = nb_items;
-    descr->items = item;
     return LB_OKAY;
 }
 
@@ -1518,29 +1521,11 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
                                    LPWSTR str, ULONG_PTR data )
 {
     LB_ITEMDATA *item;
-    INT max_items;
     INT oldfocus = descr->focus_item;
 
     if (index == -1) index = descr->nb_items;
     else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
-    if (descr->nb_items == descr->array_size)
-    {
-        /* We need to grow the array */
-        max_items = descr->array_size + LB_ARRAY_GRANULARITY;
-	if (descr->items)
-	    item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                                  max_items * sizeof(LB_ITEMDATA) );
-	else
-	    item = HeapAlloc( GetProcessHeap(), 0,
-                                  max_items * sizeof(LB_ITEMDATA) );
-        if (!item)
-        {
-            SEND_NOTIFICATION( descr, LBN_ERRSPACE );
-            return LB_ERRSPACE;
-        }
-        descr->array_size = max_items;
-        descr->items = item;
-    }
+    if (!expand_storage(descr, 1)) return LB_ERR;
 
     /* Insert the item structure */
 
-- 
2.19.1

[PATCH v5 8/8] comctl32/listbox: Shrink the item array in a helper function

[Gabriel Ivăncescu]

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

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 3b76e19..41eb631 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -148,6 +148,24 @@ static BOOL expand_storage(LB_DESCR *descr, UINT amount)
     return TRUE;
 }
 
+static void shrink_storage(LB_DESCR *descr)
+{
+    LB_ITEMDATA *p = descr->items;
+    UINT num = descr->nb_items;
+
+    if (num + LB_ARRAY_GRANULARITY * 2 < descr->array_size)
+    {
+        num += LB_ARRAY_GRANULARITY - 1;
+        num -= num % LB_ARRAY_GRANULARITY;
+        p = HeapReAlloc(GetProcessHeap(), 0, p, num * sizeof(LB_ITEMDATA));
+        if (p)
+        {
+            descr->array_size = num;
+            descr->items = p;
+        }
+    }
+}
+
 static BOOL is_item_selected(LB_DESCR *descr, UINT index)
 {
     return descr->items[index].selected;
@@ -1661,7 +1679,6 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index )
 static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
 {
     LB_ITEMDATA *item;
-    INT max_items;
 
     if ((index < 0) || (index >= descr->nb_items)) return LB_ERR;
 
@@ -1681,20 +1698,8 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
                        (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
     if (descr->anchor_item == descr->nb_items) descr->anchor_item--;
 
-    /* Shrink the item array if possible */
+    shrink_storage(descr);
 
-    max_items = descr->array_size;
-    if (descr->nb_items < max_items - 2*LB_ARRAY_GRANULARITY)
-    {
-        max_items -= LB_ARRAY_GRANULARITY;
-        item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                            max_items * sizeof(LB_ITEMDATA) );
-        if (item)
-        {
-            descr->array_size = max_items;
-            descr->items = item;
-        }
-    }
     /* Repaint the items */
 
     LISTBOX_UpdateScroll( descr );
-- 
2.19.1

[PATCH v5 8/8] comctl32/listbox: Shrink the item array in a helper function

[Huw Davies]

On Thu, Nov 22, 2018 at 03:44:44PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>  dlls/comctl32/listbox.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index 3b76e19..41eb631 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -148,6 +148,24 @@ static BOOL expand_storage(LB_DESCR *descr, UINT amount)
>      return TRUE;
>  }
>
> +static void shrink_storage(LB_DESCR *descr)
> +{
> +    LB_ITEMDATA *p = descr->items;
> +    UINT num = descr->nb_items;
> +
> +    if (num + LB_ARRAY_GRANULARITY * 2 < descr->array_size)
> +    {
> +        num += LB_ARRAY_GRANULARITY - 1;
> +        num -= num % LB_ARRAY_GRANULARITY;
> +        p = HeapReAlloc(GetProcessHeap(), 0, p, num * sizeof(LB_ITEMDATA));
> +        if (p)
> +        {
> +            descr->array_size = num;
> +            descr->items = p;
> +        }
> +    }
> +}
> +

I was hoping for a common allocation function (resize_storage() say),
that would take the length of the required array, not one function to
grow and another to shrink.

Also, let's insist that LB_ARRAY_GRANULARITY is a power of two, to
simplify these adjustments.

Huw.

[PATCH v5 8/8] comctl32/listbox: Shrink the item array in a helper function

[Alexandre Julliard]

Huw Davies <huw(a)codeweavers.com> writes:
> On Thu, Nov 22, 2018 at 03:44:44PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > ---
> >  dlls/comctl32/listbox.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index 3b76e19..41eb631 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -148,6 +148,24 @@ static BOOL expand_storage(LB_DESCR *descr, UINT amount)
> >      return TRUE;
> >  }
> >
> > +static void shrink_storage(LB_DESCR *descr)
> > +{
> > +    LB_ITEMDATA *p = descr->items;
> > +    UINT num = descr->nb_items;
> > +
> > +    if (num + LB_ARRAY_GRANULARITY * 2 < descr->array_size)
> > +    {
> > +        num += LB_ARRAY_GRANULARITY - 1;
> > +        num -= num % LB_ARRAY_GRANULARITY;
> > +        p = HeapReAlloc(GetProcessHeap(), 0, p, num * sizeof(LB_ITEMDATA));
> > +        if (p)
> > +        {
> > +            descr->array_size = num;
> > +            descr->items = p;
> > +        }
> > +    }
> > +}
> > +
>
> I was hoping for a common allocation function (resize_storage() say),
> that would take the length of the required array, not one function to
> grow and another to shrink.
>
> Also, let's insist that LB_ARRAY_GRANULARITY is a power of two, to
> simplify these adjustments.

Note that doubling the size when growing would be better than a fixed
constant (but maybe we want to avoid changing even more things at this
point...)

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH v5 8/8] comctl32/listbox: Shrink the item array in a helper function

[Huw Davies]

On Thu, Nov 22, 2018 at 03:10:21PM +0100, Alexandre Julliard wrote:
> Huw Davies <huw(a)codeweavers.com> writes:
> > I was hoping for a common allocation function (resize_storage() say),
> > that would take the length of the required array, not one function to
> > grow and another to shrink.
> >
> > Also, let's insist that LB_ARRAY_GRANULARITY is a power of two, to
> > simplify these adjustments.
>
> Note that doubling the size when growing would be better than a fixed
> constant (but maybe we want to avoid changing even more things at this
> point...)

Yeah, I was hoping to walk before we run.

Huw.

[PATCH v5 8/8] comctl32/listbox: Shrink the item array in a helper function

[Gabriel Ivăncescu]

On Thu, Nov 22, 2018 at 4:02 PM Huw Davies <huw(a)codeweavers.com> wrote:
> On Thu, Nov 22, 2018 at 03:44:44PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > ---
> >  dlls/comctl32/listbox.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index 3b76e19..41eb631 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -148,6 +148,24 @@ static BOOL expand_storage(LB_DESCR *descr, UINT amount)
> >      return TRUE;
> >  }
> >
> > +static void shrink_storage(LB_DESCR *descr)
> > +{
> > +    LB_ITEMDATA *p = descr->items;
> > +    UINT num = descr->nb_items;
> > +
> > +    if (num + LB_ARRAY_GRANULARITY * 2 < descr->array_size)
> > +    {
> > +        num += LB_ARRAY_GRANULARITY - 1;
> > +        num -= num % LB_ARRAY_GRANULARITY;
> > +        p = HeapReAlloc(GetProcessHeap(), 0, p, num * sizeof(LB_ITEMDATA));
> > +        if (p)
> > +        {
> > +            descr->array_size = num;
> > +            descr->items = p;
> > +        }
> > +    }
> > +}
> > +
>
> I was hoping for a common allocation function (resize_storage() say),
> that would take the length of the required array, not one function to
> grow and another to shrink.
>
> Also, let's insist that LB_ARRAY_GRANULARITY is a power of two, to
> simplify these adjustments.
>
> Huw.

I didn't want to change too much in one patch. Also only growing
should give error when failing to allocate (as was the behavior
before). I think adding extra checks for that is going to make the
function uglier than it needs to be, since it will be effectively two
functions in one.