From 6e7bafe5f2ac2874b80b8e31754dc534f64b301c Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 8 Jul 2020 16:40:27 -0700 Subject: [PATCH 1/3] Account for WHEEL_DELTA when dispatching VT mouse wheel events By storing up the accumulated delta in the mouse input handler, we can enlighten both conhost and terminal about wheel events that are less than one line in size. Previously, we had a workaround in conhost that clamped small scroll deltas to a whole line, which made trackpad scrolling unimaginably fast. Terminal didn't make this mistake, but it also didn't handle delta accumulation . . . which resulted in the same behavior. MouseInput will now wait until it's received WHEEL_DELTA (well-known constant, value 120) worth of scrolling delta before it dispatches a single scroll event. Future considerations may include sending multiple wheel button events for every *multiple* of WHEEL_DELTA, but that would be a slightly larger refactoring that I'm not yet ready to undertake. There's a chance that we should be dividing WHEEL_DELTA by the system's "number of lines to scroll at once" setting, because on trackpads conhost now scrolls a little _slow_. I think the only way to determine whether this is palatable is to just ship it. Fixes #6184. --- src/interactivity/win32/windowio.cpp | 15 +---------- src/terminal/input/mouseInput.cpp | 37 ++++++++++++++++++++++++++++ src/terminal/input/terminalInput.hpp | 1 + src/types/inc/utils.hpp | 8 ++++++ 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/src/interactivity/win32/windowio.cpp b/src/interactivity/win32/windowio.cpp index ba3b0385b74..023b14a8598 100644 --- a/src/interactivity/win32/windowio.cpp +++ b/src/interactivity/win32/windowio.cpp @@ -630,20 +630,7 @@ BOOL HandleMouseEvent(const SCREEN_INFORMATION& ScreenInfo, short sDelta = 0; if (Message == WM_MOUSEWHEEL) { - short sWheelDelta = GET_WHEEL_DELTA_WPARAM(wParam); - // For most devices, we'll get mouse events as multiples of - // WHEEL_DELTA, where WHEEL_DELTA represents a single scroll unit - // But sometimes, things like trackpads will scroll in finer - // measurements. In this case, the VT mouse scrolling wouldn't work. - // So if that happens, ensure we scroll at least one time. - if (abs(sWheelDelta) < WHEEL_DELTA) - { - sDelta = sWheelDelta < 0 ? -1 : 1; - } - else - { - sDelta = sWheelDelta / WHEEL_DELTA; - } + sDelta = GET_WHEEL_DELTA_WPARAM(wParam); } if (HandleTerminalMouseEvent(MousePosition, Message, GET_KEYSTATE_WPARAM(wParam), sDelta)) diff --git a/src/terminal/input/mouseInput.cpp b/src/terminal/input/mouseInput.cpp index c215c810146..eccaaff79be 100644 --- a/src/terminal/input/mouseInput.cpp +++ b/src/terminal/input/mouseInput.cpp @@ -4,6 +4,7 @@ #include "precomp.h" #include #include "terminalInput.hpp" +#include "../types/inc/utils.hpp" using namespace Microsoft::Console::VirtualTerminal; @@ -63,6 +64,17 @@ static constexpr bool _isHoverMsg(const unsigned int buttonCode) noexcept return buttonCode == WM_MOUSEMOVE; } +// Routine Description: +// - Determines if the input windows message code describes a mouse wheel event +// Parameters: +// - buttonCode - the message to decode. +// Return value: +// - true iff buttonCode is a wheel event to translate +static constexpr bool _isWheelMsg(const unsigned int buttonCode) noexcept +{ + return buttonCode == WM_MOUSEWHEEL || buttonCode == WM_MOUSEHWHEEL; +} + // Routine Description: // - Determines if the input windows message code describes a button press // (either down or doubleclick) @@ -293,6 +305,31 @@ bool TerminalInput::HandleMouse(const COORD position, const short modifierKeyState, const short delta) { + if (Utils::Sign(delta) != Utils::Sign(_mouseInputState.accumulatedDelta)) + { + // This works for wheel and non-wheel events and transitioning between wheel/non-wheel. + // Non-wheel events have a delta of 0, which will fail to match the sign on + // a real wheel event or the accumulated delta. Wheel events will be either + or - + // and we only want to accumulate them if they match in sign. + _mouseInputState.accumulatedDelta = 0; + } + + if (_isWheelMsg(button)) + { + _mouseInputState.accumulatedDelta += delta; + if (std::abs(_mouseInputState.accumulatedDelta) < WHEEL_DELTA) + { + // If we're accumulating button presses of the same type, *and* those presses are + // on the wheel, accumulate delta until we hit the amount required to dispatch one + // "line" worth of scroll. + return true; + } + + // We're ready to send this event through, but first we need to clear the accumulated; + // delta. Otherwise, we'll dispatch every subsequent sub-delta event as its own event. + _mouseInputState.accumulatedDelta = 0; + } + bool success = false; if (_ShouldSendAlternateScroll(button, delta)) { diff --git a/src/terminal/input/terminalInput.hpp b/src/terminal/input/terminalInput.hpp index 7b2d9abf0a5..167e0f68370 100644 --- a/src/terminal/input/terminalInput.hpp +++ b/src/terminal/input/terminalInput.hpp @@ -109,6 +109,7 @@ namespace Microsoft::Console::VirtualTerminal bool inAlternateBuffer{ false }; COORD lastPos{ -1, -1 }; unsigned int lastButton{ 0 }; + int accumulatedDelta{ 0 }; }; MouseInputState _mouseInputState; diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index eb37fe73a74..5117117bb23 100644 --- a/src/types/inc/utils.hpp +++ b/src/types/inc/utils.hpp @@ -15,6 +15,14 @@ Author(s): namespace Microsoft::Console::Utils { + // Function Description: + // - Returns -1, 0 or +1 to indicate the sign of the passed-in value. + template + int Sign(T val) + { + return (T{ 0 } < val) - (val < T{ 0 }); + } + bool IsValidHandle(const HANDLE handle) noexcept; // Function Description: From 01307258224cca7decdb82c9ded889e4ae96f263 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 8 Jul 2020 17:26:53 -0700 Subject: [PATCH 2/3] don't let audit mode bring you down --- src/types/inc/utils.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index 5117117bb23..5e22a25127e 100644 --- a/src/types/inc/utils.hpp +++ b/src/types/inc/utils.hpp @@ -18,7 +18,7 @@ namespace Microsoft::Console::Utils // Function Description: // - Returns -1, 0 or +1 to indicate the sign of the passed-in value. template - int Sign(T val) + constexpr int Sign(T val) noexcept { return (T{ 0 } < val) - (val < T{ 0 }); } From 9daaa5f37e64f4c8a064e0b0136e26f912bdd7e4 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Thu, 9 Jul 2020 16:02:13 -0700 Subject: [PATCH 3/3] Fix the tests --- src/terminal/adapter/ut_adapter/MouseInputTest.cpp | 14 +++++++------- src/terminal/input/mouseInput.cpp | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/terminal/adapter/ut_adapter/MouseInputTest.cpp b/src/terminal/adapter/ut_adapter/MouseInputTest.cpp index e740467cc5a..c064c330dd0 100644 --- a/src/terminal/adapter/ut_adapter/MouseInputTest.cpp +++ b/src/terminal/adapter/ut_adapter/MouseInputTest.cpp @@ -514,7 +514,7 @@ class MouseInputTest TEST_METHOD(ScrollWheelTests) { BEGIN_TEST_METHOD_PROPERTIES() - TEST_METHOD_PROPERTY(L"Data:sScrollDelta", L"{0, -1, 1, 100, -10000, 32736}") + TEST_METHOD_PROPERTY(L"Data:sScrollDelta", L"{-120, 120, -10000, 32736}") TEST_METHOD_PROPERTY(L"Data:uiModifierKeystate", L"{0x0000, 0x0004, 0x0008}") END_TEST_METHOD_PROPERTIES() @@ -606,31 +606,31 @@ class MouseInputTest Log::Comment(L"Test mouse wheel scrolling up"); s_pwszInputExpected = L"\x1B[A"; - VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, 1)); + VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, WHEEL_DELTA)); Log::Comment(L"Test mouse wheel scrolling down"); s_pwszInputExpected = L"\x1B[B"; - VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, -1)); + VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, -WHEEL_DELTA)); Log::Comment(L"Enable cursor keys mode"); mouseInput->ChangeCursorKeysMode(true); Log::Comment(L"Test mouse wheel scrolling up"); s_pwszInputExpected = L"\x1BOA"; - VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, 1)); + VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, WHEEL_DELTA)); Log::Comment(L"Test mouse wheel scrolling down"); s_pwszInputExpected = L"\x1BOB"; - VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, -1)); + VERIFY_IS_TRUE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, -WHEEL_DELTA)); Log::Comment(L"Confirm no effect when scroll mode is disabled"); mouseInput->UseAlternateScreenBuffer(); mouseInput->EnableAlternateScroll(false); - VERIFY_IS_FALSE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, 1)); + VERIFY_IS_FALSE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, WHEEL_DELTA)); Log::Comment(L"Confirm no effect when using the main buffer"); mouseInput->UseMainScreenBuffer(); mouseInput->EnableAlternateScroll(true); - VERIFY_IS_FALSE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, 1)); + VERIFY_IS_FALSE(mouseInput->HandleMouse({ 0, 0 }, WM_MOUSEWHEEL, noModifierKeys, WHEEL_DELTA)); } }; diff --git a/src/terminal/input/mouseInput.cpp b/src/terminal/input/mouseInput.cpp index eccaaff79be..70694d26dff 100644 --- a/src/terminal/input/mouseInput.cpp +++ b/src/terminal/input/mouseInput.cpp @@ -322,7 +322,8 @@ bool TerminalInput::HandleMouse(const COORD position, // If we're accumulating button presses of the same type, *and* those presses are // on the wheel, accumulate delta until we hit the amount required to dispatch one // "line" worth of scroll. - return true; + // Mark the event as "handled" if we would have otherwise emitted a scroll event. + return IsTrackingMouseInput() || _ShouldSendAlternateScroll(button, delta); } // We're ready to send this event through, but first we need to clear the accumulated;