[PATCH v3 1/7] comctl32/tests: Confirm that emptying the listbox sends a LB_RESETCONTENT to itself
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/tests/listbox.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 43da3ca..d457b13 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -110,6 +110,15 @@ static HWND create_listbox(DWORD add_style, HWND parent) return handle; } +static UINT lb_resetcontent_count; +static WNDPROC lb_test_subclass_proc_prev; +static LRESULT WINAPI lb_test_subclass_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + if (msg == LB_RESETCONTENT) lb_resetcontent_count++; + + return CallWindowProcW(lb_test_subclass_proc_prev, hwnd, msg, wParam, lParam); +} + struct listbox_prop { DWORD add_style; @@ -227,6 +236,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); + + lb_resetcontent_count = 0; + for (i = 4; i--;) + { + res = SendMessageA(hLB, LB_DELETESTRING, 0, 0); + ok(res == i, "Expected %d items, got %d\n", i, res); + } + ok(lb_resetcontent_count == 1, "Expected 1 LB_RESETCONTENT message, got %u\n", lb_resetcontent_count); + DestroyWindow(hLB); } @@ -1798,6 +1818,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); + + lb_resetcontent_count = 0; + ret = SendMessageA(listbox, LB_SETCOUNT, 0, 0); + ok(ret == 0, "got %d\n", ret); + ok(lb_resetcontent_count == 1, "Expected 1 LB_RESETCONTENT message, got %u\n", lb_resetcontent_count); + DestroyWindow( listbox ); for (i = 0; i < ARRAY_SIZE(styles); ++i) -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/tests/listbox.c | 54 +++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index d457b13..eeb1c7b 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -735,6 +735,59 @@ 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 + }; + HWND parent, listbox; + LONG ret, expected; + DWORD style; + UINT i, j; + + parent = create_parent(); + for (i = 0; i < ARRAY_SIZE(styles); i++) + { + for (j = 0; j < ARRAY_SIZE(selstyles); j++) + { + listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | selstyles[j] | WS_VISIBLE, + 0, 0, 100, 100, NULL, NULL, NULL, 0); + ok(listbox != NULL, "%u: Failed to create ListBox window.\n", j); + + 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]; + expected = 0; + } + else + { + style |= LBS_MULTIPLESEL | LBS_EXTENDEDSEL; + expected = LB_ERR; + } + SetWindowLongA(listbox, GWL_STYLE, style); + style = GetWindowLongA(listbox, GWL_STYLE); + ok(!(style & selstyles[j]), "%u: unexpected window styles %#x.\n", j, style); + + ret = SendMessageA(listbox, LB_GETSELCOUNT, 0, 0); + ok(ret == expected, "%u: expected %d from LB_GETSELCOUNT, got %d\n", j, expected, ret); + DestroyWindow(listbox); + } + } + DestroyWindow(parent); +} + static void test_itemfrompoint(void) { /* WS_POPUP is required in order to have a more accurate size calculation ( @@ -2390,6 +2443,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
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
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
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
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- SetCount's count was changed to UINT because otherwise it will fail to pass the tests (next patch). dlls/comctl32/listbox.c | 269 +++++++++++++++++++++++++++++----------- 1 file changed, 194 insertions(+), 75 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 0638cdb..059ac90 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> @@ -125,8 +125,75 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +/*********************************************************************** + * Helper functions for LBS_NODATA listboxes + * + * Since LBS_NODATA listboxes can be extremely large, we need to store + * them with minimal overhead, both for performance and memory usage. + * + * In all cases, it is important to note that descr->items must never be + * dereferenced directly with LBS_NODATA, outside of these helpers. + * + * For single-selection listboxes, we store literally no data for items, + * and thus descr->items will always be NULL in this case. + * + * FIXME: Multi-selection listboxes are not implemented yet for LBS_NODATA. + * + */ +static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & LBS_NODATA) && + !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)); +} + +static BOOL NODATA_is_sel(const LB_DESCR *descr, UINT index) +{ + return index == descr->selected_item; +} + +static BOOL NODATA_multisel_expand(LB_DESCR *descr, UINT num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = num * sizeof(*p); + + if (!p || sz > HeapSize(GetProcessHeap(), 0, p)) + { + sz += LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + sz -= sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if (!p) p = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz); + else p = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, p, sz); + if (!p) return FALSE; + descr->items = p; + } + return TRUE; +} + +static void NODATA_multisel_shrink(LB_DESCR *descr, UINT orig_num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = descr->nb_items * sizeof(*p); + UINT orig_sz = orig_num * sizeof(*p); + + /* Shrink the array if needed */ + if (sz + LB_ARRAY_GRANULARITY * sizeof(*p) * 2 < HeapSize(GetProcessHeap(), 0, p)) + { + UINT rnd_sz = sz + LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + rnd_sz -= rnd_sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if ((p = HeapReAlloc(GetProcessHeap(), 0, p, rnd_sz))) + { + descr->items = p; + orig_sz = rnd_sz; + } + } + memset(&p[sz / sizeof(*p)], 0, orig_sz - sz); +} + + + static BOOL is_item_selected(LB_DESCR *descr, UINT index) { + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, index); return descr->items[index].selected; } @@ -522,6 +589,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, dis.hDC = hdc; dis.itemID = index; dis.itemState = 0; + if (is_singlesel_NODATA(descr)) item = NULL; if (is_item_selected(descr, index)) dis.itemState |= ODS_SELECTED; if (!ignoreFocus && (descr->focus_item == index) && (descr->caret_on) && @@ -678,6 +746,9 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items ) { LB_ITEMDATA *item; + if (is_singlesel_NODATA(descr)) + return LB_OKAY; + nb_items += LB_ARRAY_GRANULARITY - 1; nb_items -= (nb_items % LB_ARRAY_GRANULARITY); if (descr->items) { @@ -1445,10 +1516,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 ); @@ -1521,54 +1595,59 @@ 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) - { - /* We need to grow the array */ - max_items += 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) + if (is_singlesel_NODATA(descr)) + { + descr->nb_items++; + } + else + { + if (!descr->items) max_items = 0; + else max_items = HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(*item); + if (descr->nb_items == max_items) { - SEND_NOTIFICATION( descr, LBN_ERRSPACE ); - return LB_ERRSPACE; + /* We need to grow the array */ + max_items += 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->items = item; } - descr->items = 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 */ + /* 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++; - if (descr->style & LBS_OWNERDRAWVARIABLE) - { - MEASUREITEMSTRUCT mis; - UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + /* 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 */ @@ -1683,28 +1762,38 @@ 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; - - /* Remove the item */ + if (is_singlesel_NODATA(descr)) + { + if (!descr->nb_items) + { + SendMessageW(descr->self, LB_RESETCONTENT, 0, 0); + return LB_OKAY; + } + } + else + { + LISTBOX_DeleteItem( descr, index ); - 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--; + if (!descr->nb_items) return LB_OKAY; - /* Shrink the item array if possible */ + /* Remove the item */ + item = &descr->items[index]; + if (index < descr->nb_items) + RtlMoveMemory( item, item + 1, + (descr->nb_items - index) * sizeof(LB_ITEMDATA) ); - max_items = HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(LB_ITEMDATA); - 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; + /* Shrink the item array if possible */ + max_items = HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(LB_ITEMDATA); + 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; + } } + descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1); + /* Repaint the items */ LISTBOX_UpdateScroll( descr ); @@ -1742,7 +1831,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; @@ -1756,24 +1846,53 @@ 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; + INT orig_num; if (!(descr->style & LBS_NODATA)) return LB_ERR; - /* FIXME: this is far from optimal... */ if (count > descr->nb_items) { - while (count > descr->nb_items) - if ((ret = LISTBOX_InsertString( descr, -1, 0 )) < 0) - return ret; + if ((descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) && + !NODATA_multisel_expand(descr, count)) + { + SEND_NOTIFICATION(descr, LBN_ERRSPACE); + return LB_ERRSPACE; + } + orig_num = descr->nb_items; + 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) { - while (count < descr->nb_items) - if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0) - return ret; + LISTBOX_InvalidateItems(descr, count); + orig_num = descr->nb_items; + descr->nb_items = count; + + if (count == 0) SendMessageW(descr->self, LB_RESETCONTENT, 0, 0); + else + { + descr->anchor_item = min(descr->anchor_item, count - 1); + + if (descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) + NODATA_multisel_shrink(descr, orig_num); + else if (descr->selected_item >= descr->nb_items) + descr->selected_item = -1; + + LISTBOX_UpdateScroll(descr); + + /* If we removed the scrollbar, reset the top of the list */ + if (descr->nb_items <= descr->page_size && orig_num > descr->page_size) + LISTBOX_SetTopItem(descr, 0, TRUE); + + descr->focus_item = min(descr->focus_item, descr->nb_items - 1); + } } InvalidateRect( descr->self, NULL, TRUE ); -- 2.19.1
On Wed, Nov 21, 2018 at 03:38:47PM +0200, Gabriel Ivăncescu wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
SetCount's count was changed to UINT because otherwise it will fail to pass the tests (next patch).
dlls/comctl32/listbox.c | 269 +++++++++++++++++++++++++++++----------- 1 file changed, 194 insertions(+), 75 deletions(-)
This is getting better, but this patch is still way too big and does things that are unnecessary.
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 0638cdb..059ac90 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> @@ -125,8 +125,75 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
+/*********************************************************************** + * Helper functions for LBS_NODATA listboxes + * + * Since LBS_NODATA listboxes can be extremely large, we need to store + * them with minimal overhead, both for performance and memory usage. + * + * In all cases, it is important to note that descr->items must never be + * dereferenced directly with LBS_NODATA, outside of these helpers. + * + * For single-selection listboxes, we store literally no data for items, + * and thus descr->items will always be NULL in this case. + * + * FIXME: Multi-selection listboxes are not implemented yet for LBS_NODATA. + * + */ +static BOOL is_singlesel_NODATA(const LB_DESCR *descr) +{ + return (descr->style & LBS_NODATA) && + !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL)); +} + +static BOOL NODATA_is_sel(const LB_DESCR *descr, UINT index) +{ + return index == descr->selected_item; +}
The idea would be to put all of this logic into is_item_selected().
+ +static BOOL NODATA_multisel_expand(LB_DESCR *descr, UINT num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = num * sizeof(*p); + + if (!p || sz > HeapSize(GetProcessHeap(), 0, p)) + { + sz += LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + sz -= sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if (!p) p = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz); + else p = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, p, sz); + if (!p) return FALSE; + descr->items = p; + } + return TRUE; +}
You shouldn't be adding modata muiltsel stuff in this patch. One thing you may want to do is to tidy up the allocation functions (in a separate patch or two). For example InsertItem should call InitStorage rather than duplicate the allocation logic. Also it would be nice to get rid of the HeapSize calls and simply store the length of the items array. Another thing you could do would be to move to the wine/heap.h functions which would let you use the heap_realloc(NULL,...) behaviour to further simplify things.
+ +static void NODATA_multisel_shrink(LB_DESCR *descr, UINT orig_num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = descr->nb_items * sizeof(*p); + UINT orig_sz = orig_num * sizeof(*p); + + /* Shrink the array if needed */ + if (sz + LB_ARRAY_GRANULARITY * sizeof(*p) * 2 < HeapSize(GetProcessHeap(), 0, p)) + { + UINT rnd_sz = sz + LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + rnd_sz -= rnd_sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if ((p = HeapReAlloc(GetProcessHeap(), 0, p, rnd_sz))) + { + descr->items = p; + orig_sz = rnd_sz; + } + } + memset(&p[sz / sizeof(*p)], 0, orig_sz - sz); +} + + + static BOOL is_item_selected(LB_DESCR *descr, UINT index) { + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, index); return descr->items[index].selected; }
So this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected; and you'll extend it when nodata multiselect is optimized. Huw.
On 11/21/18 5:51 PM, Huw Davies wrote:
static BOOL is_item_selected(LB_DESCR *descr, UINT index) { + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, index); return descr->items[index].selected; } So this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected;
This also should go to is_item_selected().
On Wed, Nov 21, 2018 at 06:19:18PM +0300, Nikolay Sivov wrote:
On 11/21/18 5:51 PM, Huw Davies wrote:
static BOOL is_item_selected(LB_DESCR *descr, UINT index) { + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, index); return descr->items[index].selected; } So this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected;
This also should go to is_item_selected().
Right, I was rewriting is_item_selected(). Huw.
On 11/21/18 6:53 PM, Huw Davies wrote:
On Wed, Nov 21, 2018 at 06:19:18PM +0300, Nikolay Sivov wrote:
On 11/21/18 5:51 PM, Huw Davies wrote:
static BOOL is_item_selected(LB_DESCR *descr, UINT index) { + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, index); return descr->items[index].selected; } So this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected; This also should go to is_item_selected(). Right, I was rewriting is_item_selected().
Eh, sorry, missed context completely.
On Wed, Nov 21, 2018 at 4:51 PM Huw Davies <huw(a)codeweavers.com> wrote:
+static BOOL NODATA_is_sel(const LB_DESCR *descr, UINT index) +{ + return index == descr->selected_item; +}
The idea would be to put all of this logic into is_item_selected().
Sure. I was just trying to separate all of the NODATA internal stuff into their own helpers.
+ +static BOOL NODATA_multisel_expand(LB_DESCR *descr, UINT num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = num * sizeof(*p); + + if (!p || sz > HeapSize(GetProcessHeap(), 0, p)) + { + sz += LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + sz -= sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if (!p) p = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz); + else p = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, p, sz); + if (!p) return FALSE; + descr->items = p; + } + return TRUE; +}
You shouldn't be adding modata muiltsel stuff in this patch.
Yeah I mentioned it in the comments on the first iteration I sent. The reason I did it this way was to avoid having to keep the old SetCount lingering around. Which means I'd have to either (1) use a goto to minimize patch size (yes, I know!) or (2) indent the old SetCount code in this patch. Worse still, the old SetCount would have to be removed later during the multi-selection listbox patch, increasing *that* one's size even more. It's a tradeoff between this patch's size and the multi-selection one.
One thing you may want to do is to tidy up the allocation functions (in a separate patch or two). For example InsertItem should call InitStorage rather than duplicate the allocation logic. Also it would be nice to get rid of the HeapSize calls and simply store the length of the items array. Another thing you could do would be to move to the wine/heap.h functions which would let you use the heap_realloc(NULL,...) behaviour to further simplify things.
I can use InitStorage in InsertItem but I didn't think it would have anything to do with this patch. Not sure if it will help with the multisel_expand case though since I need to pass the HEAP_ZERO_MEMORY flag for the setcount expansion (same limitation for heap_alloc/realloc here). Secondly, about the common allocation. I'm not sure that's going to work in the future because the allocation itself will be changed in NODATA helpers (since it won't use normal item structure), so they won't be identical anymore. And about the HeapSize calls -- well, yeah I agree that it should be changed, but I didn't want to overcomplicate this patchset for changing too much stuff. I'll look into it.
So this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected;
and you'll extend it when nodata multiselect is optimized.
Huw.
Well the reason I wanted to avoid that, was due to wanting to move all implementation detail of nodata stuff into NODATA_* helpers. However, if I won't use the NODATA_is_sel helper at all and just implement everything in this function, then it doesn't make a difference I guess.
On Wed, Nov 21, 2018 at 05:47:06PM +0200, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 4:51 PM Huw Davies <huw(a)codeweavers.com> wrote:
+static BOOL NODATA_multisel_expand(LB_DESCR *descr, UINT num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = num * sizeof(*p); + + if (!p || sz > HeapSize(GetProcessHeap(), 0, p)) + { + sz += LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + sz -= sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if (!p) p = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz); + else p = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, p, sz); + if (!p) return FALSE; + descr->items = p; + } + return TRUE; +}
You shouldn't be adding modata muiltsel stuff in this patch.
Yeah I mentioned it in the comments on the first iteration I sent. The reason I did it this way was to avoid having to keep the old SetCount lingering around.
Which means I'd have to either (1) use a goto to minimize patch size (yes, I know!) or (2) indent the old SetCount code in this patch. Worse still, the old SetCount would have to be removed later during the multi-selection listbox patch, increasing *that* one's size even more.
It's a tradeoff between this patch's size and the multi-selection one.
Like I've already said, I think you want to change SetCount in an earlier patch to allocate the block in one go - it would call into some common allocator function also called by InitStorage. This patch would then simply skip the call to the allocator.
One thing you may want to do is to tidy up the allocation functions (in a separate patch or two). For example InsertItem should call InitStorage rather than duplicate the allocation logic. Also it would be nice to get rid of the HeapSize calls and simply store the length of the items array. Another thing you could do would be to move to the wine/heap.h functions which would let you use the heap_realloc(NULL,...) behaviour to further simplify things.
I can use InitStorage in InsertItem but I didn't think it would have anything to do with this patch. Not sure if it will help with the multisel_expand case though since I need to pass the HEAP_ZERO_MEMORY flag for the setcount expansion (same limitation for heap_alloc/realloc here).
Secondly, about the common allocation. I'm not sure that's going to work in the future because the allocation itself will be changed in NODATA helpers (since it won't use normal item structure), so they won't be identical anymore.
And about the HeapSize calls -- well, yeah I agree that it should be changed, but I didn't want to overcomplicate this patchset for changing too much stuff. I'll look into it.
Plewase do this. Part of the reason this patch is proving difficult is the messy allocation scheme that already exists. Let's tidy that up first. Everything should fall back to a common allocator, once that's done, it becomes easy to alter the size that that will allocate if we shift to using a byte array for the nodata multiselect case.
so this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected;
and you'll extend it when nodata multiselect is optimized.
Huw.
Well the reason I wanted to avoid that, was due to wanting to move all implementation detail of nodata stuff into NODATA_* helpers. However, if I won't use the NODATA_is_sel helper at all and just implement everything in this function, then it doesn't make a difference I guess.
We're trying to stop you pulling the nodata stuff into separate functions, but rather use helpers to abstract certain operations (like checking the selected state and allocation). Those helpers will then need to worry about the details. Huw.
On Wed, Nov 21, 2018 at 7:04 PM Huw Davies <huw(a)codeweavers.com> wrote:
On Wed, Nov 21, 2018 at 05:47:06PM +0200, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 4:51 PM Huw Davies <huw(a)codeweavers.com> wrote:
+static BOOL NODATA_multisel_expand(LB_DESCR *descr, UINT num) +{ + LB_ITEMDATA *p = descr->items; + UINT sz = num * sizeof(*p); + + if (!p || sz > HeapSize(GetProcessHeap(), 0, p)) + { + sz += LB_ARRAY_GRANULARITY * sizeof(*p) - 1; + sz -= sz % (LB_ARRAY_GRANULARITY * sizeof(*p)); + if (!p) p = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz); + else p = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, p, sz); + if (!p) return FALSE; + descr->items = p; + } + return TRUE; +}
You shouldn't be adding modata muiltsel stuff in this patch.
Yeah I mentioned it in the comments on the first iteration I sent. The reason I did it this way was to avoid having to keep the old SetCount lingering around.
Which means I'd have to either (1) use a goto to minimize patch size (yes, I know!) or (2) indent the old SetCount code in this patch. Worse still, the old SetCount would have to be removed later during the multi-selection listbox patch, increasing *that* one's size even more.
It's a tradeoff between this patch's size and the multi-selection one.
Like I've already said, I think you want to change SetCount in an earlier patch to allocate the block in one go - it would call into some common allocator function also called by InitStorage. This patch would then simply skip the call to the allocator.
Yeah, that's exactly what I was doing with the multisel expand function, except for using a common allocator. I guess I can move it to before this patch somehow.
One thing you may want to do is to tidy up the allocation functions (in a separate patch or two). For example InsertItem should call InitStorage rather than duplicate the allocation logic. Also it would be nice to get rid of the HeapSize calls and simply store the length of the items array. Another thing you could do would be to move to the wine/heap.h functions which would let you use the heap_realloc(NULL,...) behaviour to further simplify things.
I can use InitStorage in InsertItem but I didn't think it would have anything to do with this patch. Not sure if it will help with the multisel_expand case though since I need to pass the HEAP_ZERO_MEMORY flag for the setcount expansion (same limitation for heap_alloc/realloc here).
Secondly, about the common allocation. I'm not sure that's going to work in the future because the allocation itself will be changed in NODATA helpers (since it won't use normal item structure), so they won't be identical anymore.
And about the HeapSize calls -- well, yeah I agree that it should be changed, but I didn't want to overcomplicate this patchset for changing too much stuff. I'll look into it.
Plewase do this. Part of the reason this patch is proving difficult is the messy allocation scheme that already exists. Let's tidy that up first. Everything should fall back to a common allocator, once that's done, it becomes easy to alter the size that that will allocate if we shift to using a byte array for the nodata multiselect case.
The only difference I can see is that the custom allocator deals with sizes in bytes, instead of sizes in "number of items", otherwise there's no way to share it. I'll try something like that, hope that's fine.
so this looks like if (!IS_MULTISELECT(descr)) return index != descr->selected_item; return descr->items[index].selected;
and you'll extend it when nodata multiselect is optimized.
Huw.
Well the reason I wanted to avoid that, was due to wanting to move all implementation detail of nodata stuff into NODATA_* helpers. However, if I won't use the NODATA_is_sel helper at all and just implement everything in this function, then it doesn't make a difference I guess.
We're trying to stop you pulling the nodata stuff into separate functions, but rather use helpers to abstract certain operations (like checking the selected state and allocation). Those helpers will then need to worry about the details.
Huw.
Well I was trying to abstract the operations also within the helpers (so helpers within helpers!) for NODATA :-) But nevermind.
On Wed, Nov 21, 2018 at 07:26:05PM +0200, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 7:04 PM Huw Davies <huw(a)codeweavers.com> wrote:
Plewase do this. Part of the reason this patch is proving difficult is the messy allocation scheme that already exists. Let's tidy that up first. Everything should fall back to a common allocator, once that's done, it becomes easy to alter the size that that will allocate if we shift to using a byte array for the nodata multiselect case.
The only difference I can see is that the custom allocator deals with sizes in bytes, instead of sizes in "number of items", otherwise there's no way to share it. I'll try something like that, hope that's fine.
The common allocator would take number of items, it would then pass that on to heap_realloc() having done the appropriate multiplication.
We're trying to stop you pulling the nodata stuff into separate functions, but rather use helpers to abstract certain operations (like checking the selected state and allocation). Those helpers will then need to worry about the details.
Well I was trying to abstract the operations also within the helpers (so helpers within helpers!) for NODATA :-) But nevermind.
But you ended up scattering calls to these specialized helpers throughout the code, which resulted in a mess. Huw.
On Wed, Nov 21, 2018 at 7:33 PM Huw Davies <huw(a)codeweavers.com> wrote:
On Wed, Nov 21, 2018 at 07:26:05PM +0200, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 7:04 PM Huw Davies <huw(a)codeweavers.com> wrote:
Plewase do this. Part of the reason this patch is proving difficult is the messy allocation scheme that already exists. Let's tidy that up first. Everything should fall back to a common allocator, once that's done, it becomes easy to alter the size that that will allocate if we shift to using a byte array for the nodata multiselect case.
The only difference I can see is that the custom allocator deals with sizes in bytes, instead of sizes in "number of items", otherwise there's no way to share it. I'll try something like that, hope that's fine.
The common allocator would take number of items, it would then pass that on to heap_realloc() having done the appropriate multiplication.
But that's exactly what InitStorage is currently doing (barring the bug it has, since my bugfix got rejected :-/ ), so why not just use InitStorage? My concern is: how would that be "common" then for the case where we need to allocate items *without* multiplying by anything? Consider the case where we need to allocate byte array. If it multiplies by sizeof(LB_ITEMDATA) it will over-allocate by more than an order of magnitude in that case. So it's not so "common" after all and only used in one place (InitStorage) which makes it pointless.
On Wed, Nov 21, 2018 at 07:45:01PM +0200, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 7:33 PM Huw Davies <huw(a)codeweavers.com> wrote:
On Wed, Nov 21, 2018 at 07:26:05PM +0200, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 7:04 PM Huw Davies <huw(a)codeweavers.com> wrote:
Plewase do this. Part of the reason this patch is proving difficult is the messy allocation scheme that already exists. Let's tidy that up first. Everything should fall back to a common allocator, once that's done, it becomes easy to alter the size that that will allocate if we shift to using a byte array for the nodata multiselect case.
The only difference I can see is that the custom allocator deals with sizes in bytes, instead of sizes in "number of items", otherwise there's no way to share it. I'll try something like that, hope that's fine.
The common allocator would take number of items, it would then pass that on to heap_realloc() having done the appropriate multiplication.
But that's exactly what InitStorage is currently doing (barring the bug it has, since my bugfix got rejected :-/ ), so why not just use InitStorage?
No it isn't, InitStorage only increments the storage.
My concern is: how would that be "common" then for the case where we need to allocate items *without* multiplying by anything?
It will always multiply by something, that number might be sizeof(BYTE). The function will decide what to multiply by based on descr->style. This really isn't that complicated... Huw.
On Wed, Nov 21, 2018 at 8:07 PM Huw Davies <huw(a)codeweavers.com> wrote:
It will always multiply by something, that number might be sizeof(BYTE). The function will decide what to multiply by based on descr->style. This really isn't that complicated...
Huw.
Sorry, gmail bugged out on me without updating, and I saw your reply only after sending patches in (they use the previous approach but split from the patch). I didn't mean to sound like I ignored this approach, I'll see what I can do tomorrow about it if you still dislike this way. That said, the allocators for SetCount also need to zero out (rounded, for future) memory so I think it's still plausible to have them separated and avoid style checks. Also, using one common allocator will also complicate it if we ever change to a bit array, since we can't multiply by 1/8. Anyway like I said I will resend tomorrow if you don't change your mind about it, since i just saw your reply and I already sent them :-/
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This is placed here *after* LBS_NODATA's implementation because otherwise the test would timeout or run out of memory (since without LBS_NODATA it ends up allocating forever until it runs out of memory). 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 eeb1c7b..5cb42d9 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -1871,6 +1871,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 ); + /* 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); -- 2.19.1
On Wed, Nov 21, 2018 at 5:21 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Please use message sequence for this one, see how ok_sequence() is used.
I'm sorry I missed your reply, I'll resend it fixed tomorrow (please review rest of patches if you can so I can fix more at once).
On 11/21/18 11:30 PM, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 5:21 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Please use message sequence for this one, see how ok_sequence() is used.
I'm sorry I missed your reply, I'll resend it fixed tomorrow (please review rest of patches if you can so I can fix more at once).
I'll try, but please keep it around 5 patches in a batch. I see patch 10 introduces NODATA_* helpers again, and I already explained that we don't need that.
On Wed, Nov 21, 2018 at 10:42 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 11/21/18 11:30 PM, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 5:21 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Please use message sequence for this one, see how ok_sequence() is used.
I'm sorry I missed your reply, I'll resend it fixed tomorrow (please review rest of patches if you can so I can fix more at once).
I'll try, but please keep it around 5 patches in a batch.
I see patch 10 introduces NODATA_* helpers again, and I already explained that we don't need that.
I thought you meant just the NODATA_is_sel helper, which was removed. The NODATA helpers will be very useful for maintenance and to ease changes when implementing multi-selection. Consider the fact that we'll need at least NODATA behavior for Expand, Shrink, InsertItem, RemoveItem, GetSelItems, SelectItemRange, and perhaps GetSelCount. The helpers allow to (1) comment everything above them so their purpose and behavior is clear and (2) allow very easy maintenance and changes to the storage since all you have to care about are the helpers. For example, suppose we want to modify the byte array of multi-selection into the range-based storage as you initially suggested (as per ListView). Or perhaps we want to change it to a bit array, which is probably simpler to implement, and provides nice gains and much lower memory usage. Either way, it's easy to make these changes. To do that, we simply change the relevant NODATA helpers in a new patch, and that's it. The patch will be clean and there won't be side-effects such as "did I forget to change X?" and you won't have to look through the whole listbox code to figure that out. It keeps the NODATA internal implementation (storage etc) separate so that it can be cleanly modified without affecting the rest of the code. It's all done to make changing it much easier. In short, I think the NODATA helpers massively ease maintenance and future changes to the storage layout. These are just first step to make the multi-selection patch smaller. This is another reason the allocators are done also in NODATA helpers, because obviously if we later go by the ListView route of storing ranges of selections, it will have to be completely separate from the normal allocation (works totally differently). What is so bad about them?
On 11/22/18 10:22 AM, Gabriel Ivăncescu wrote:
<snip>
What is so bad about them? I really don't want to be mean but could you please stop whining? We know you're trying to help the project but you're doing it wrong. Developers read your code and give some feedback and help you making it right. Please stop fighting and always asking why. Moreover, you'll improve your code quality and could spend more time writing stuff instead of writing e-mails. Really, keep it simple, small patch set, add conformance tests and leave the performance "issue" for a moment. Correct code first, optimizations after. Remember you're not writing code for you but for other people.
On Thu, 22 Nov 2018 at 22:57, M. GOUJON <ale.goujon(a)gmail.com> wrote:
On 11/22/18 10:22 AM, Gabriel Ivăncescu wrote:
What is so bad about them? I really don't want to be mean but could you please stop whining? We know you're trying to help the project but you're doing it wrong. Developers read your code and give some feedback and help you making it right. Please stop fighting and always asking why.
Actually, I'd prefer people ask about things they don't understand, instead of just blindly following the instructions. Of course figuring things out on your own is also a valuable skill.
participants (5)
-
Gabriel Ivăncescu -
Henri Verbeet -
Huw Davies -
M. GOUJON -
Nikolay Sivov