Skip to content

Commit

Permalink
Fix selection logic with shift on multi-click (#9403)
Browse files Browse the repository at this point in the history
## PR Checklist
* [x] Closes #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.
  • Loading branch information
Don-Vito authored Mar 8, 2021
1 parent 87fa526 commit 83f2a3b
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 21 deletions.
63 changes: 42 additions & 21 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_blinkTimer{},
_lastMouseClickTimestamp{},
_lastMouseClickPos{},
_lastMouseClickPosNoSelection{},
_selectionNeedsToBeCopied{ false },
_searchBox{ nullptr }
{
Expand Down Expand Up @@ -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())
{
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
unsigned int _multiClickCounter;
Timestamp _lastMouseClickTimestamp;
std::optional<winrt::Windows::Foundation::Point> _lastMouseClickPos;
std::optional<winrt::Windows::Foundation::Point> _lastMouseClickPosNoSelection;
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
Expand Down

0 comments on commit 83f2a3b

Please sign in to comment.