Skip to content

Commit

Permalink
First three interactivity fixes (#9980)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

This PR encompasses the first three bugs we found post-#9820.

### A: Mousedown, select, SCROLL does a weird thing with endpoints that doesn't happen in stable

We were using the terminal position to set the selection anchor, when we should have used the pixel position.

This is fixed in 4f4df01.

### B: Trackpad scrolling down with small increments seems buggy

This one's the most complicated.  The touchpad sends very many small scroll deltas, less than one row at a time. The control scrollbar can store a `double`, so small deltas can accumulate. Originally, these would accumulate in the scrollbar, and we'd only read that out as an `int` in the scrollbar updater, which is throttled. 

In the interactivity split, there's no place for us to store that double. We immediately narrow to an `int` for `ControlInteractivity::_updateScrollbar`. 

So this introduces a double inside `ControlInteractivity` as a fake scrollbar, with which to accumulate to. 

This is fixed in 33d29fa...0fefc5b

### C:  Looks like there's a selection issue when you click and drag too quickly.

The diff for this one is:

<table>

<tr><td>1.8</td><td>main</td></tr>
<tr>

<td>

```c++
if (_singleClickTouchdownPos)
{
    // Figure out if the user's moved a quarter of a cell's smaller axis away from the clickdown point
    auto& touchdownPoint{ *_singleClickTouchdownPos };
    auto distance{ std::sqrtf(std::powf(cursorPosition.X - touchdownPoint.X, 2) + std::powf(cursorPosition.Y - touchdownPoint.Y, 2)) };
    const til::size fontSize{ _actualFont.GetSize() };

    const auto fontSizeInDips = fontSize.scale(til::math::rounding, 1.0f / _renderEngine->GetScaling());
    if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f))
    {
        _terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint));
        // stop tracking the touchdown point
        _singleClickTouchdownPos = std::nullopt;
    }
}
```

</td>

<td>

```c++
if (_singleClickTouchdownPos)
{
    // Figure out if the user's moved a quarter of a cell's smaller axis away from the clickdown point
    auto& touchdownPoint{ *_singleClickTouchdownPos };
    float dx = ::base::saturated_cast<float>(pixelPosition.x() - touchdownPoint.x());
    float dy = ::base::saturated_cast<float>(pixelPosition.y() - touchdownPoint.y());
    auto distance{ std::sqrtf(std::powf(dx, 2) +
                              std::powf(dy, 2)) };

    const auto fontSizeInDips{ _core->FontSizeInDips() };
    if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f))
    {
        _core->SetSelectionAnchor(terminalPosition);
        // stop tracking the touchdown point
        _singleClickTouchdownPos = std::nullopt;
    }
}
```

</td>
</tr>
</table>

```c++
        _terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint));
```

vs

```c++
        _core->SetSelectionAnchor(terminalPosition);
```

We're now using the location of the drag event as the selection anchor, instead of the location that the user initially clicked. Oops.


## PR Checklist
* [x] Checks three boxes, though I'll be shocked if they're the last.
* [x] I work here
* [x] Tests added/passed 🎉🎉🎉
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

All three have tests, 🙌🙌🙌🙌

## Validation Steps Performed

Manual, and automated via tests
  • Loading branch information
zadjii-msft committed May 4, 2021
1 parent 2b4c20b commit c3ca94c
Show file tree
Hide file tree
Showing 4 changed files with 269 additions and 26 deletions.
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;

// 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
// - 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;

// 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));
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);
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

0 comments on commit c3ca94c

Please sign in to comment.