[PATCH v2 1/2] comctl32/listbox: Implement LBS_NODATA for single-selection listboxes
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- I reduced the patch to just single-selection changes so that it still works. After these are in, the multi-selection patches should be smaller. Please note that the new helper functions (such as NODATA_is_sel) should remain as-is because they will be extended later for multi-selection listboxes, even if they seem redundant for now, so that I can really trim down the size of the multi-selection patches. :-) By the way, this patch really looks larger than it is. Most of the changes are in SetCount and the helper functions. InsertItem and RemoveItem seem large, but the changes are minor -- it's mostly just indentation, so it should be simple to review. The reason I changed the entirety of SetCount is to reduce the amount of changes for the multi-selection patches that will come later, otherwise I'd have to go back-and-forth with indentation changes unless I used a goto :-/ v2: Fix a dumb mistake when rounding the size. dlls/comctl32/listbox.c | 280 ++++++++++++++++++++++++++++------------ 1 file changed, 201 insertions(+), 79 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index dbc7b4a..7ba6b1b 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,6 +125,71 @@ 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); +} + + + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -494,7 +559,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 ); @@ -517,16 +582,19 @@ 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_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) || + (!is_singlesel_NODATA(descr) && item->selected) ) + dis.itemState |= ODS_SELECTED; + if (!ignoreFocus && (descr->focus_item == index) && (descr->caret_on) && (descr->in_focus)) dis.itemState |= ODS_FOCUS; if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED; - dis.itemData = item->data; + dis.itemData = (descr->style & LBS_NODATA) ? 0 : item->data; dis.rcItem = *rect; TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n", - descr->self, index, debugstr_w(item->str), action, - dis.itemState, wine_dbgstr_rect(rect) ); + descr->self, index, debugstr_w((descr->style & LBS_NODATA) ? NULL : item->str), + action, dis.itemState, wine_dbgstr_rect(rect) ); SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis); SelectClipRgn( hdc, hrgn ); if (hrgn) DeleteObject( hrgn ); @@ -673,6 +741,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) { @@ -1440,10 +1511,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 ); @@ -1516,54 +1590,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 */ @@ -1678,28 +1757,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 ); @@ -1737,7 +1826,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) { INT i; - for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!is_singlesel_NODATA(descr)) + 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; @@ -1753,22 +1843,52 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) */ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) { - LRESULT ret; + INT orig_num; if (!(descr->style & LBS_NODATA)) return LB_ERR; + count = max(count, 0); - /* 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 ); @@ -2763,6 +2883,8 @@ 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; + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, wParam); return descr->items[wParam].selected; case LB_SETSEL: -- 2.19.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/listbox.c | 280 ++++++++++++++++++++++++++++++------------ 1 file changed, 201 insertions(+), 79 deletions(-) diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index 286a33b..e52ddf3 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -18,7 +18,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> @@ -150,6 +150,71 @@ const struct builtin_class_descr COMBOLBOX_builtin_class = }; +/*********************************************************************** + * 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); +} + + + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -519,7 +584,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 ); @@ -542,16 +607,19 @@ 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_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) || + (!is_singlesel_NODATA(descr) && item->selected) ) + dis.itemState |= ODS_SELECTED; + if (!ignoreFocus && (descr->focus_item == index) && (descr->caret_on) && (descr->in_focus)) dis.itemState |= ODS_FOCUS; if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED; - dis.itemData = item->data; + dis.itemData = (descr->style & LBS_NODATA) ? 0 : item->data; dis.rcItem = *rect; TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n", - descr->self, index, debugstr_w(item->str), action, - dis.itemState, wine_dbgstr_rect(rect) ); + descr->self, index, debugstr_w((descr->style & LBS_NODATA) ? NULL : item->str), + action, dis.itemState, wine_dbgstr_rect(rect) ); SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis); SelectClipRgn( hdc, hrgn ); if (hrgn) DeleteObject( hrgn ); @@ -698,6 +766,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) { @@ -1449,10 +1520,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 ); @@ -1525,54 +1599,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++; + /* 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 ); + /* 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 = str ? (ULONG_PTR)str : 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 = str ? (ULONG_PTR)str : 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 */ @@ -1687,28 +1766,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 ); @@ -1746,7 +1835,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) { INT i; - for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!is_singlesel_NODATA(descr)) + 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; @@ -1762,26 +1852,56 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) */ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) { - LRESULT ret; + INT orig_num; if (!(descr->style & LBS_NODATA)) { SetLastError(ERROR_SETCOUNT_ON_BAD_LB); return LB_ERR; } + count = max(count, 0); - /* 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 ); @@ -2866,6 +2986,8 @@ LRESULT ListBoxWndProc_common( HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam case LB_GETSEL: if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items)) return LB_ERR; + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, wParam); return descr->items[wParam].selected; case LB_SETSEL: -- 2.19.1
On 11/20/18 4:40 PM, Gabriel Ivăncescu wrote:
I reduced the patch to just single-selection changes so that it still works. After these are in, the multi-selection patches should be smaller.
This should be reduced further.
Please note that the new helper functions (such as NODATA_is_sel) should remain as-is because they will be extended later for multi-selection listboxes, even if they seem redundant for now, so that I can really trim down the size of the multi-selection patches.:-)
By the way, this patch really looks larger than it is. Most of the changes are in SetCount and the helper functions. InsertItem and RemoveItem seem large, but the changes are minor -- it's mostly just indentation, so it should be simple to review.
It is large, and again for no good reason. For example painting part:
@@ -494,7 +559,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, RECT r; HRGN hrgn;
- if (!item) + if (index >= descr->nb_items) {
'index' should be made unsigned throughout, unless there is a reason not to.
if (action == ODA_FOCUS) DrawFocusRect( hdc, rect ); @@ -517,16 +582,19 @@ 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_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) || + (!is_singlesel_NODATA(descr) && item->selected) ) + dis.itemState |= ODS_SELECTED; +
This is really ugly.
if (!ignoreFocus && (descr->focus_item == index) && (descr->caret_on) && (descr->in_focus)) dis.itemState |= ODS_FOCUS; if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED; - dis.itemData = item->data; + dis.itemData = (descr->style & LBS_NODATA) ? 0 : item->data; dis.rcItem = *rect; TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n", - descr->self, index, debugstr_w(item->str), action, - dis.itemState, wine_dbgstr_rect(rect) ); + descr->self, index, debugstr_w((descr->style & LBS_NODATA) ? NULL : item->str), + action, dis.itemState, wine_dbgstr_rect(rect) ); SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis); SelectClipRgn( hdc, hrgn ); if (hrgn) DeleteObject( hrgn );
Instead of spreading style checks just make sure 'item' is unset correctly.
@@ -1678,28 +1757,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; + } + }
We'll need additional test for that.
- for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!is_singlesel_NODATA(descr)) + for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i); Is it the same as testing for null items array?
static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) { - LRESULT ret; + INT orig_num;
if (!(descr->style & LBS_NODATA)) return LB_ERR; + count = max(count, 0); Do we have tests for that?
case LB_GETSEL: if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items)) return LB_ERR; + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, wParam); return descr->items[wParam].selected;
Do you need this? For single selection case you'll only need to check wParam == selected_item, regardless of LBS_NODATA presence. Regarding item storage, it's important to test what happens when multiselection styles are changed after window creation. Speaking of optimizations, having additional field for a number of selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.
On Wed, Nov 21, 2018 at 12:39 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
For example painting part:
@@ -494,7 +559,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, RECT r; HRGN hrgn;
- if (!item) + if (index >= descr->nb_items) {
'index' should be made unsigned throughout, unless there is a reason not to.
I don't mind making it unsigned but I don't see what it has to do with the patch and didn't want to make more changes than necessary. This check is changed because checking for NULL item will not work if (1) index is zero (valid) and (2) we have single-selection LBS_NODATA. In this case it will erroneously assume it's out of bounds (NULL + 0 = NULL) when it's a perfectly valid index (zero). So it's related to LBS_NODATA that's why it's in this patch.
if (action == ODA_FOCUS) DrawFocusRect( hdc, rect ); @@ -517,16 +582,19 @@ 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_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) || + (!is_singlesel_NODATA(descr) && item->selected) ) + dis.itemState |= ODS_SELECTED; +
This is really ugly.
Right, I'll just set item to NULL and if so assume it's LBS_NODATA.
@@ -1678,28 +1757,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; + } + }
We'll need additional test for that.
This is not new behavior though. I did it because that's what happens with DeleteItem when it removes the last item, so that it keeps the same behavior as before. If this isn't based on a test then the normal listbox code is wrong too.
- for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!is_singlesel_NODATA(descr)) + for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i); Is it the same as testing for null items array?
For now yes since we only have single-selection listboxes. I'll change it to NULL check then.
static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) { - LRESULT ret; + INT orig_num;
if (!(descr->style & LBS_NODATA)) return LB_ERR; + count = max(count, 0); Do we have tests for that?
Ah no, I'll see what I can add here as a test. I just did it to prevent it from blowing up on negative values.
case LB_GETSEL: if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items)) return LB_ERR; + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, wParam); return descr->items[wParam].selected;
Do you need this? For single selection case you'll only need to check wParam == selected_item, regardless of LBS_NODATA presence.
But, as I mentioned in the comments of the patch, this will make the multi-selection patch smaller, since it will share this code path. This will become a check for NODATA rather than just single-selection and anything else will be identical (the helper itself will be extended). If I change it for single-selection, I then have to add *another* check for LBS_NODATA in the next patch, and also keep the normal code path. IMO let's just keep it like this.
Regarding item storage, it's important to test what happens when multiselection styles are changed after window creation.
Well, the current code assumes they can't be changed since it never updates them. I don't see why LBS_NODATA has to be special here, and I really didn't want to complicate this more than it is, but to keep the existing behavior where possible. I'll see what I can do about tests though.
Speaking of optimizations, having additional field for a number of selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.
Yeah, but it will require some changes in existing cases (not just LBS_NODATA) to keep track of them (like when setting the range of selections), I think we should leave that for later. I don't know if it's very worth it since getting the count should be fast anyway on multi-selection listboxes (and usually it's not spammed by apps).
On 11/21/18 1:12 PM, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 12:39 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
For example painting part:
@@ -494,7 +559,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, RECT r; HRGN hrgn;
- if (!item) + if (index >= descr->nb_items) { 'index' should be made unsigned throughout, unless there is a reason not to.
I don't mind making it unsigned but I don't see what it has to do with the patch and didn't want to make more changes than necessary. This condition will depend on it.
This check is changed because checking for NULL item will not work if (1) index is zero (valid) and (2) we have single-selection LBS_NODATA. In this case it will erroneously assume it's out of bounds (NULL + 0 = NULL) when it's a perfectly valid index (zero). So it's related to LBS_NODATA that's why it's in this patch.
if (action == ODA_FOCUS) DrawFocusRect( hdc, rect ); @@ -517,16 +582,19 @@ 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_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) || + (!is_singlesel_NODATA(descr) && item->selected) ) + dis.itemState |= ODS_SELECTED; +
This is really ugly.
Right, I'll just set item to NULL and if so assume it's LBS_NODATA.
@@ -1678,28 +1757,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; + } + } We'll need additional test for that.
This is not new behavior though. I did it because that's what happens with DeleteItem when it removes the last item, so that it keeps the same behavior as before. If this isn't based on a test then the normal listbox code is wrong too. Yes, but now it will be duplicated in two places.
- for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!is_singlesel_NODATA(descr)) + for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i); Is it the same as testing for null items array?
For now yes since we only have single-selection listboxes. I'll change it to NULL check then. It doesn't matter, for multiselect case you'll have to cleanup one way or another.
static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) { - LRESULT ret; + INT orig_num;
if (!(descr->style & LBS_NODATA)) return LB_ERR; + count = max(count, 0); Do we have tests for that?
Ah no, I'll see what I can add here as a test. I just did it to prevent it from blowing up on negative values. From what I can tell negative values are accepted.
case LB_GETSEL: if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items)) return LB_ERR; + if (is_singlesel_NODATA(descr)) + return NODATA_is_sel(descr, wParam); return descr->items[wParam].selected;
Do you need this? For single selection case you'll only need to check wParam == selected_item, regardless of LBS_NODATA presence.
But, as I mentioned in the comments of the patch, this will make the multi-selection patch smaller, since it will share this code path. This will become a check for NODATA rather than just single-selection and anything else will be identical (the helper itself will be extended).
If I change it for single-selection, I then have to add *another* check for LBS_NODATA in the next patch, and also keep the normal code path. IMO let's just keep it like this. I don't see why NODATA would be special here.
Regarding item storage, it's important to test what happens when multiselection styles are changed after window creation.
Well, the current code assumes they can't be changed since it never updates them. I don't see why LBS_NODATA has to be special here, and I really didn't want to complicate this more than it is, but to keep the existing behavior where possible.
I'll see what I can do about tests though. It doesn't matter what is currently assumed, if we want to improve or optimize anything, we'll have to make sure it's correct.
Speaking of optimizations, having additional field for a number of selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.
Yeah, but it will require some changes in existing cases (not just LBS_NODATA) to keep track of them (like when setting the range of selections), I think we should leave that for later. I don't know if it's very worth it since getting the count should be fast anyway on multi-selection listboxes (and usually it's not spammed by apps). I think it's a very small change, and yes, it's separate and unrelated to NODATA.
On Wed, Nov 21, 2018 at 12:26 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
This is not new behavior though. I did it because that's what happens with DeleteItem when it removes the last item, so that it keeps the same behavior as before. If this isn't based on a test then the normal listbox code is wrong too. Yes, but now it will be duplicated in two places.
Ok. If tests show that it's not sent, can I just call the function(s) directly or do you have some other approach in mind?
But, as I mentioned in the comments of the patch, this will make the multi-selection patch smaller, since it will share this code path. This will become a check for NODATA rather than just single-selection and anything else will be identical (the helper itself will be extended).
If I change it for single-selection, I then have to add *another* check for LBS_NODATA in the next patch, and also keep the normal code path. IMO let's just keep it like this.
I don't see why NODATA would be special here.
Of course it is special, since it has no data and we can't dereference the item struct (and for multi-selection it will be stored differently too, so again another "special case"). But, if we leave it like it is currently, all this implementation detail will happen only within the NODATA helpers, so it's separated, which is better IMO. I mean, when multi-selection LBS_NODATA is implemented (in any way), we *will* have to add a check here for it, there's literally no way around it, if we don't do it now. Might as well just share the code path with all LBS_NODATA and keep it inside those helpers, as an implementation detail (so that, if it's changed in the future, it will be easily done so only within the helpers). And of course it doesn't make this patch any worse off by itself anyway...
Speaking of optimizations, having additional field for a number of selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.
Yeah, but it will require some changes in existing cases (not just LBS_NODATA) to keep track of them (like when setting the range of selections), I think we should leave that for later. I don't know if it's very worth it since getting the count should be fast anyway on multi-selection listboxes (and usually it's not spammed by apps).
I think it's a very small change, and yes, it's separate and unrelated to NODATA.
Well that's only for multi-selection listboxes, I'll look into it after at least the single-selection LBS_NODATA is done. :-)
participants (2)
-
Gabriel Ivăncescu -
Nikolay Sivov