[PATCH 2/2] user32/listbox: Handle Mouse Wheel scrolling for multi-column listboxes properly

[Gabriel Ivăncescu]

Well the float "appears" to fix a bug, because pulScrollLines is
unsigned, so an INT cast is needed, otherwise the promotion rules will
do an unsigned multiplication. Float takes precedence so it converted
it to float and then it worked.

But IMO it's not the correct way to fix this at all, just cast
pulScrollLines to INT, as it's done on the next line as well, and now
the multiplication will be signed and it works fine (tested it
rigorously!)

On Wed, Aug 22, 2018 at 11:32 AM, Nikolay Sivov <nsivov at codeweavers.com> wrote:
> On 08/20/2018 09:53 PM, Gabriel Ivăncescu wrote:
>
>> Multi-column listboxes scroll horizontally, so each wheel tick must go an
>> entire page at a time instead of an item at a time. But we have to limit the
>> amount of scrolling in this case to avoid "jumping over" columns, if the
>> window is too small. This matches Windows behavior.
>>
>> The calculation has also been simplified to just integer arithmetic in all
>> cases, since the division (the only operation with a fractional result) was
>> immediately truncated to integer anyway, so the float cast was useless. This
>> works fine because the multiplication is done before the division
>> (parentheses have been added to emphasize this point).
>>
>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22253
>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>> ---
>>   dlls/user32/listbox.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c
>> index 991ab87..7256095 100644
>> --- a/dlls/user32/listbox.c
>> +++ b/dlls/user32/listbox.c
>> @@ -2007,10 +2007,21 @@ static LRESULT LISTBOX_HandleMouseWheel(LB_DESCR
>> *descr, SHORT delta )
>>         if (descr->wheel_remain && pulScrollLines)
>>       {
>> -        int cLineScroll;
>> -        pulScrollLines = min((UINT) descr->page_size, pulScrollLines);
>> -        cLineScroll = pulScrollLines * (float)descr->wheel_remain /
>> WHEEL_DELTA;
>> -        descr->wheel_remain -= WHEEL_DELTA * cLineScroll /
>> (int)pulScrollLines;
>> +        INT cLineScroll;
>> +        if (descr->style & LBS_MULTICOLUMN)
>> +        {
>> +            pulScrollLines = min((UINT)descr->width /
>> descr->column_width, pulScrollLines);
>> +            pulScrollLines = max(1, pulScrollLines);
>> +            cLineScroll = ((INT)pulScrollLines * descr->wheel_remain) /
>> WHEEL_DELTA;
>> +            descr->wheel_remain -= (cLineScroll * WHEEL_DELTA) /
>> (INT)pulScrollLines;
>> +            cLineScroll *= descr->page_size;
>> +        }
>> +        else
>> +        {
>> +            pulScrollLines = min((UINT) descr->page_size,
>> pulScrollLines);
>> +            cLineScroll = ((INT)pulScrollLines * descr->wheel_remain) /
>> WHEEL_DELTA;
>> +            descr->wheel_remain -= (cLineScroll * WHEEL_DELTA) /
>> (INT)pulScrollLines;
>> +        }
>>           LISTBOX_SetTopItem( descr, descr->top_item - cLineScroll, TRUE
>> );
>>       }
>>       return 0;
>
>
> Float cast was a deliberate addition it seems, and relatively recent one,
> see f42cfc04eb05fa266cfae0c64452686a68c24151, CC'ing Ken.