From 5d05e95fe0d5b4ed0589a751e27ea39a682766b0 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 10:32:24 -0500 Subject: [PATCH 01/10] This is a test for #9955.c --- .../TerminalControl/ControlInteractivity.cpp | 3 -- .../TerminalControl/ControlInteractivity.h | 1 - .../ControlInteractivityTests.cpp | 44 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index ecee7cc0e56..f41516a1af9 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -197,7 +197,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (multiClickMapper == 1) { _singleClickTouchdownPos = pixelPosition; - _singleClickTouchdownTerminalPos = terminalPosition; if (!_core->HasSelection()) { @@ -269,7 +268,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation _core->SetSelectionAnchor(terminalPosition); // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; - _singleClickTouchdownTerminalPos = std::nullopt; } } @@ -341,7 +339,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation } _singleClickTouchdownPos = std::nullopt; - _singleClickTouchdownTerminalPos = std::nullopt; } void ControlInteractivity::TouchReleased() diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index cd099dff3a4..e4fae12ef86 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -97,7 +97,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation Timestamp _lastMouseClickTimestamp; std::optional _lastMouseClickPos; std::optional _singleClickTouchdownPos; - std::optional _singleClickTouchdownTerminalPos; std::optional _lastMouseClickPosNoSelection; // This field tracks whether the selection has changed meaningfully // since it was last copied. It's generally used to prevent copyOnSelect diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index eeb9825475f..c6fb3e9e079 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -29,6 +29,7 @@ namespace ControlUnitTests TEST_METHOD(TestScrollWithMouse); TEST_METHOD(CreateSubsequentSelectionWithDragging); + TEST_METHOD(TestQuickDragOnSelect); TEST_CLASS_SETUP(ClassSetup) { @@ -330,4 +331,47 @@ namespace ControlUnitTests VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); } + + void ControlInteractivityTests::TestQuickDragOnSelect() + { + // This is a test for GH#9955.c + + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const TerminalInput::MouseButtonState leftMouseDown{ true, false, false }; + const TerminalInput::MouseButtonState noMouseDown{ false, false, false }; + + const til::size fontSize{ 9, 21 }; + + Log::Comment(L"Click on the terminal"); + const til::point terminalPosition0{ 6, 0 }; + const til::point cursorPosition0 = terminalPosition0 * fontSize; + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition0); + + Log::Comment(L"Verify that there's not yet a selection"); + VERIFY_IS_FALSE(core->HasSelection()); + + Log::Comment(L"Drag the mouse a lot. This simulates draging the mouse real fast."); + const til::point cursorPosition1{ 6 + fontSize.width() * 2, 0 }; + interactivity->PointerMoved(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + modifiers, + true, // focused, + cursorPosition1); + Log::Comment(L"Verify that there's one selection"); + VERIFY_IS_TRUE(core->HasSelection()); + VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); + + Log::Comment(L"Verify that it started on the first cell we clicked on, not the one we dragged to"); + COORD expectedAnchor{ 0, 0 }; + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); + } } From 167ff7c13f8aa3c3019dabab9762dea905f27c95 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 10:59:25 -0500 Subject: [PATCH 02/10] Nevermind, this is the real test --- .../UnitTests_Control/ControlInteractivityTests.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index c6fb3e9e079..59df2fa5eb0 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -348,8 +348,7 @@ namespace ControlUnitTests const til::size fontSize{ 9, 21 }; Log::Comment(L"Click on the terminal"); - const til::point terminalPosition0{ 6, 0 }; - const til::point cursorPosition0 = terminalPosition0 * fontSize; + const til::point cursorPosition0{ 6, 0 }; interactivity->PointerPressed(leftMouseDown, WM_LBUTTONDOWN, //pointerUpdateKind 0, // timestamp @@ -359,6 +358,9 @@ namespace ControlUnitTests Log::Comment(L"Verify that there's not yet a selection"); VERIFY_IS_FALSE(core->HasSelection()); + VERIFY_IS_TRUE(interactivity->_singleClickTouchdownPos.has_value()); + VERIFY_ARE_EQUAL(cursorPosition0, interactivity->_singleClickTouchdownPos.value()); + Log::Comment(L"Drag the mouse a lot. This simulates draging the mouse real fast."); const til::point cursorPosition1{ 6 + fontSize.width() * 2, 0 }; interactivity->PointerMoved(leftMouseDown, From 9d33e89b58b9f5d8d25bc335c52443b0d57f972e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 10:59:43 -0500 Subject: [PATCH 03/10] Fixes #9955.C --- src/cascadia/TerminalControl/ControlInteractivity.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index f41516a1af9..552512815e8 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -265,7 +265,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto fontSizeInDips{ _core->FontSizeInDips() }; if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f)) { - _core->SetSelectionAnchor(terminalPosition); + // GH#9955.c: Make sure to use the temrinal location of the + // _touchdown_ point here. We want to start the selection + // from where the user initially clicked, not where they are + // now. + _core->SetSelectionAnchor(_getTerminalPosition(touchdownPoint)); + // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; } From be7ea6f0bffb2c377c834f44b7753ab8a555d6b7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 11:30:21 -0500 Subject: [PATCH 04/10] Add a test for #9955.a --- .../ControlInteractivityTests.cpp | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 59df2fa5eb0..65c78d8edb8 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -29,6 +29,7 @@ namespace ControlUnitTests TEST_METHOD(TestScrollWithMouse); TEST_METHOD(CreateSubsequentSelectionWithDragging); + TEST_METHOD(ScrollWithSelection); TEST_METHOD(TestQuickDragOnSelect); TEST_CLASS_SETUP(ClassSetup) @@ -332,6 +333,83 @@ namespace ControlUnitTests VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); } + void ControlInteractivityTests::ScrollWithSelection() + { + // This is a test for GH#9955.a + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + // For the sake of this test, scroll one line at a time + interactivity->_rowsToScroll = 1; + + Log::Comment(L"Add some test to the terminal so we can scroll"); + for (int i = 0; i < 40; ++i) + { + conn->WriteInput(L"Foo\r\n"); + } + // We printed that 40 times, but the final \r\n bumped the view down one MORE row. + VERIFY_ARE_EQUAL(20, core->_terminal->GetViewport().Height()); + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + VERIFY_ARE_EQUAL(20, core->ViewHeight()); + VERIFY_ARE_EQUAL(41, core->BufferHeight()); + + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const TerminalInput::MouseButtonState leftMouseDown{ true, false, false }; + const TerminalInput::MouseButtonState noMouseDown{ false, false, false }; + + const til::size fontSize{ 9, 21 }; + + Log::Comment(L"Click on the terminal"); + const til::point terminalPosition0{ 5, 5 }; + const til::point cursorPosition0{ terminalPosition0 * fontSize }; + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition0); + + Log::Comment(L"Verify that there's not yet a selection"); + VERIFY_IS_FALSE(core->HasSelection()); + + VERIFY_IS_TRUE(interactivity->_singleClickTouchdownPos.has_value()); + VERIFY_ARE_EQUAL(cursorPosition0, interactivity->_singleClickTouchdownPos.value()); + + Log::Comment(L"Drag the mouse just a little"); + // move not quite a whole cell, but enough to start a selection + const til::point cursorPosition1{ cursorPosition0 + til::point{ 6, 0 } }; + interactivity->PointerMoved(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + modifiers, + true, // focused, + cursorPosition1); + Log::Comment(L"Verify that there's one selection"); + VERIFY_IS_TRUE(core->HasSelection()); + VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); + + Log::Comment(L"Verify the location of the selection"); + // The viewport is on row 21, so the selection will be on: + // {(5, 5)+(0, 21)} to {(5, 5)+(0, 21)} + COORD expectedAnchor{ 5, 26 }; + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); + + Log::Comment(L"Scroll up a line, with the left mouse button selected"); + interactivity->MouseWheel(modifiers, + WHEEL_DELTA, + cursorPosition1, + { true, false, false }); + + Log::Comment(L"Verify the location of the selection"); + // The viewport is now on row 20, so the selection will be on: + // {(5, 5)+(0, 20)} to {(5, 5)+(0, 21)} + COORD newExpectedAnchor{ 5, 25 }; + // Remember, the anchor is always before the end in the buffer. So yes, + // se started the selection on 5,26, but now that's the end. + VERIFY_ARE_EQUAL(newExpectedAnchor, core->_terminal->GetSelectionAnchor()); + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); + } + void ControlInteractivityTests::TestQuickDragOnSelect() { // This is a test for GH#9955.c From 4f4df01391ef1433846b0867c74915a89cb3abe5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 11:31:08 -0500 Subject: [PATCH 05/10] Fixes #9955.a ironic that all these bugs are all in the pixelPosition code that I yeeted in late on the last Wednesday I was working on the original PR --- src/cascadia/TerminalControl/ControlInteractivity.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 552512815e8..7db1d486992 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -398,7 +398,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } else { - _mouseScrollHandler(delta, terminalPosition, state.isLeftButtonDown); + _mouseScrollHandler(delta, pixelPosition, state.isLeftButtonDown); } return false; } @@ -430,10 +430,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - Scroll the visible viewport in response to a mouse wheel event. // Arguments: // - mouseDelta: the mouse wheel delta that triggered this event. - // - point: the location of the mouse during this event + // - pixelPosition: the location of the mouse during this event // - isLeftButtonPressed: true iff the left mouse button was pressed during this event. void ControlInteractivity::_mouseScrollHandler(const double mouseDelta, - const til::point terminalPosition, + const til::point pixelPosition, const bool isLeftButtonPressed) { const auto currentOffset = _core->ScrollOffset(); @@ -458,7 +458,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // If user is mouse selecting and scrolls, they then point at new // character. Make sure selection reflects that immediately. - SetEndSelectionPoint(terminalPosition); + SetEndSelectionPoint(pixelPosition); } } From 33d29fa30ace884653d797a8f92d69758ffcaae7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 14:11:55 -0500 Subject: [PATCH 06/10] This fixes #9955.b --- .../TerminalControl/ControlInteractivity.cpp | 60 +++++++++++++++---- .../TerminalControl/ControlInteractivity.h | 5 +- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 7db1d486992..323bd9aaac5 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -35,6 +35,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation _selectionNeedsToBeCopied{ false } { _core = winrt::make_self(settings, connection); + + _core->ScrollPositionChanged({ this, &ControlInteractivity::_coreScrollPositionChanged }); } void ControlInteractivity::UpdateSettings() @@ -306,8 +308,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation // panning down) const float numRows = -1.0f * (dy / fontSizeInDips.height()); - const auto currentOffset = ::base::ClampedNumeric(_core->ScrollOffset()); - const auto newValue = numRows + currentOffset; + const double currentOffset = ::base::ClampedNumeric(_core->ScrollOffset()); + const double newValue = numRows + currentOffset; // Update the Core's viewport position, and raise a // ScrollPositionChanged event to update the scrollbar @@ -436,23 +438,28 @@ namespace winrt::Microsoft::Terminal::Control::implementation const til::point pixelPosition, const bool isLeftButtonPressed) { - const auto currentOffset = _core->ScrollOffset(); + // GH#9955.b: Start scrolling from our internal scrollbar position. This + // lets us accumulate fractional numbers of rows to scroll with each + // event. Especially for precision trackpads, we might be getting scroll + // deltas smaller than a single row, but we still want lots of those to + // accumulate. + const double currentOffset = _internalScrollbarPosition; // negative = down, positive = up // However, for us, the signs are flipped. // With one of the precision mice, one click is always a multiple of 120 (WHEEL_DELTA), // but the "smooth scrolling" mode results in non-int values - const auto rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA); + const double rowDelta = mouseDelta / (-1.0 * 120.0); // WHEEL_PAGESCROLL is a Win32 constant that represents the "scroll one page // at a time" setting. If we ignore it, we will scroll a truly absurd number // of rows. - const auto rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? _core->ViewHeight() : _rowsToScroll }; + const double rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? ::base::saturated_cast(_core->ViewHeight()) : _rowsToScroll }; double newValue = (rowsToScroll * rowDelta) + (currentOffset); // Update the Core's viewport position, and raise a // ScrollPositionChanged event to update the scrollbar - _updateScrollbar(::base::saturated_cast(newValue)); + _updateScrollbar(newValue); if (isLeftButtonPressed) { @@ -478,15 +485,42 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - newValue: The new top of the viewport // Return Value: // - - void ControlInteractivity::_updateScrollbar(const int newValue) + void ControlInteractivity::_updateScrollbar(const double newValue) { - _core->UserScrollViewport(newValue); + // Set this as the new value of our internal scrollbar representation. + // We're doing this so we can accumulate fractional amounts of a row to + // scroll each time the mouse scrolls. + _internalScrollbarPosition = newValue; + + // If the new scrollbar position, rounded to an int, is at a different + // row, then actually update the scroll position in the core, and raise + // a ScrollPositionChanged to inform the control. + int viewTop = ::base::saturated_cast(::std::round(_internalScrollbarPosition)); + if (viewTop != _core->ScrollOffset()) + { + _core->UserScrollViewport(viewTop); + + // _core->ScrollOffset() is now set to newValue + _ScrollPositionChangedHandlers(*this, + winrt::make(_core->ScrollOffset(), + _core->ViewHeight(), + _core->BufferHeight())); + } + } - // _core->ScrollOffset() is now set to newValue - _ScrollPositionChangedHandlers(*this, - winrt::make(_core->ScrollOffset(), - _core->ViewHeight(), - _core->BufferHeight())); + // Method Description: + // - Event handler for the core's ScrollPositionChanged event. This is + // called when the core changes its viewport position, due to more text + // being output. We'll use this event to update our own internal scrollbar + // tracker to the position the viewport is at now. + // Arguments: + // - args: args containing infor about the position of the viewport in the buffer. + // Return Value: + // - + void ControlInteractivity::_coreScrollPositionChanged(const IInspectable& /*sender*/, + const Control::ScrollPositionChangedArgs& args) + { + _internalScrollbarPosition = args.ViewTop(); } void ControlInteractivity::_hyperlinkHandler(const std::wstring_view uri) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index e4fae12ef86..87114add6ce 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -83,6 +83,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation private: winrt::com_ptr _core{ nullptr }; unsigned int _rowsToScroll; + double _internalScrollbarPosition{ 0.0 }; // If this is set, then we assume we are in the middle of panning the // viewport via touch input. @@ -123,9 +124,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _canSendVTMouseInput(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers); void _sendPastedTextToConnection(std::wstring_view wstr); - void _updateScrollbar(const int newValue); + void _updateScrollbar(const double newValue); til::point _getTerminalPosition(const til::point& pixelPosition); + void _coreScrollPositionChanged(const IInspectable& /*sender*/, const Control::ScrollPositionChangedArgs& args); + TYPED_EVENT(OpenHyperlink, IInspectable, Control::OpenHyperlinkEventArgs); TYPED_EVENT(PasteFromClipboard, IInspectable, Control::PasteFromClipboardEventArgs); TYPED_EVENT(ScrollPositionChanged, IInspectable, Control::ScrollPositionChangedArgs); From 0fefc5b9eacf44ebfb7cbecd69f665e927c45d15 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 28 Apr 2021 15:02:52 -0500 Subject: [PATCH 07/10] add tests for #9955.b as well --- .../TerminalControl/ControlInteractivity.cpp | 8 +- .../TerminalControl/ControlInteractivity.h | 4 +- src/cascadia/TerminalControl/TermControl.cpp | 4 +- .../ControlInteractivityTests.cpp | 110 ++++++++++++++++++ 4 files changed, 119 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 323bd9aaac5..8ced4e65146 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -313,7 +313,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Update the Core's viewport position, and raise a // ScrollPositionChanged event to update the scrollbar - _updateScrollbar(newValue); + UpdateScrollbar(newValue); // Use this point as our new scroll anchor. _touchAnchor = newTouchPoint; @@ -459,7 +459,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Update the Core's viewport position, and raise a // ScrollPositionChanged event to update the scrollbar - _updateScrollbar(newValue); + UpdateScrollbar(newValue); if (isLeftButtonPressed) { @@ -485,12 +485,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - newValue: The new top of the viewport // Return Value: // - - void ControlInteractivity::_updateScrollbar(const double newValue) + void ControlInteractivity::UpdateScrollbar(const double newValue) { // Set this as the new value of our internal scrollbar representation. // We're doing this so we can accumulate fractional amounts of a row to // scroll each time the mouse scrolls. - _internalScrollbarPosition = newValue; + _internalScrollbarPosition = std::clamp(newValue, 0.0, _core->BufferHeight()); // If the new scrollbar position, rounded to an int, is at a different // row, then actually update the scroll position in the core, and raise diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index 87114add6ce..f1b1b22aab1 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -73,6 +73,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation const int32_t delta, const til::point pixelPosition, const ::Microsoft::Console::VirtualTerminal::TerminalInput::MouseButtonState state); + + void UpdateScrollbar(const double newValue); + #pragma endregion bool CopySelectionToClipboard(bool singleLine, @@ -124,7 +127,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _canSendVTMouseInput(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers); void _sendPastedTextToConnection(std::wstring_view wstr); - void _updateScrollbar(const double newValue); til::point _getTerminalPosition(const til::point& pixelPosition); void _coreScrollPositionChanged(const IInspectable& /*sender*/, const Control::ScrollPositionChangedArgs& args); diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 5346d0507c9..3c9108da3b9 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1302,8 +1302,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation return; } - const auto newValue = static_cast(args.NewValue()); - _core->UserScrollViewport(newValue); + const auto newValue = args.NewValue(); + _interactivity->UpdateScrollbar(newValue); // User input takes priority over terminal events so cancel // any pending scroll bar update if the user scrolls. diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 65c78d8edb8..84c2f905ae0 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -30,6 +30,7 @@ namespace ControlUnitTests TEST_METHOD(CreateSubsequentSelectionWithDragging); TEST_METHOD(ScrollWithSelection); + TEST_METHOD(TestScrollWithTrackpad); TEST_METHOD(TestQuickDragOnSelect); TEST_CLASS_SETUP(ClassSetup) @@ -228,11 +229,13 @@ namespace ControlUnitTests Log::Comment(L"Scroll down 21 more times, to the bottom"); for (int i = 0; i < 21; ++i) { + Log::Comment(NoThrowString().Format(L"---scroll down #%d---", i)); expectedTop++; interactivity->MouseWheel(modifiers, -WHEEL_DELTA, til::point{ 0, 0 }, { false, false, false }); + Log::Comment(NoThrowString().Format(L"internal scrollbar pos:%f", interactivity->_internalScrollbarPosition)); } Log::Comment(L"Scrolling up more should do nothing"); expectedTop = 21; @@ -410,6 +413,113 @@ namespace ControlUnitTests VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); } + void ControlInteractivityTests::TestScrollWithTrackpad() + { + WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{}; + + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + // For the sake of this test, scroll one line at a time + interactivity->_rowsToScroll = 1; + + // int expectedTop = 0; + // int expectedViewHeight = 20; + // int expectedBufferHeight = 20; + + // auto scrollChangedHandler = [&](auto&&, const Control::ScrollPositionChangedArgs& args) mutable { + // VERIFY_ARE_EQUAL(expectedTop, args.ViewTop()); + // VERIFY_ARE_EQUAL(expectedViewHeight, args.ViewHeight()); + // VERIFY_ARE_EQUAL(expectedBufferHeight, args.BufferSize()); + // }; + // core->ScrollPositionChanged(scrollChangedHandler); + // interactivity->ScrollPositionChanged(scrollChangedHandler); + + for (int i = 0; i < 40; ++i) + { + // Log::Comment(NoThrowString().Format(L"Writing line #%d", i)); + // The \r\n in the 19th loop will cause the view to start moving + // if (i >= 19) + // { + // expectedTop++; + // expectedBufferHeight++; + // } + + conn->WriteInput(L"Foo\r\n"); + } + // We printed that 40 times, but the final \r\n bumped the view down one MORE row. + VERIFY_ARE_EQUAL(20, core->_terminal->GetViewport().Height()); + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + VERIFY_ARE_EQUAL(20, core->ViewHeight()); + VERIFY_ARE_EQUAL(41, core->BufferHeight()); + + Log::Comment(L"Scroll up a line"); + const auto modifiers = ControlKeyStates(); + // expectedBufferHeight = 41; + // expectedTop = 20; + + // Deltas that I saw while scrolling with the surface laptop trackpad + // were on the range [-22, 7], though I'm sure they could be greater in + // magnitude. + // + // WHEEL_DELTA is 120, so we'll use 24 for now as the delta, just so the tests don't take forever. + + const int delta = WHEEL_DELTA / 5; + const til::point mousePos{ 0, 0 }; + TerminalInput::MouseButtonState state{ false, false, false }; + + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + + Log::Comment(L"Scroll up 4 more times. Once we're at 3/5 scrolls, " + L"we'll round the internal scrollbar position to scrolling to the next row."); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 3/5 + VERIFY_ARE_EQUAL(20, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 4/5 + VERIFY_ARE_EQUAL(20, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 5/5 + VERIFY_ARE_EQUAL(20, core->ScrollOffset()); + + Log::Comment(L"Jump to line 5, so we can scroll down from there."); + interactivity->UpdateScrollbar(5); + VERIFY_ARE_EQUAL(5, core->ScrollOffset()); + Log::Comment(L"Scroll down 5 times, at which point we should accumulate a whole row of delta."); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(5, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(5, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 3/5 + VERIFY_ARE_EQUAL(6, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 4/5 + VERIFY_ARE_EQUAL(6, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 5/5 + VERIFY_ARE_EQUAL(6, core->ScrollOffset()); + + Log::Comment(L"Jump to the bottom."); + interactivity->UpdateScrollbar(21); + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + Log::Comment(L"Scroll a bit, then emit a line of text. We should reset our internal scroll position."); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + + conn->WriteInput(L"Foo\r\n"); + VERIFY_ARE_EQUAL(22, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(22, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(22, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 3/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 4/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 5/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + } + void ControlInteractivityTests::TestQuickDragOnSelect() { // This is a test for GH#9955.c From 7a2bc6a68fc8f9fcd5aa42b613b4a6da4aecc9e5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 29 Apr 2021 10:12:33 -0500 Subject: [PATCH 08/10] turns out, we can get away without the event --- .../TerminalControl/ControlInteractivity.cpp | 33 ++++++++----------- .../TerminalControl/ControlInteractivity.h | 2 -- .../ControlInteractivityTests.cpp | 31 +++++++---------- 3 files changed, 26 insertions(+), 40 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 8ced4e65146..15c75885b61 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -36,7 +36,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _core = winrt::make_self(settings, connection); - _core->ScrollPositionChanged({ this, &ControlInteractivity::_coreScrollPositionChanged }); + // _core->ScrollPositionChanged({ this, &ControlInteractivity::_coreScrollPositionChanged }); } void ControlInteractivity::UpdateSettings() @@ -267,7 +267,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto fontSizeInDips{ _core->FontSizeInDips() }; if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f)) { - // GH#9955.c: Make sure to use the temrinal location of the + // GH#9955.c: Make sure to use the terminal location of the // _touchdown_ point here. We want to start the selection // from where the user initially clicked, not where they are // now. @@ -443,13 +443,23 @@ namespace winrt::Microsoft::Terminal::Control::implementation // event. Especially for precision trackpads, we might be getting scroll // deltas smaller than a single row, but we still want lots of those to // accumulate. - const double currentOffset = _internalScrollbarPosition; + // + // At the start, let's compare what we _think_ the scrollbar is, with + // what it should be. It's possible the core scrolled out from + // underneath us. We wouldn't know - we don't want the overhead of + // another ScrollPositionChanged handler. If the scrollbar should be + // somewhere other than where it is currently, then start from that row. + const int currentInternalRow = ::base::saturated_cast(::std::round(_internalScrollbarPosition)); + const int currentCoreRow = _core->ScrollOffset(); + const double currentOffset = currentInternalRow == currentCoreRow ? + _internalScrollbarPosition : + currentCoreRow; // negative = down, positive = up // However, for us, the signs are flipped. // With one of the precision mice, one click is always a multiple of 120 (WHEEL_DELTA), // but the "smooth scrolling" mode results in non-int values - const double rowDelta = mouseDelta / (-1.0 * 120.0); + const double rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA); // WHEEL_PAGESCROLL is a Win32 constant that represents the "scroll one page // at a time" setting. If we ignore it, we will scroll a truly absurd number @@ -508,21 +518,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation } } - // Method Description: - // - Event handler for the core's ScrollPositionChanged event. This is - // called when the core changes its viewport position, due to more text - // being output. We'll use this event to update our own internal scrollbar - // tracker to the position the viewport is at now. - // Arguments: - // - args: args containing infor about the position of the viewport in the buffer. - // Return Value: - // - - void ControlInteractivity::_coreScrollPositionChanged(const IInspectable& /*sender*/, - const Control::ScrollPositionChangedArgs& args) - { - _internalScrollbarPosition = args.ViewTop(); - } - void ControlInteractivity::_hyperlinkHandler(const std::wstring_view uri) { _OpenHyperlinkHandlers(*this, winrt::make(winrt::hstring{ uri })); diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index f1b1b22aab1..f684f2dfff3 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -129,8 +129,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _sendPastedTextToConnection(std::wstring_view wstr); til::point _getTerminalPosition(const til::point& pixelPosition); - void _coreScrollPositionChanged(const IInspectable& /*sender*/, const Control::ScrollPositionChangedArgs& args); - TYPED_EVENT(OpenHyperlink, IInspectable, Control::OpenHyperlinkEventArgs); TYPED_EVENT(PasteFromClipboard, IInspectable, Control::PasteFromClipboardEventArgs); TYPED_EVENT(ScrollPositionChanged, IInspectable, Control::ScrollPositionChangedArgs); diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 84c2f905ae0..ca63063f6a1 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -423,28 +423,23 @@ namespace ControlUnitTests // For the sake of this test, scroll one line at a time interactivity->_rowsToScroll = 1; - // int expectedTop = 0; - // int expectedViewHeight = 20; - // int expectedBufferHeight = 20; - // auto scrollChangedHandler = [&](auto&&, const Control::ScrollPositionChangedArgs& args) mutable { - // VERIFY_ARE_EQUAL(expectedTop, args.ViewTop()); - // VERIFY_ARE_EQUAL(expectedViewHeight, args.ViewHeight()); - // VERIFY_ARE_EQUAL(expectedBufferHeight, args.BufferSize()); + // // This mock emulates how the TermControl updates the + // // interactivity's internal scrollbar when the core changes its + // // viewport. + // // + // // In reality, the TermControl throttles scrollbar updates, and only + // // calls back to UpdateScrollbar once every 60 seconds. + // const auto newValue = args.ViewTop(); + // if (newValue != core->ScrollOffset()) + // { + // interactivity->UpdateScrollbar(newValue); + // } // }; // core->ScrollPositionChanged(scrollChangedHandler); - // interactivity->ScrollPositionChanged(scrollChangedHandler); for (int i = 0; i < 40; ++i) { - // Log::Comment(NoThrowString().Format(L"Writing line #%d", i)); - // The \r\n in the 19th loop will cause the view to start moving - // if (i >= 19) - // { - // expectedTop++; - // expectedBufferHeight++; - // } - conn->WriteInput(L"Foo\r\n"); } // We printed that 40 times, but the final \r\n bumped the view down one MORE row. @@ -455,8 +450,6 @@ namespace ControlUnitTests Log::Comment(L"Scroll up a line"); const auto modifiers = ControlKeyStates(); - // expectedBufferHeight = 41; - // expectedTop = 20; // Deltas that I saw while scrolling with the surface laptop trackpad // were on the range [-22, 7], though I'm sure they could be greater in @@ -549,7 +542,7 @@ namespace ControlUnitTests VERIFY_IS_TRUE(interactivity->_singleClickTouchdownPos.has_value()); VERIFY_ARE_EQUAL(cursorPosition0, interactivity->_singleClickTouchdownPos.value()); - Log::Comment(L"Drag the mouse a lot. This simulates draging the mouse real fast."); + Log::Comment(L"Drag the mouse a lot. This simulates dragging the mouse real fast."); const til::point cursorPosition1{ 6 + fontSize.width() * 2, 0 }; interactivity->PointerMoved(leftMouseDown, WM_LBUTTONDOWN, //pointerUpdateKind From 9fb46b15c694e38266eed0af36eadb2e40195b7a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 29 Apr 2021 10:33:18 -0500 Subject: [PATCH 09/10] whoops, dead code --- src/cascadia/TerminalControl/ControlInteractivity.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 15c75885b61..433150cabb5 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -35,8 +35,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation _selectionNeedsToBeCopied{ false } { _core = winrt::make_self(settings, connection); - - // _core->ScrollPositionChanged({ this, &ControlInteractivity::_coreScrollPositionChanged }); } void ControlInteractivity::UpdateSettings() From 20018bd0af1b35dbb8b3510de472b348026bd355 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Tue, 4 May 2021 17:28:13 -0500 Subject: [PATCH 10/10] Update src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp --- .../ControlInteractivityTests.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index ca63063f6a1..3839d86e8bc 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -423,21 +423,6 @@ namespace ControlUnitTests // For the sake of this test, scroll one line at a time interactivity->_rowsToScroll = 1; - // auto scrollChangedHandler = [&](auto&&, const Control::ScrollPositionChangedArgs& args) mutable { - // // This mock emulates how the TermControl updates the - // // interactivity's internal scrollbar when the core changes its - // // viewport. - // // - // // In reality, the TermControl throttles scrollbar updates, and only - // // calls back to UpdateScrollbar once every 60 seconds. - // const auto newValue = args.ViewTop(); - // if (newValue != core->ScrollOffset()) - // { - // interactivity->UpdateScrollbar(newValue); - // } - // }; - // core->ScrollPositionChanged(scrollChangedHandler); - for (int i = 0; i < 40; ++i) { conn->WriteInput(L"Foo\r\n");