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

[Nikolay Sivov]

On 08/22/2018 06:49 PM, Ken Thomases wrote:

> To be honest, I don't clearly remember why I put the float cast in there.  I think perhaps to avoid potential (if unlikely) integer overflow.  We want something like MulDiv() but always rounding toward zero.  Perhaps just casting to INT64 would be better.

I like MulDiv better. Gabriel, do you think it's possible to get rid of 
some casts by changing variable types?

>
> -Ken
>
>> On Aug 22, 2018, at 7:02 AM, Gabriel Ivăncescu <gabrielopcode at gmail.com> wrote:
>>
>> 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.