Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
The case where count < descr->nb_items isn't really optimal, just stops redrawing right now. The other case (growing) should be optimal now, though.
dlls/comctl32/listbox.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index 2137ef8..01588bb 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1758,18 +1758,31 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) 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; + INT num = descr->nb_items; + if ((ret = LISTBOX_InitStorage(descr, count - num)) != LB_OKAY) + return ret; + memset(&descr->items[num], 0, (count - num) * sizeof(*descr->items)); + descr->nb_items = count; + + LISTBOX_UpdateScroll(descr); + LISTBOX_InvalidateItems(descr, num); + + /* If listbox was empty, set focus to the first item */ + if (count == 1) LISTBOX_SetCaretIndex(descr, 0, FALSE); } else if (count < descr->nb_items) { + /* FIXME: This is not really optimal */ + DWORD oldstyle = descr->style; + LISTBOX_SetRedraw(descr, FALSE); + while (count < descr->nb_items) if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0) - return ret; + break; + if (!(oldstyle & LBS_NOREDRAW)) LISTBOX_SetRedraw(descr, TRUE); + if (ret < 0) return ret; }
InvalidateRect( descr->self, NULL, TRUE );
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/user32/listbox.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index c8bd148..a8de8d3 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -1763,18 +1763,31 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) 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; + INT num = descr->nb_items; + if ((ret = LISTBOX_InitStorage(descr, count - num)) != LB_OKAY) + return ret; + memset(&descr->items[num], 0, (count - num) * sizeof(*descr->items)); + descr->nb_items = count; + + LISTBOX_UpdateScroll(descr); + LISTBOX_InvalidateItems(descr, num); + + /* If listbox was empty, set focus to the first item */ + if (count == 1) LISTBOX_SetCaretIndex(descr, 0, FALSE); } else if (count < descr->nb_items) { + /* FIXME: This is not really optimal */ + DWORD oldstyle = descr->style; + LISTBOX_SetRedraw(descr, FALSE); + while (count < descr->nb_items) if ((ret = LISTBOX_RemoveItem( descr, (descr->nb_items - 1) )) < 0) - return ret; + break; + if (!(oldstyle & LBS_NOREDRAW)) LISTBOX_SetRedraw(descr, TRUE); + if (ret < 0) return ret; }
InvalidateRect( descr->self, NULL, TRUE );
On Tue, Sep 18, 2018 at 11:15:58PM +0300, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
dlls/user32/listbox.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index c8bd148..a8de8d3 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -1763,18 +1763,31 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) 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;
INT num = descr->nb_items;if ((ret = LISTBOX_InitStorage(descr, count - num)) != LB_OKAY)return ret;memset(&descr->items[num], 0, (count - num) * sizeof(*descr->items));descr->nb_items = count;LISTBOX_UpdateScroll(descr);LISTBOX_InvalidateItems(descr, num);/* If listbox was empty, set focus to the first item */if (count == 1) LISTBOX_SetCaretIndex(descr, 0, FALSE);
Shouldn't this be if (num == 0) ? That would at least match the comment.
Huw.
On Wed, Sep 19, 2018 at 10:46 AM, Huw Davies huw@codeweavers.com wrote:
Shouldn't this be if (num == 0) ? That would at least match the comment.
Huw.
Yeah, you are right. I copied the relevant logic from InsertItem, but num == 0 definitely makes more sense.
On Wed, Sep 19, 2018 at 01:52:37PM +0300, Gabriel Ivăncescu wrote:
On Wed, Sep 19, 2018 at 10:46 AM, Huw Davies huw@codeweavers.com wrote:
Shouldn't this be if (num == 0) ? That would at least match the comment.
Huw.
Yeah, you are right. I copied the relevant logic from InsertItem, but num == 0 definitely makes more sense.
I'm also concerned that we won't get WM_MEASUREITEMs in the LBS_OWNERDRAWVARIABLE case.
Huw.
On Wed, Sep 19, 2018 at 1:55 PM, Huw Davies huw@codeweavers.com wrote:
I'm also concerned that we won't get WM_MEASUREITEMs in the LBS_OWNERDRAWVARIABLE case.
Huw.
I don't know if that's an issue since SetCount should theoretically be an "instant" operation and these items are just added at the end as empty. I think Windows doesn't even support SetCount without LBS_NODATA (which is not supported by Wine yet, I think I'll try working on it and implement it in an optimal way, i.e. with the minimum amount of memory needed, since it's meant for very very large lists).
But, at least for now, I can fall back to the original (unoptimal) behavior if OWNERDRAWVARIABLE is set, would that be acceptable?
On 19 Sep 2018, at 12:46, Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
On Wed, Sep 19, 2018 at 1:55 PM, Huw Davies huw@codeweavers.com wrote:
I'm also concerned that we won't get WM_MEASUREITEMs in the LBS_OWNERDRAWVARIABLE case.
Huw.
I don't know if that's an issue since SetCount should theoretically be an "instant" operation and these items are just added at the end as empty. I think Windows doesn't even support SetCount without LBS_NODATA (which is not supported by Wine yet, I think I'll try working on it and implement it in an optimal way, i.e. with the minimum amount of memory needed, since it's meant for very very large lists).
But, at least for now, I can fall back to the original (unoptimal) behavior if OWNERDRAWVARIABLE is set, would that be acceptable?
I think implementing LBS_NODATA should come before adding LB_SETCOUNT code that will need to be changed again. Also, adding a test to show that LB_SETCOUNT fails if the style is LBS_OWNERDRAWFIXED, for example, wouldn't hurt.
Huw.
On Wed, Sep 19, 2018 at 4:53 PM, Huw Davies huw@codeweavers.com wrote:
I think implementing LBS_NODATA should come before adding LB_SETCOUNT code that will need to be changed again. Also, adding a test to show that LB_SETCOUNT fails if the style is LBS_OWNERDRAWFIXED, for example, wouldn't hurt.
Huw.
To be honest, we should just fail SetCount if LBS_NODATA is not set, but maybe later not now, if tests prove this is Windows' behavior, which I'll check in a bit. Even if Wine doesn't support it, it still "works" currently (but very slow and suboptimal, there's some bugs around about it), so I think it should be left with this wrong behavior for now.
BTW I think you meant OWNERDRAWVARIABLE right? LBS_OWNERDRAWFIXED should work (and is actually required) for LBS_NODATA (and thus, LB_SETCOUNT), or I'm missing something.
I guess for now this patchset should be rejected until LBS_NODATA is implemented which would require a totally different patchset anyway, or I find some other behavior with tests.
On 19 Sep 2018, at 15:35, Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
On Wed, Sep 19, 2018 at 4:53 PM, Huw Davies huw@codeweavers.com wrote:
I think implementing LBS_NODATA should come before adding LB_SETCOUNT code that will need to be changed again. Also, adding a test to show that LB_SETCOUNT fails if the style is LBS_OWNERDRAWFIXED, for example, wouldn't hurt.
BTW I think you meant OWNERDRAWVARIABLE right? LBS_OWNERDRAWFIXED should work (and is actually required) for LBS_NODATA (and thus, LB_SETCOUNT), or I'm missing something.
I just meant using the current test for LB_SETCOUNT and removing the LBS_NODATA, which leaves LBS_OWNERDRAWFIXED on its own.
Huw.
On Wed, Sep 19, 2018 at 6:08 PM, Huw Davies huw@codeweavers.com wrote:
I just meant using the current test for LB_SETCOUNT and removing the LBS_NODATA, which leaves LBS_OWNERDRAWFIXED on its own.
Huw.
Yeah I've added a new test just after it, which actually seems to fail, along with a fix of course so that it doesn't have to be marked todo_wine. I'm sending it now.
-
Gabriel Ivăncescu -
Huw Davies