From 53080d304103e6d40af33938e1277d13e08e2dc9 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 23 Mar 2020 23:06:53 -0700 Subject: [PATCH 1/4] Rework and simplify selection in TermControl 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 #4082; Fixes #4477 --- src/cascadia/TerminalControl/TermControl.cpp | 60 ++++++++++--------- src/cascadia/TerminalControl/TermControl.h | 7 +-- src/cascadia/TerminalCore/Terminal.cpp | 6 +- src/cascadia/TerminalCore/Terminal.hpp | 4 -- .../TerminalCore/TerminalSelection.cpp | 34 ----------- 5 files changed, 35 insertions(+), 76 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 47d93ca8591..ea370d19889 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -69,9 +69,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _cursorTimer{}, _lastMouseClickTimestamp{}, _lastMouseClickPos{}, - _searchBox{ nullptr }, - _focusRaisedClickPos{ std::nullopt }, - _clickDrag{ false } + _selectionUpdatedThisCycle{ false }, + _searchBox{ nullptr } { _EnsureStaticInitialization(); InitializeComponent(); @@ -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) @@ -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); @@ -956,20 +945,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation if (multiClickMapper == 3) { _terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Line); + _selectionUpdatedThisCycle = true; } else if (multiClickMapper == 2) { _terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Word); + _selectionUpdatedThisCycle = true; } else { if (shiftEnabled && _terminal->IsSelectionActive()) { _terminal->SetSelectionEnd(terminalPosition, ::Terminal::SelectionExpansionMode::Cell); + _selectionUpdatedThisCycle = true; } else { - _terminal->SetSelectionAnchor(terminalPosition); + // A single click down resets the selection and begins a new one. + _terminal->ClearSelection(); + _singleClickTouchdownPos = cursorPosition; + _selectionUpdatedThisCycle = false; // there's no selection, so there's nothing to update } _lastMouseClickTimestamp = point.Timestamp(); @@ -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 (!_selectionUpdatedThisCycle || !_terminal->IsSelectionActive()) { PasteTextFromClipboard(); } @@ -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(); @@ -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(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(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 (_selectionUpdatedThisCycle) { CopySelectionToClipboard(shiftEnabled); } @@ -1142,8 +1140,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _touchAnchor = std::nullopt; } - _focusRaisedClickPos = std::nullopt; - _clickDrag = false; + _singleClickTouchdownPos = std::nullopt; _TryStopAutoScroll(ptr.PointerId()); @@ -1649,6 +1646,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // save location (for rendering) + render _terminal->SetSelectionEnd(terminalPosition); _renderer->TriggerSelection(); + _selectionUpdatedThisCycle = true; } // Method Description: @@ -1799,6 +1797,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { return false; } + + // Mark the current selection as copied + _selectionUpdatedThisCycle = false; + // extract text from buffer const auto bufferData = _terminal->RetrieveSelectedTextFromBuffer(collapseText); @@ -1822,7 +1824,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _actualFont.GetFaceName(), _settings.DefaultBackground()); - if (!_terminal->IsCopyOnSelectActive()) + if (!_settings.CopyOnSelect()) { _terminal->ClearSelection(); _renderer->TriggerSelection(); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 57606506bee..39c900d1026 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -155,12 +155,11 @@ 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 _lastMouseClickPos; - - std::optional _focusRaisedClickPos; - bool _clickDrag; + std::optional _singleClickTouchdownPos; + bool _selectionUpdatedThisCycle; winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 36daae54fce..265d1f3d372 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -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(*this); auto engine = std::make_unique(std::move(dispatch)); @@ -145,8 +143,6 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting _wordDelimiters = settings.WordDelimiters(); - _copyOnSelect = settings.CopyOnSelect(); - _suppressApplicationTitle = settings.SuppressApplicationTitle(); _startingTitle = settings.StartingTitle(); diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 8498efa1aff..c50e3a91143 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -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 newExpansionMode = std::nullopt); @@ -222,8 +221,6 @@ class Microsoft::Terminal::Core::Terminal final : }; std::optional _selection; bool _blockSelection; - bool _allowSingleCharSelection; - bool _copyOnSelect; std::wstring _wordDelimiters; SelectionExpansionMode _multiClickSelectionMode; #pragma endregion @@ -275,7 +272,6 @@ class Microsoft::Terminal::Core::Terminal final : std::pair _PivotSelection(const COORD targetPos) const; std::pair _ExpandSelectionAnchors(std::pair anchors) const; COORD _ConvertToBufferCell(const COORD viewportPos) const; - const bool _IsSingleCellSelection() const noexcept; #pragma endregion #ifdef UNIT_TESTING diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 3d096e2ec5e..f8a13a56151 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -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(); } @@ -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: @@ -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); @@ -172,13 +146,6 @@ void Terminal::SetSelectionEnd(const COORD viewportPos, std::optionalstart, _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: @@ -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; } From 975622db12849a72c045203842e9ae141a521a23 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 24 Mar 2020 10:24:02 -0700 Subject: [PATCH 2/4] This test dies with TerminalCore's knowledge of CoS --- .../UnitTests_TerminalCore/SelectionTest.cpp | 44 ------------------- 1 file changed, 44 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp index 447910e47eb..5e117dc8db6 100644 --- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp @@ -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(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(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()); - } }; } From 67092d4512cbda638ebb98ec35098872f5e4bd50 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 25 Mar 2020 12:56:23 -0700 Subject: [PATCH 3/4] PR feedback, comment --- src/cascadia/TerminalControl/TermControl.cpp | 18 +++++++++--------- src/cascadia/TerminalControl/TermControl.h | 6 +++++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index ea370d19889..dfc057ee84c 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -69,7 +69,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _cursorTimer{}, _lastMouseClickTimestamp{}, _lastMouseClickPos{}, - _selectionUpdatedThisCycle{ false }, + _selectionNeedsToBeCopied{ false }, _searchBox{ nullptr } { _EnsureStaticInitialization(); @@ -945,26 +945,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation if (multiClickMapper == 3) { _terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Line); - _selectionUpdatedThisCycle = true; + _selectionNeedsToBeCopied = true; } else if (multiClickMapper == 2) { _terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Word); - _selectionUpdatedThisCycle = true; + _selectionNeedsToBeCopied = true; } else { if (shiftEnabled && _terminal->IsSelectionActive()) { _terminal->SetSelectionEnd(terminalPosition, ::Terminal::SelectionExpansionMode::Cell); - _selectionUpdatedThisCycle = true; + _selectionNeedsToBeCopied = true; } else { // A single click down resets the selection and begins a new one. _terminal->ClearSelection(); _singleClickTouchdownPos = cursorPosition; - _selectionUpdatedThisCycle = false; // there's no selection, so there's nothing to update + _selectionNeedsToBeCopied = false; // there's no selection, so there's nothing to update } _lastMouseClickTimestamp = point.Timestamp(); @@ -975,7 +975,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation else if (point.Properties().IsRightButtonPressed()) { // CopyOnSelect right click always pastes - if (!_selectionUpdatedThisCycle || !_terminal->IsSelectionActive()) + if (_settings.CopyOnSelect() || !_terminal->IsSelectionActive()) { PasteTextFromClipboard(); } @@ -1129,7 +1129,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // macro directly with a VirtualKeyModifiers const auto shiftEnabled = WI_IsFlagSet(modifiers, static_cast(VirtualKeyModifiers::Shift)); - if (_selectionUpdatedThisCycle) + if (_selectionNeedsToBeCopied) { CopySelectionToClipboard(shiftEnabled); } @@ -1646,7 +1646,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // save location (for rendering) + render _terminal->SetSelectionEnd(terminalPosition); _renderer->TriggerSelection(); - _selectionUpdatedThisCycle = true; + _selectionNeedsToBeCopied = true; } // Method Description: @@ -1799,7 +1799,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } // Mark the current selection as copied - _selectionUpdatedThisCycle = false; + _selectionNeedsToBeCopied = false; // extract text from buffer const auto bufferData = _terminal->RetrieveSelectedTextFromBuffer(collapseText); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 39c900d1026..9c4ab8fe666 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -159,7 +159,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation Timestamp _lastMouseClickTimestamp; std::optional _lastMouseClickPos; std::optional _singleClickTouchdownPos; - bool _selectionUpdatedThisCycle; + // 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; From 9cfb0d5874f5a307a456ce10df573748c5ee556f Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 25 Mar 2020 13:13:33 -0700 Subject: [PATCH 4/4] Move CopyOnSelect to Control settings, complete the trifecta --- src/cascadia/TerminalSettings/IControlSettings.idl | 2 ++ src/cascadia/TerminalSettings/ICoreSettings.idl | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettings/IControlSettings.idl b/src/cascadia/TerminalSettings/IControlSettings.idl index 12400ad9d9b..4f40c32691b 100644 --- a/src/cascadia/TerminalSettings/IControlSettings.idl +++ b/src/cascadia/TerminalSettings/IControlSettings.idl @@ -38,6 +38,8 @@ namespace Microsoft.Terminal.Settings IKeyBindings KeyBindings; + Boolean CopyOnSelect; + String Commandline; String StartingDirectory; String EnvironmentVariables; diff --git a/src/cascadia/TerminalSettings/ICoreSettings.idl b/src/cascadia/TerminalSettings/ICoreSettings.idl index d76352c7a21..305d85adb55 100644 --- a/src/cascadia/TerminalSettings/ICoreSettings.idl +++ b/src/cascadia/TerminalSettings/ICoreSettings.idl @@ -32,7 +32,6 @@ namespace Microsoft.Terminal.Settings String StartingTitle; Boolean SuppressApplicationTitle; String WordDelimiters; - Boolean CopyOnSelect; }; }