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

[Ken Thomases]

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.

-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.