Skip to content

Commit

Permalink
Lock when changing selection endpoint on wheel/auto-scroll (#5551)
Browse files Browse the repository at this point in the history
Takes the lock inside two routines in `TermControl` that were changing
the selection endpoint while a rendering frame was still drawing,
resulting in several variants of graphical glitches from double-struck
selection boxes to duplicated line text.

## References
- Introduced with #5185

## PR Checklist
* [x] Closes #5471 
* [x] Already signed life away to company.
* [x] Manual tests passed since it's visual.
* [x] No extra doc besides the comments.
* [x] Am core contributor: Roar.

The renderer base and specific renderer engine do a lot of work to
remember the previous selection and compensate for scrolling regions and
deltas between frames. However, all that work doesn't quite match up
when the endpoints are changed out from under it. Unfortunately,
`TermControl` doesn't have a robust history of locking correctly in step
with the renderer nor does the renderer's `IRenderData` currently
provide any way of 'snapping' state at the beginning of a frame so it
could work without a full lock. So the solution for now is for the
methods that scroll the display in `TermControl` to take the lock that
is shared with the renderer's frame painter so they can't change out of
sync.

## Validation Steps Performed
- Opened terminal with Powershell core.
  Did ls a bunch of times.
  Clicked to make selection and held mouse button while wheeling around.
- Opened terminal with Powershell core.;
  Did ls a bunch of times.
  Clicked to make selection and dragged mouse outside the window to make
  auto scroll happen.
- Opened terminal with Powershell core.
  Did ls a bunch of times.
  Clicked to make selection and released. Wheeled around like a crazy
  person to make sure I didn't regress that.
  • Loading branch information
miniksa committed Apr 24, 2020
1 parent 757db46 commit 98ac196
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

if (_terminal->IsSelectionActive() && isLeftButtonPressed)
{
// Have to take the lock or we could change the endpoints out from under the renderer actively rendering.
auto lock = _terminal->LockForWriting();

// If user is mouse selecting and scrolls, they then point at new character.
// Make sure selection reflects that immediately.
_SetEndSelectionPointAtCursor(point);
Expand Down Expand Up @@ -1517,6 +1520,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

if (_autoScrollingPointerPoint.has_value())
{
// Have to take the lock because the renderer will not draw correctly if you move its endpoints while it is generating a frame.
auto lock = _terminal->LockForWriting();

_SetEndSelectionPointAtCursor(_autoScrollingPointerPoint.value().Position());
}
}
Expand Down

0 comments on commit 98ac196

Please sign in to comment.