Skip to content

Commit

Permalink
Rework and simplify selection in TermControl (#5096)
Browse files Browse the repository at this point in the history 
This commit rewrites selection handling at the TermControl layer.
Previously, we were keeping track of a number of redundant variables
that were easy to get out of sync.

The new selection model is as follows:

* A single left click will always begin a _pending_ selection operation
* A single left click will always clear a selection (#4477)
* A double left click will always begin a word selection
* A triple left click will always begin a line selection
* A selection will only truly start when the cursor moves a quarter of
  the smallest dimension of a cell (usually its width) in any direction
  _This eliminates the selection of a single cell on one click._
  (#4282, #5082)
* We now keep track of whether the selection has been "copied", or
  "updated" since it was last copied. If an endpoint moves, it is
  updated. For copy-on-select, it is only copied if it's updated.
  (#4740)

Because of this, we can stop tracking the position of the focus-raising
click, and whether it was part of click-drag operation. All clicks can
_become_ part of a click-drag operation if the user drags.

We can also eliminate the special handling of single cell selection at
the TerminalCore layer: since TermControl determines when to begin a
selection, TerminalCore no longer needs to know whether copy on select
is enabled _or_ whether the user has started and then backtracked over a
single cell. This is now implicit in TermControl.

Fixes #5082; Fixes #4477
  • Loading branch information
@DHowett
DHowett committed Mar 25, 2020
1 parent 0be070f commit 499f24a
Show file tree
Hide file tree
Showing 8 changed files with 41 additions and 121 deletions.
60 changes: 31 additions & 29 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_cursorTimer{},
_lastMouseClickTimestamp{},
_lastMouseClickPos{},
_searchBox{ nullptr },
_focusRaisedClickPos{ std::nullopt },
_clickDrag{ false }
_selectionNeedsToBeCopied{ false },
_searchBox{ nullptr }
{
_EnsureStaticInitialization();
InitializeComponent();
Expand Down Expand Up @@ -911,9 +910,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
if (!_focused)
{
Focus(FocusState::Pointer);
// Save the click position that brought this control into focus
// in case the user wants to perform a click-drag selection.
_focusRaisedClickPos = point.Position();
}

if (ptr.PointerDeviceType() == Windows::Devices::Input::PointerDeviceType::Mouse || ptr.PointerDeviceType() == Windows::Devices::Input::PointerDeviceType::Pen)
Expand All @@ -933,13 +929,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

if (point.Properties().IsLeftButtonPressed())
{
// A single left click from out of focus should only focus.
if (_focusRaisedClickPos)
{
args.Handled(true);
return;
}

const auto cursorPosition = point.Position();
const auto terminalPosition = _GetTerminalPosition(cursorPosition);

Expand All @@ -956,20 +945,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
if (multiClickMapper == 3)
{
_terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Line);
_selectionNeedsToBeCopied = true;
}
else if (multiClickMapper == 2)
{
_terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Word);
_selectionNeedsToBeCopied = true;
}
else
{
if (shiftEnabled && _terminal->IsSelectionActive())
{
_terminal->SetSelectionEnd(terminalPosition, ::Terminal::SelectionExpansionMode::Cell);
_selectionNeedsToBeCopied = true;
}
else
{
_terminal->SetSelectionAnchor(terminalPosition);
// A single click down resets the selection and begins a new one.
_terminal->ClearSelection();
_singleClickTouchdownPos = cursorPosition;
_selectionNeedsToBeCopied = false; // there's no selection, so there's nothing to update
}

_lastMouseClickTimestamp = point.Timestamp();
Expand All @@ -980,7 +975,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
else if (point.Properties().IsRightButtonPressed())
{
// CopyOnSelect right click always pastes
if (_terminal->IsCopyOnSelectActive() || !_terminal->IsSelectionActive())
if (_settings.CopyOnSelect() || !_terminal->IsSelectionActive())
{
PasteTextFromClipboard();
}
Expand Down Expand Up @@ -1028,16 +1023,22 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

if (point.Properties().IsLeftButtonPressed())
{
_clickDrag = true;
const auto cursorPosition = point.Position();

// PointerPressedHandler doesn't set the SelectionAnchor when the click was
// from out of focus, so PointerMoved has to set it.
if (_focusRaisedClickPos)
if (_singleClickTouchdownPos)
{
_terminal->SetSelectionAnchor(_GetTerminalPosition(*_focusRaisedClickPos));
// 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 auto fontSize{ _actualFont.GetSize() };
if (distance >= (std::min(fontSize.X, fontSize.Y) / 4.f))
{
_terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint));
// stop tracking the touchdown point
_singleClickTouchdownPos = std::nullopt;
}
}

const auto cursorPosition = point.Position();
_SetEndSelectionPointAtCursor(cursorPosition);

const double cursorBelowBottomDist = cursorPosition.Y - SwapChainPanel().Margin().Top - SwapChainPanel().ActualHeight();
Expand Down Expand Up @@ -1121,17 +1122,14 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// Only a left click release when copy on select is active should perform a copy.
// Right clicks and middle clicks should not need to do anything when released.
if (_terminal->IsCopyOnSelectActive() && point.Properties().PointerUpdateKind() == Windows::UI::Input::PointerUpdateKind::LeftButtonReleased)
if (_settings.CopyOnSelect() && point.Properties().PointerUpdateKind() == Windows::UI::Input::PointerUpdateKind::LeftButtonReleased)
{
const auto modifiers = static_cast<uint32_t>(args.KeyModifiers());
// static_cast to a uint32_t because we can't use the WI_IsFlagSet
// macro directly with a VirtualKeyModifiers
const auto shiftEnabled = WI_IsFlagSet(modifiers, static_cast<uint32_t>(VirtualKeyModifiers::Shift));

// In a Copy on Select scenario,
// All left click drags should copy,
// All left clicks on a focused control should copy if a selection is active.
if (_clickDrag || !_focusRaisedClickPos)
if (_selectionNeedsToBeCopied)
{
CopySelectionToClipboard(shiftEnabled);
}
Expand All @@ -1142,8 +1140,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_touchAnchor = std::nullopt;
}

_focusRaisedClickPos = std::nullopt;
_clickDrag = false;
_singleClickTouchdownPos = std::nullopt;

_TryStopAutoScroll(ptr.PointerId());

Expand Down Expand Up @@ -1649,6 +1646,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// save location (for rendering) + render
_terminal->SetSelectionEnd(terminalPosition);
_renderer->TriggerSelection();
_selectionNeedsToBeCopied = true;
}

// Method Description:
Expand Down Expand Up @@ -1799,6 +1797,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
return false;
}

// Mark the current selection as copied
_selectionNeedsToBeCopied = false;

// extract text from buffer
const auto bufferData = _terminal->RetrieveSelectedTextFromBuffer(collapseText);

Expand All @@ -1822,7 +1824,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_actualFont.GetFaceName(),
_settings.DefaultBackground());

if (!_terminal->IsCopyOnSelectActive())
if (!_settings.CopyOnSelect())
{
_terminal->ClearSelection();
_renderer->TriggerSelection();
Expand Down
11 changes: 7 additions & 4 deletions src/cascadia/TerminalControl/TermControl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// imported from WinUser
// Used for PointerPoint.Timestamp Property (https://docs.microsoft.com/en-us/uwp/api/windows.ui.input.pointerpoint.timestamp#Windows_UI_Input_PointerPoint_Timestamp)
Timestamp _multiClickTimer;
Timestamp _lastMouseClickTimestamp;
unsigned int _multiClickCounter;
Timestamp _lastMouseClickTimestamp;
std::optional<winrt::Windows::Foundation::Point> _lastMouseClickPos;

std::optional<winrt::Windows::Foundation::Point> _focusRaisedClickPos;
bool _clickDrag;
std::optional<winrt::Windows::Foundation::Point> _singleClickTouchdownPos;
// This field tracks whether the selection has changed meaningfully
// since it was last copied. It's generally used to prevent copyOnSelect
// from firing when the pointer _just happens_ to be released over the
// terminal.
bool _selectionNeedsToBeCopied;

winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;

Expand Down
6 changes: 1 addition & 5 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ Terminal::Terminal() :
_scrollOffset{ 0 },
_snapOnInput{ true },
_blockSelection{ false },
_selection{ std::nullopt },
_allowSingleCharSelection{ true },
_copyOnSelect{ false }
_selection{ std::nullopt }
{
auto dispatch = std::make_unique<TerminalDispatch>(*this);
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
Expand Down Expand Up @@ -145,8 +143,6 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting

_wordDelimiters = settings.WordDelimiters();

_copyOnSelect = settings.CopyOnSelect();

_suppressApplicationTitle = settings.SuppressApplicationTitle();

_startingTitle = settings.StartingTitle();
Expand Down
4 changes: 0 additions & 4 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ class Microsoft::Terminal::Core::Terminal final :
Word,
Line
};
const bool IsCopyOnSelectActive() const noexcept;
void MultiClickSelection(const COORD viewportPos, SelectionExpansionMode expansionMode);
void SetSelectionAnchor(const COORD position);
void SetSelectionEnd(const COORD position, std::optional<SelectionExpansionMode> newExpansionMode = std::nullopt);
Expand Down Expand Up @@ -222,8 +221,6 @@ class Microsoft::Terminal::Core::Terminal final :
};
std::optional<SelectionAnchors> _selection;
bool _blockSelection;
bool _allowSingleCharSelection;
bool _copyOnSelect;
std::wstring _wordDelimiters;
SelectionExpansionMode _multiClickSelectionMode;
#pragma endregion
Expand Down Expand Up @@ -275,7 +272,6 @@ class Microsoft::Terminal::Core::Terminal final :
std::pair<COORD, COORD> _PivotSelection(const COORD targetPos) const;
std::pair<COORD, COORD> _ExpandSelectionAnchors(std::pair<COORD, COORD> anchors) const;
COORD _ConvertToBufferCell(const COORD viewportPos) const;
const bool _IsSingleCellSelection() const noexcept;
#pragma endregion

#ifdef UNIT_TESTING
Expand Down
34 changes: 0 additions & 34 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,27 +82,12 @@ const COORD Terminal::GetSelectionEnd() const noexcept
return _selection->end;
}

// Method Description:
// - Checks if selection is on a single cell
// Return Value:
// - bool representing if selection is only a single cell. Used for copyOnSelect
const bool Terminal::_IsSingleCellSelection() const noexcept
{
return (_selection->start == _selection->end);
}

// Method Description:
// - Checks if selection is active
// Return Value:
// - bool representing if selection is active. Used to decide copy/paste on right click
const bool Terminal::IsSelectionActive() const noexcept
{
// A single cell selection is not considered an active selection,
// if it's not allowed
if (!_allowSingleCharSelection && _IsSingleCellSelection())
{
return false;
}
return _selection.has_value();
}

Expand All @@ -111,15 +96,6 @@ const bool Terminal::IsBlockSelection() const noexcept
return _blockSelection;
}

// Method Description:
// - Checks if the CopyOnSelect setting is active
// Return Value:
// - true if feature is active, false otherwise.
const bool Terminal::IsCopyOnSelectActive() const noexcept
{
return _copyOnSelect;
}

// Method Description:
// - Perform a multi-click selection at viewportPos expanding according to the expansionMode
// Arguments:
Expand Down Expand Up @@ -148,8 +124,6 @@ void Terminal::SetSelectionAnchor(const COORD viewportPos)
_selection = SelectionAnchors{};
_selection->pivot = _ConvertToBufferCell(viewportPos);

_allowSingleCharSelection = (_copyOnSelect) ? false : true;

_multiClickSelectionMode = SelectionExpansionMode::Cell;
SetSelectionEnd(viewportPos);

Expand All @@ -172,13 +146,6 @@ void Terminal::SetSelectionEnd(const COORD viewportPos, std::optional<SelectionE

const auto anchors = _PivotSelection(textBufferPos);
std::tie(_selection->start, _selection->end) = _ExpandSelectionAnchors(anchors);

// moving the endpoint of what used to be a single cell selection
// allows the user to drag back and select just one cell
if (_copyOnSelect && !_IsSingleCellSelection())
{
_allowSingleCharSelection = true;
}
}

// Method Description:
Expand Down Expand Up @@ -248,7 +215,6 @@ void Terminal::SetBlockSelection(const bool isEnabled) noexcept
#pragma warning(disable : 26440) // changing this to noexcept would require a change to ConHost's selection model
void Terminal::ClearSelection()
{
_allowSingleCharSelection = false;
_selection = std::nullopt;
}

Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettings/IControlSettings.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ namespace Microsoft.Terminal.Settings

IKeyBindings KeyBindings;

Boolean CopyOnSelect;

String Commandline;
String StartingDirectory;
String EnvironmentVariables;
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalSettings/ICoreSettings.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ namespace Microsoft.Terminal.Settings
String StartingTitle;
Boolean SuppressApplicationTitle;
String WordDelimiters;
Boolean CopyOnSelect;
};

}
44 changes: 0 additions & 44 deletions src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,49 +672,5 @@ namespace TerminalCoreUnitTests
selection = term.GetViewport().ConvertToOrigin(selectionRects.at(1)).ToInclusive();
VERIFY_ARE_EQUAL(selection, SMALL_RECT({ 0, 11, 99, 11 }));
}

TEST_METHOD(CopyOnSelect)
{
Terminal term;
DummyRenderTarget emptyRT;
term.Create({ 100, 100 }, 0, emptyRT);

// set copyOnSelect for terminal
auto settings = winrt::make<MockTermSettings>(0, 100, 100);
settings.CopyOnSelect(true);
term.UpdateSettings(settings);

// Simulate click at (x,y) = (5,10)
term.SetSelectionAnchor({ 5, 10 });

// Simulate move to (x,y) = (5,10)
// (So, no movement)
term.SetSelectionEnd({ 5, 10 });

// Case 1: single cell selection not allowed
{
// Simulate renderer calling TriggerSelection and acquiring selection area
auto selectionRects = term.GetSelectionRects();

// Validate selection area
VERIFY_ARE_EQUAL(selectionRects.size(), static_cast<size_t>(0));

// single cell selection should not be allowed
// thus, selection is NOT active
VERIFY_IS_FALSE(term.IsSelectionActive());
}

// Case 2: move off of single cell
term.SetSelectionEnd({ 6, 10 });
ValidateSingleRowSelection(term, { 5, 10, 6, 10 });
VERIFY_IS_TRUE(term.IsSelectionActive());

// Case 3: move back onto single cell (now allowed)
term.SetSelectionEnd({ 5, 10 });
ValidateSingleRowSelection(term, { 5, 10, 5, 10 });

// single cell selection should now be allowed
VERIFY_IS_TRUE(term.IsSelectionActive());
}
};
}

1 comment on commit 499f24a

@github-actions
Copy link

Choose a reason for hiding this comment

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

New misspellings found, please review:

  • clickdown
  • defterm
  • INLINEPREFIX
  • Inplace
  • kimwalisch
  • libpopcnt
  • msys
  • pinam
  • powf
  • SOURCESDIRECTORY
  • SPACEBAR
  • sqrtf
  • tdbuildteamid
  • Walisch
  • Wojciech
To accept these changes, run the following commands 
(
echo "
clickdown
defterm
INLINEPREFIX
Inplace
kimwalisch
libpopcnt
msys
pinam
powf
SOURCESDIRECTORY
SPACEBAR
sqrtf
tdbuildteamid
Walisch
Wojciech
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/499f24a29e6f321d6c4da8eb22c2ef0d30f13770.txt'

Please sign in to comment.