[PATCH v4 1/4] comctl32/listbox: Move the item data insertion into a separate function
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This is a no-op patch. nb_items is incremented before the call and this is taken care of in the helper function. This is for two reasons: 1) This simplifies the nodata patch (only adds a single line to insert_item_data). 2) This matches what RemoveItem does, so it's more consistent. dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 50ff5c5..330d3ae 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) descr->items[index].selected = state; } +static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) +{ + LB_ITEMDATA *item; + + item = &descr->items[index]; + + if (index < descr->nb_items - 1) + RtlMoveMemory(item + 1, item, + (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA)); + item->str = str; + item->data = HAS_STRINGS(descr) ? 0 : data; + item->height = 0; + item->selected = FALSE; + + /* Get item height */ + if (descr->style & LBS_OWNERDRAWVARIABLE) + { + MEASUREITEMSTRUCT mis; + UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + + mis.CtlType = ODT_LISTBOX; + mis.CtlID = id; + mis.itemID = index; + mis.itemData = data; + mis.itemHeight = descr->item_height; + SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); + item->height = mis.itemHeight ? mis.itemHeight : 1; + TRACE("[%p]: measure item %d (%s) = %d\n", + descr->self, index, str ? debugstr_w(str) : "", item->height ); + } +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -1553,42 +1585,14 @@ static void LISTBOX_MoveCaret( LB_DESCR *descr, INT index, BOOL fully_visible ) static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index, LPWSTR str, ULONG_PTR data ) { - LB_ITEMDATA *item; INT oldfocus = descr->focus_item; if (index == -1) index = descr->nb_items; else if ((index < 0) || (index > descr->nb_items)) return LB_ERR; if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR; - /* Insert the item structure */ - - item = &descr->items[index]; - if (index < descr->nb_items) - RtlMoveMemory( item + 1, item, - (descr->nb_items - index) * sizeof(LB_ITEMDATA) ); - item->str = str; - item->data = HAS_STRINGS(descr) ? 0 : data; - item->height = 0; - item->selected = FALSE; descr->nb_items++; - - /* Get item height */ - - if (descr->style & LBS_OWNERDRAWVARIABLE) - { - MEASUREITEMSTRUCT mis; - UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); - - mis.CtlType = ODT_LISTBOX; - mis.CtlID = id; - mis.itemID = index; - mis.itemData = data; - mis.itemHeight = descr->item_height; - SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); - item->height = mis.itemHeight ? mis.itemHeight : 1; - TRACE("[%p]: measure item %d (%s) = %d\n", - descr->self, index, str ? debugstr_w(str) : "", item->height ); - } + insert_item_data(descr, index, str, data); /* Repaint the items */ -- 2.20.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 330d3ae..214bfb0 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -126,6 +126,7 @@ typedef enum static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE; static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ); static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { @@ -208,6 +209,14 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d } } +static void remove_item_data(LB_DESCR *descr, INT index) +{ + LISTBOX_DeleteItem(descr, index); + if (index < descr->nb_items) + RtlMoveMemory(&descr->items[index], &descr->items[index + 1], + (descr->nb_items - index) * sizeof(LB_ITEMDATA)); +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize * @@ -1689,8 +1698,6 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index ) */ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) { - LB_ITEMDATA *item; - if ((index < 0) || (index >= descr->nb_items)) return LB_ERR; /* We need to invalidate the original rect instead of the updated one. */ @@ -1702,16 +1709,9 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) return LB_OKAY; } descr->nb_items--; - LISTBOX_DeleteItem( descr, index ); - - /* Remove the item */ - - item = &descr->items[index]; - if (index < descr->nb_items) - RtlMoveMemory( item, item + 1, - (descr->nb_items - index) * sizeof(LB_ITEMDATA) ); - if (descr->anchor_item == descr->nb_items) descr->anchor_item--; + remove_item_data(descr, index); + descr->anchor_item = min(descr->anchor_item, descr->nb_items - 1); resize_storage(descr, descr->nb_items); /* Repaint the items */ -- 2.20.1
On Mon, Feb 18, 2019 at 03:47:52PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 330d3ae..214bfb0 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -126,6 +126,7 @@ typedef enum static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect ); +static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index );
static BOOL resize_storage(LB_DESCR *descr, UINT items_size) { @@ -208,6 +209,14 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d } }
+static void remove_item_data(LB_DESCR *descr, INT index) +{ + LISTBOX_DeleteItem(descr, index);
Similarly let's leave LISTBOX_DeleteItem in RemoveItem().
+ if (index < descr->nb_items) + RtlMoveMemory(&descr->items[index], &descr->items[index + 1], + (descr->nb_items - index) * sizeof(LB_ITEMDATA)); +} + /*********************************************************************** * LISTBOX_GetCurrentPageSize *
The LBS_NODATA style's purpose is to drastically improve performance and memory usage on very large lists, since they should function as virtual lists. Thus, don't store any data for single-selection listboxes (descr->items always NULL). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v4: Get rid of the nodata helpers and open code the checks. I have kept the check in ResetContent because it reduces algorithm complexity from O(n) to O(1) and does not depend on the list size. LBS_NODATA listboxes can be large and possibly have millions of virtual items (that's what they are for), so I think a single extra line check is really worth it to avoid this scenario (it can reduce annoying stuttering when resetting such a listbox to instant), and also to be consistent with SetCount(0) in effect. The linked bug report is an indirect example of such case (an IDE with a large database: https://forum.winehq.org/viewtopic.php?f=8&t=17812). As a bonus, it also cleans up the formatting since it indents the line anyway. This will also help with multi-selection nodata listboxes later. dlls/comctl32/listbox.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 214bfb0..032a3b3 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -19,7 +19,7 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA * * TODO: - * - LBS_NODATA + * - LBS_NODATA for multi-selection listboxes */ #include <string.h> @@ -136,17 +136,20 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size) items_size + LB_ARRAY_GRANULARITY * 2 < descr->items_size) { items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1); - items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA)); - if (!items) + if ((descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) != LBS_NODATA) { - SEND_NOTIFICATION(descr, LBN_ERRSPACE); - return FALSE; + items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA)); + if (!items) + { + SEND_NOTIFICATION(descr, LBN_ERRSPACE); + return FALSE; + } + descr->items = items; } descr->items_size = items_size; - descr->items = items; } - if ((descr->style & LBS_NODATA) && items_size > descr->nb_items) + if ((descr->style & LBS_NODATA) && descr->items && items_size > descr->nb_items) { memset(&descr->items[descr->nb_items], 0, (items_size - descr->nb_items) * sizeof(LB_ITEMDATA)); @@ -181,6 +184,7 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d { LB_ITEMDATA *item; + if (!descr->items) return; item = &descr->items[index]; if (index < descr->nb_items - 1) @@ -211,6 +215,8 @@ static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR d static void remove_item_data(LB_DESCR *descr, INT index) { + if (!descr->items) return; + LISTBOX_DeleteItem(descr, index); if (index < descr->nb_items) RtlMoveMemory(&descr->items[index], &descr->items[index + 1], @@ -1751,7 +1757,8 @@ static void LISTBOX_ResetContent( LB_DESCR *descr ) { INT i; - for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i); + if (!(descr->style & LBS_NODATA)) + for(i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i); HeapFree( GetProcessHeap(), 0, descr->items ); descr->nb_items = 0; descr->top_item = 0; -- 2.20.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/tests/listbox.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c index 0cdcdea..de73ad2 100644 --- a/dlls/comctl32/tests/listbox.c +++ b/dlls/comctl32/tests/listbox.c @@ -1996,6 +1996,11 @@ static void test_set_count( void ) GetUpdateRect( listbox, &r, TRUE ); ok( !IsRectEmpty( &r ), "got empty rect\n"); + ret = SendMessageA( listbox, LB_SETCOUNT, -5, 0 ); + ok( ret == 0, "got %d\n", ret ); + ret = SendMessageA( listbox, LB_GETCOUNT, 0, 0 ); + ok( ret == -5, "got %d\n", ret ); + DestroyWindow( listbox ); for (i = 0; i < ARRAY_SIZE(styles); ++i) -- 2.20.1
On Mon, Feb 18, 2019 at 03:47:51PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
This is a no-op patch. nb_items is incremented before the call and this is taken care of in the helper function. This is for two reasons:
1) This simplifies the nodata patch (only adds a single line to insert_item_data). 2) This matches what RemoveItem does, so it's more consistent.
dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 50ff5c5..330d3ae 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) descr->items[index].selected = state; }
+static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) +{ + LB_ITEMDATA *item; + + item = &descr->items[index]; + + if (index < descr->nb_items - 1) + RtlMoveMemory(item + 1, item, + (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA)); + item->str = str; + item->data = HAS_STRINGS(descr) ? 0 : data; + item->height = 0; + item->selected = FALSE; + + /* Get item height */ + if (descr->style & LBS_OWNERDRAWVARIABLE) + { + MEASUREITEMSTRUCT mis; + UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + + mis.CtlType = ODT_LISTBOX; + mis.CtlID = id; + mis.itemID = index; + mis.itemData = data; + mis.itemHeight = descr->item_height; + SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); + item->height = mis.itemHeight ? mis.itemHeight : 1; + TRACE("[%p]: measure item %d (%s) = %d\n", + descr->self, index, str ? debugstr_w(str) : "", item->height ); + }
Let's leave the OWNERDRAWVARIABLE bit in InsertItem. insert_item_data() should just be about manipulating the items array. You'll want to add a set_item_height() helper first. Huw.
On 2/19/19 12:14 PM, Huw Davies wrote:
On Mon, Feb 18, 2019 at 03:47:51PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
This is a no-op patch. nb_items is incremented before the call and this is taken care of in the helper function. This is for two reasons:
1) This simplifies the nodata patch (only adds a single line to insert_item_data). 2) This matches what RemoveItem does, so it's more consistent.
dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 50ff5c5..330d3ae 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) descr->items[index].selected = state; }
+static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) +{ + LB_ITEMDATA *item; + + item = &descr->items[index]; + + if (index < descr->nb_items - 1) + RtlMoveMemory(item + 1, item, + (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA)); + item->str = str; + item->data = HAS_STRINGS(descr) ? 0 : data; + item->height = 0; + item->selected = FALSE; + + /* Get item height */ + if (descr->style & LBS_OWNERDRAWVARIABLE) + { + MEASUREITEMSTRUCT mis; + UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + + mis.CtlType = ODT_LISTBOX; + mis.CtlID = id; + mis.itemID = index; + mis.itemData = data; + mis.itemHeight = descr->item_height; + SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); + item->height = mis.itemHeight ? mis.itemHeight : 1; + TRACE("[%p]: measure item %d (%s) = %d\n", + descr->self, index, str ? debugstr_w(str) : "", item->height ); + }
Let's leave the OWNERDRAWVARIABLE bit in InsertItem. insert_item_data() should just be about manipulating the items array. You'll want to add a set_item_height() helper first.
Huw.
LBS_OWNERDRAWVARIABLE is incompatible with LBS_NODATA (and disabled when listbox is created, due to OWNERDRAWFIXED which is mandatory), should I still add a set_item_height helper? It's not really needed in this case. Besides, even if we assume that it's not incompatible, it sounds weird to send the WM_MEASUREITEM message and not do anything with the result in that situation (the helper would do nothing with nodata, right?). Of course, it doesn't happen in practice anyway but... :-)
On Tue, Feb 19, 2019 at 12:47:07PM +0200, Gabriel Ivăncescu wrote:
On 2/19/19 12:14 PM, Huw Davies wrote:
On Mon, Feb 18, 2019 at 03:47:51PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
This is a no-op patch. nb_items is incremented before the call and this is taken care of in the helper function. This is for two reasons:
1) This simplifies the nodata patch (only adds a single line to insert_item_data). 2) This matches what RemoveItem does, so it's more consistent.
dlls/comctl32/listbox.c | 62 ++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 50ff5c5..330d3ae 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -176,6 +176,38 @@ static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state) descr->items[index].selected = state; } +static void insert_item_data(LB_DESCR *descr, INT index, WCHAR *str, ULONG_PTR data) +{ + LB_ITEMDATA *item; + + item = &descr->items[index]; + + if (index < descr->nb_items - 1) + RtlMoveMemory(item + 1, item, + (descr->nb_items - 1 - index) * sizeof(LB_ITEMDATA)); + item->str = str; + item->data = HAS_STRINGS(descr) ? 0 : data; + item->height = 0; + item->selected = FALSE; + + /* Get item height */ + if (descr->style & LBS_OWNERDRAWVARIABLE) + { + MEASUREITEMSTRUCT mis; + UINT id = (UINT)GetWindowLongPtrW( descr->self, GWLP_ID ); + + mis.CtlType = ODT_LISTBOX; + mis.CtlID = id; + mis.itemID = index; + mis.itemData = data; + mis.itemHeight = descr->item_height; + SendMessageW( descr->owner, WM_MEASUREITEM, id, (LPARAM)&mis ); + item->height = mis.itemHeight ? mis.itemHeight : 1; + TRACE("[%p]: measure item %d (%s) = %d\n", + descr->self, index, str ? debugstr_w(str) : "", item->height ); + }
Let's leave the OWNERDRAWVARIABLE bit in InsertItem. insert_item_data() should just be about manipulating the items array. You'll want to add a set_item_height() helper first.
Huw.
LBS_OWNERDRAWVARIABLE is incompatible with LBS_NODATA (and disabled when listbox is created, due to OWNERDRAWFIXED which is mandatory), should I still add a set_item_height helper? It's not really needed in this case.
Yes, I know this. At the risk of repeating myself (but apparently I have to), the aim is to get rid of direct accesses to item, outside of the item g/setters. Huw.
participants (2)
-
Gabriel Ivăncescu -
Huw Davies