Rework and simplify selection in TermControl

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 }
+        _selectionNeedsToBeCopied{ 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);
+                    _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();
@@ -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();
                 }
@@ -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<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);
                 }
@@ -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();
+        _selectionNeedsToBeCopied = true;
     }
 
     // Method Description:
@@ -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);
 
@@ -1822,7 +1824,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
                                                 _actualFont.GetFaceName(),
                                                 _settings.DefaultBackground());
 
-        if (!_terminal->IsCopyOnSelectActive())
+        if (!_settings.CopyOnSelect())
         {
             _terminal->ClearSelection();
             _renderer->TriggerSelection();

src/cascadia/TerminalControl/TermControl.h

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

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<TerminalDispatch>(*this);
     auto engine = std::make_unique<OutputStateMachineEngine>(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();

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<SelectionExpansionMode> newExpansionMode = std::nullopt);
@@ -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
@@ -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

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::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:
@@ -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;
 }
 

src/cascadia/TerminalSettings/IControlSettings.idl

@@ -38,6 +38,8 @@ namespace Microsoft.Terminal.Settings
 
         IKeyBindings KeyBindings;
 
+        Boolean CopyOnSelect;
+
         String Commandline;
         String StartingDirectory;
         String EnvironmentVariables;

src/cascadia/TerminalSettings/ICoreSettings.idl

@@ -32,7 +32,6 @@ namespace Microsoft.Terminal.Settings
         String StartingTitle;
         Boolean SuppressApplicationTitle;
         String WordDelimiters;
-        Boolean CopyOnSelect;
     };
 
 }

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<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());
-        }
     };
 }