Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix selection logic with shift on multi-click #9403

Merged
4 commits merged into from
Mar 8, 2021
Merged

Fix selection logic with shift on multi-click #9403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7cca19d
Fix selection not to extend on multi-click
Don-Vito Mar 5, 2021
1aa147f
Introduce selection reset on anchor / same position click
Don-Vito Mar 7, 2021
a11c9c2
Remove clean selection on anchor as mintty lives without it
Don-Vito Mar 7, 2021
8d9d713
Remove extra space
carlos-zamora Mar 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
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 @@ -1342,35 +1343,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;
}
Comment on lines +1374 to +1389
Copy link
Member

Choose a reason for hiding this comment

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

IN RETROSPECT: I don't think this is right. We might have lost the case where there's no selection, and you double/triple click on a word

@Don-Vito as an fyi

Copy link
Member

Choose a reason for hiding this comment

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

I may stage a revert for this until we can get it in and get it right.

I'd like to see manual test runs for the following scenarios:

  • single click = no selection
  • single click and drag = selection starting from first point
  • single click in unfocused pane and drag = focus pane, selection starting from first point
  • double-click = selects a whole word
  • triple-click = selects a whole line
  • double-click and drag = selects a whole word, drag selects whole words
  • triple-click and drag = selects a whole line, drag selects whole lines
  • Shift single-click = defines start point
  • second Shift single-click = defines end point
  • Shift double-click = selects entire word
  • Shift triple-click = selects entire line
  • Shift double-click and drag = selects entire word, drag selects whole words
  • Shift triple-click and drag = selects entire line, drag selects whole lines


_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
View file
Open in desktop
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