[PATCH 1/2] comctl32/listbox: Handle Mouse Wheel scrolling for multi-column listboxes properly
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(a)gmail.com> --- dlls/comctl32/listbox.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b3491bb..9275db9 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -2002,10 +2002,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; -- 1.9.1
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(a)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; -- 1.9.1
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(a)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.
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(a)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(a)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.
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(a)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(a)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(a)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.
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(a)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(a)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(a)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.
On Aug 22, 2018, at 12:48 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
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?
But MulDiv() rounds to the nearest integer (sometimes rounding up), which isn't acceptable. It can lead to scrolling too far and/or end up reversing the sign of wheel_remain. -Ken
On Wed, Aug 22, 2018 at 8:48 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
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?
I don't think integer overflow is an issue. WHEEL_DELTA is defined as 120, which is not that big. That means the result would have to be larger than 2^31/120 (in magnitude), i.e. 17895697 (this is the result *after* the division). That's a pretty large number for the amount of lines to scroll. I don't think it's ever possible in practice :) So honestly casting to INT64 sounds overkill to me. As for reducing the amount of casts: I initially thought that pulScrollLines can't be signed because the function SystemParametersInfoW needs it unsigned (output parameter), however it seems it takes a PVOID. So I can just set pulScrollLines itself to INT and get rid of the casts. I'll prepare a v2 patch, also for the other float casting patch series.
Sorry for another message, but MSDN says it *must* point to a UINT variable when SPI_GETWHEELSCROLLLINES is used. While in practice it doesn't make a difference, I guess I'll just use another variable instead. Hope that's fine.
participants (3)
-
Gabriel Ivăncescu -
Ken Thomases -
Nikolay Sivov