Skip to content

Commit

Permalink
Fix signed/unsigned arithmetic bug during scrolling (#13256)
Browse files Browse the repository at this point in the history
ed27737 contains a regression where (pseudocode)
```c
unsigned long ulActualDelta;
short ScreenInfo.WheelDelta;
delta *= (ScreenInfo.WheelDelta / (short)ulActualDelta);
//                                ^^^^^^^
```
was changed to
```c
delta *= (ScreenInfo.WheelDelta / ulActualDelta);
```

Due to `ulActualDelta` being unsigned, the new code casts the signed integer
to a unsigned one first, before doing the division. This causes scrolling
downwards (`WheelDelta` is negative) to appear as a large positive `delta`.

## PR Checklist
* [x] Closes #13253
* [x] I work here

## Validation Steps Performed
* Scrolling up/down works in OpenConsole again ✅
  • Loading branch information
lhecker authored Jun 9, 2022
1 parent cd35bc5 commit 730eb5f
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 11 deletions.
18 changes: 9 additions & 9 deletions src/host/scrolling.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
using namespace Microsoft::Console::Interactivity;
using namespace Microsoft::Console::Types;

ULONG Scrolling::s_ucWheelScrollLines = 0;
ULONG Scrolling::s_ucWheelScrollChars = 0;
til::CoordType Scrolling::s_ucWheelScrollLines = 0;
til::CoordType Scrolling::s_ucWheelScrollChars = 0;

void Scrolling::s_UpdateSystemMetrics()
{
s_ucWheelScrollLines = ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollLines();
s_ucWheelScrollChars = ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollCharacters();
s_ucWheelScrollLines = ::base::saturated_cast<decltype(s_ucWheelScrollLines)>(ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollLines());
s_ucWheelScrollChars = ::base::saturated_cast<decltype(s_ucWheelScrollChars)>(ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollCharacters());
}

bool Scrolling::s_IsInScrollMode()
Expand Down Expand Up @@ -112,7 +112,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
if (isMouseWheel && s_ucWheelScrollLines > 0)
{
// Rounding could cause this to be zero if gucWSL is bigger than 240 or so.
const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollLines, 1ul);
const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollLines, 1);

// If we change direction we need to throw away any remainder we may have in the other direction.
if ((ScreenInfo.WheelDelta > 0) == (wheelDelta > 0))
Expand All @@ -124,7 +124,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
ScreenInfo.WheelDelta = wheelDelta;
}

if ((ULONG)abs(ScreenInfo.WheelDelta) >= ulActualDelta)
if (abs(ScreenInfo.WheelDelta) >= ulActualDelta)
{
/*
* By default, SHIFT + WM_MOUSEWHEEL will scroll 1/2 the
Expand All @@ -134,7 +134,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
til::CoordType delta = 1;
if (hasShift)
{
delta = std::max((ScreenInfo.GetViewport().Height() * ScreenInfo.ScrollScale) / 2, 1u);
delta = std::max((ScreenInfo.GetViewport().Height() * ::base::saturated_cast<til::CoordType>(ScreenInfo.ScrollScale)) / 2, 1);

// Account for scroll direction changes by adjusting delta if there was a direction change.
delta *= (ScreenInfo.WheelDelta < 0 ? -1 : 1);
Expand All @@ -161,7 +161,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
}
else if (isMouseHWheel && s_ucWheelScrollChars > 0)
{
const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollChars, 1ul);
const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollChars, 1);

if ((ScreenInfo.HWheelDelta > 0) == (wheelDelta > 0))
{
Expand All @@ -172,7 +172,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
ScreenInfo.HWheelDelta = wheelDelta;
}

if ((ULONG)abs(ScreenInfo.HWheelDelta) >= ulActualDelta)
if (abs(ScreenInfo.HWheelDelta) >= ulActualDelta)
{
til::CoordType delta = 1;

Expand Down
4 changes: 2 additions & 2 deletions src/host/scrolling.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ class Scrolling
private:
static BOOL s_IsPointInRectangle(const til::rect* prc, const til::point pt);

static ULONG s_ucWheelScrollLines;
static ULONG s_ucWheelScrollChars;
static til::CoordType s_ucWheelScrollLines;
static til::CoordType s_ucWheelScrollChars;
};

0 comments on commit 730eb5f

Please sign in to comment.