[PATCH v2 08/10] comctl32/listbox: Send LB_RESETCONTENT from RemoveItem rather than DeleteItem

[Gabriel Ivăncescu]

On 2/12/19 11:32 AM, Huw Davies wrote:
> On Mon, Feb 11, 2019 at 07:03:11PM +0200, Gabriel Ivăncescu wrote:
>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>> ---
>>
>> Ok, this is how I ended up making the code simpler later. The reason being
>> that it really makes no sense at all to send LB_RESETCONTENT from within
>> DeleteItem: that function is called *from* ResetContent itself and it only
>> happened to work since it updated nb_items afterward. It was too fragile
>> to begin with.
>>
>> Note that LB_RESETCONTENT already calls DeleteItem, so we can simply re-use
>> that, and IMO this makes much more sense and will simplify the code in the
>> last patch.
> 
> Good, this is clearly how it should be done.
> 
>   
>>   dlls/comctl32/listbox.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
>> index 633e7c6..0f74e80 100644
>> --- a/dlls/comctl32/listbox.c
>> +++ b/dlls/comctl32/listbox.c
>> @@ -1660,13 +1660,9 @@ static LRESULT LISTBOX_InsertString( LB_DESCR *descr, INT index, LPCWSTR str )
>>    */
>>   static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index )
>>   {
>> -    /* save the item data before it gets freed by LB_RESETCONTENT */
>>       ULONG_PTR item_data = descr->items[index].data;
>>       LPWSTR item_str = descr->items[index].str;
> 
> As a nit-pick, there's no need to have these temp variables anymore,
> you can just call the getter helpers as needed. (Yes, we'll call
> get_item_data() twice, but it'll almost certainly get inlined anyway).
> 
> Huw.
> 

I have one question about this. DeleteItem shouldn't do anything for 
LBS_NODATA (not send any messages, etc). Should I still use the helpers 
and change the code, or leave it as it is (perhaps just getting rid of 
temporaries and using directly)?

We don't have tests for this yet in the Wine tree, because of 
multi-select nodata listboxes not being implemented, I wanted to send 
the tests after that because then I won't have to special-case "todo" 
wines for multi-select cases, etc.