Skip to content
73 changes: 51 additions & 22 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (multiClickMapper == 1)
{
_singleClickTouchdownPos = pixelPosition;
_singleClickTouchdownTerminalPos = terminalPosition;

if (!_core->HasSelection())
{
Expand Down Expand Up @@ -266,10 +265,14 @@ 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 terminal 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;
_singleClickTouchdownTerminalPos = std::nullopt;
}
}

Expand Down Expand Up @@ -303,12 +306,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// panning down)
const float numRows = -1.0f * (dy / fontSizeInDips.height<float>());

const auto currentOffset = ::base::ClampedNumeric<double>(_core->ScrollOffset());
const auto newValue = numRows + currentOffset;
const double currentOffset = ::base::ClampedNumeric<double>(_core->ScrollOffset());
const double newValue = numRows + currentOffset;
Comment on lines +309 to +310
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you'd want to keep auto currentOffset. That way you get a safe chromium math add for newValue.


// 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;
Expand Down Expand Up @@ -341,7 +344,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

_singleClickTouchdownPos = std::nullopt;
_singleClickTouchdownTerminalPos = std::nullopt;
}

void ControlInteractivity::TouchReleased()
Expand Down Expand Up @@ -396,7 +398,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
else
{
_mouseScrollHandler(delta, terminalPosition, state.isLeftButtonDown);
_mouseScrollHandler(delta, pixelPosition, state.isLeftButtonDown);
}
return false;
}
Expand Down Expand Up @@ -428,35 +430,50 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// - pixelPosition: the location of the mouse during this event
// - pixelPosition: the pixel location of the mouse during this event

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh. i hate when the name is obvious and we are forced to document the obvious name

// - 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();
// 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.
//
// 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<int>(::std::round(_internalScrollbarPosition));
const int currentCoreRow = _core->ScrollOffset();
const double currentOffset = currentInternalRow == currentCoreRow ?
_internalScrollbarPosition :
currentCoreRow;
Comment on lines +450 to +454
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you want auto or base::ClampedNumerics here? Isn't there a chance that currentCoreRow won't fit in a double?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope


// 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 * 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
// of rows.
const auto rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? _core->ViewHeight() : _rowsToScroll };
const double rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? ::base::saturated_cast<double>(_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<int>(newValue));
UpdateScrollbar(newValue);

if (isLeftButtonPressed)
{
// If user is mouse selecting and scrolls, they then point at new
// character. Make sure selection reflects that immediately.
SetEndSelectionPoint(terminalPosition);
SetEndSelectionPoint(pixelPosition);
}
}

Expand All @@ -476,15 +493,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - newValue: The new top of the viewport
// Return Value:
// - <none>
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 = std::clamp<double>(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
// a ScrollPositionChanged to inform the control.
int viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Does rounding match the pre-split behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might actually be better, a little smoother in both directions now. Hard to say for sure comparing Debug vs Release

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));
const auto viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));

if (viewTop != _core->ScrollOffset())
{
_core->UserScrollViewport(viewTop);

// _core->ScrollOffset() is now set to newValue
_ScrollPositionChangedHandlers(*this,
winrt::make<ScrollPositionChangedArgs>(_core->ScrollOffset(),
_core->ViewHeight(),
_core->BufferHeight()));
// _core->ScrollOffset() is now set to newValue
_ScrollPositionChangedHandlers(*this,
winrt::make<ScrollPositionChangedArgs>(_core->ScrollOffset(),
_core->ViewHeight(),
_core->BufferHeight()));
}
}

void ControlInteractivity::_hyperlinkHandler(const std::wstring_view uri)
Expand Down
6 changes: 4 additions & 2 deletions src/cascadia/TerminalControl/ControlInteractivity.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -83,6 +86,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
winrt::com_ptr<ControlCore> _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.
Expand All @@ -97,7 +101,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Timestamp _lastMouseClickTimestamp;
std::optional<til::point> _lastMouseClickPos;
std::optional<til::point> _singleClickTouchdownPos;
std::optional<til::point> _singleClickTouchdownTerminalPos;
std::optional<til::point> _lastMouseClickPosNoSelection;
// This field tracks whether the selection has changed meaningfully
// since it was last copied. It's generally used to prevent copyOnSelect
Expand All @@ -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 int newValue);
til::point _getTerminalPosition(const til::point& pixelPosition);

TYPED_EVENT(OpenHyperlink, IInspectable, Control::OpenHyperlinkEventArgs);
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1302,8 +1302,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return;
}

const auto newValue = static_cast<int>(args.NewValue());
_core->UserScrollViewport(newValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, good!

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.
Expand Down
Loading