From 83f2a3bb3dbb00a6e36aa29cd7dba01060254666 Mon Sep 17 00:00:00 2001 From: Don-Vito Date: Mon, 8 Mar 2021 21:15:46 +0200 Subject: [PATCH] Fix selection logic with shift on multi-click (#9403) ## PR Checklist * [x] Closes https://github.com/microsoft/terminal/issues/9382 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. ## Detailed Description of the Pull Request / Additional comments The selection with shift is quite broken in 1.6. It started with #8611 that introduces cell selection on `shift+click`. This change resulted in the following defect: `shift+double-click`, `shift+triple-click` select only parts of the word. The reason for this is that the first `shift+click` establishes the selection, while the consequent clicks simply extend it to the relevant boundary (aka word / line boundary) However, the logic was broken even before #8611. For instance, `shift+triple-click` had exactly the same handicap: `shift+double-click` was establishing the selection and the third click was simply extending it to the line boundary. This PR addresses the both defects in the following manner: upon multi-click that starts new selection we establish a new selection on every consequent click using appropriate mode (cell/word/line) rather than trying to extend one. For this purpose we remember the position that started the selection. --- src/cascadia/TerminalControl/TermControl.cpp | 63 +++++++++++++------- src/cascadia/TerminalControl/TermControl.h | 1 + 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index a63d29869..136c91221 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -79,6 +79,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _blinkTimer{}, _lastMouseClickTimestamp{}, _lastMouseClickPos{}, + _lastMouseClickPosNoSelection{}, _selectionNeedsToBeCopied{ false }, _searchBox{ nullptr } { @@ -1345,35 +1346,55 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation mode = ::Terminal::SelectionExpansionMode::Line; } - // Update the selection appropriately if (ctrlEnabled && multiClickMapper == 1 && !(_terminal->GetHyperlinkAtPosition(terminalPosition).empty())) { _HyperlinkHandler(_terminal->GetHyperlinkAtPosition(terminalPosition)); } - else if (shiftEnabled && _terminal->IsSelectionActive()) - { - // Shift+Click: only set expand on the "end" selection point - _terminal->SetSelectionEnd(terminalPosition, mode); - _selectionNeedsToBeCopied = true; - } - else if (mode == ::Terminal::SelectionExpansionMode::Cell && !shiftEnabled) - { - // Single Click: reset the selection and begin a new one - _terminal->ClearSelection(); - _singleClickTouchdownPos = cursorPosition; - _selectionNeedsToBeCopied = false; // there's no selection, so there's nothing to update - } else { - // Multi-Click Selection: expand both "start" and "end" selection points - _terminal->MultiClickSelection(terminalPosition, mode); - _selectionNeedsToBeCopied = true; - } + // Update the selection appropriately - _lastMouseClickTimestamp = point.Timestamp(); - _lastMouseClickPos = cursorPosition; - _renderer->TriggerSelection(); + // Capture the position of the first click when no selection is active + if (mode == ::Terminal::SelectionExpansionMode::Cell && !_terminal->IsSelectionActive()) + { + _singleClickTouchdownPos = cursorPosition; + _lastMouseClickPosNoSelection = cursorPosition; + } + + // We reset the active selection if one of the conditions apply: + // - shift is not held + // - GH#9384: the position is the same as of the first click starting the selection + // (we need to reset selection on double-click or triple-click, so it captures the word or the line, + // rather than extending the selection) + if (_terminal->IsSelectionActive() && (!shiftEnabled || _lastMouseClickPosNoSelection == cursorPosition)) + { + // Reset the selection + _terminal->ClearSelection(); + _selectionNeedsToBeCopied = false; // there's no selection, so there's nothing to update + } + + if (shiftEnabled) + { + if (_terminal->IsSelectionActive()) + { + // If there is a selection we extend it using the selection mode + // (expand the "end"selection point) + _terminal->SetSelectionEnd(terminalPosition, mode); + } + else + { + // If there is no selection we establish it using the selected mode + // (expand both "start" and "end" selection points) + _terminal->MultiClickSelection(terminalPosition, mode); + } + _selectionNeedsToBeCopied = true; + } + + _lastMouseClickTimestamp = point.Timestamp(); + _lastMouseClickPos = cursorPosition; + _renderer->TriggerSelection(); + } } else if (point.Properties().IsRightButtonPressed()) { diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 4a8514200..0cd9b463b 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -261,6 +261,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation unsigned int _multiClickCounter; Timestamp _lastMouseClickTimestamp; std::optional _lastMouseClickPos; + std::optional _lastMouseClickPosNoSelection; std::optional _singleClickTouchdownPos; // This field tracks whether the selection has changed meaningfully // since it was last copied. It's generally used to prevent copyOnSelect