-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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.
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
Implement Keyboard Selection #10824
Implement Keyboard Selection #10824
Changes from all commits
428dec9
bcbf01e
a0e4f2d
8a64613
724cd2c
bf0cf5f
b13b8a7
118d1cd
30009eb
b784262
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -359,28 +359,38 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
const ControlKeyStates modifiers, | ||
const bool keyDown) | ||
{ | ||
// When there is a selection active, escape should clear it and NOT flow through | ||
// to the terminal. With any other keypress, it should clear the selection AND | ||
// flow through to the terminal. | ||
// Update the selection, if it's present | ||
// GH#6423 - don't dismiss selection if the key that was pressed was a | ||
// modifier key. We'll wait for a real keystroke to dismiss the | ||
// GH #7395 - don't dismiss selection when taking PrintScreen | ||
// selection. | ||
// GH#8522, GH#3758 - Only dismiss the selection on key _down_. If we | ||
// dismiss on key up, then there's chance that we'll immediately dismiss | ||
// GH#8522, GH#3758 - Only modify the selection on key _down_. If we | ||
// modify on key up, then there's chance that we'll immediately dismiss | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. <showerthought> If we could handle this modification on key up, then could we do something like bind This is of course, crazy, but something to think about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HECK IT DOESN"T EVEN MATTER a user could just bind markmost to shift+left, and that would start a selection at the cursor, and the subsequent keypresses would extend it without hitting the keybinding, which is actually just like conhost. Brilliant 🥂 |
||
// a selection created by an action bound to a keydown. | ||
if (HasSelection() && | ||
!KeyEvent::IsModifierKey(vkey) && | ||
vkey != VK_SNAPSHOT && | ||
keyDown) | ||
{ | ||
// try to update the selection | ||
if (const auto updateSlnParams{ ::Terminal::ConvertKeyEventToUpdateSelectionParams(modifiers, vkey) }) | ||
{ | ||
auto lock = _terminal->LockForWriting(); | ||
_terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second); | ||
_renderer->TriggerSelection(); | ||
return true; | ||
} | ||
|
||
// GH#8791 - don't dismiss selection if Windows key was also pressed as a key-combination. | ||
if (!modifiers.IsWinPressed()) | ||
{ | ||
_terminal->ClearSelection(); | ||
_renderer->TriggerSelection(); | ||
} | ||
|
||
// When there is a selection active, escape should clear it and NOT flow through | ||
// to the terminal. With any other keypress, it should clear the selection AND | ||
// flow through to the terminal. | ||
if (vkey == VK_ESCAPE) | ||
{ | ||
return true; | ||
|
@@ -1422,18 +1432,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
// handle ALT key | ||
_terminal->SetBlockSelection(altEnabled); | ||
|
||
::Terminal::SelectionExpansionMode mode = ::Terminal::SelectionExpansionMode::Cell; | ||
::Terminal::SelectionExpansion mode = ::Terminal::SelectionExpansion::Char; | ||
if (numberOfClicks == 1) | ||
{ | ||
mode = ::Terminal::SelectionExpansionMode::Cell; | ||
mode = ::Terminal::SelectionExpansion::Char; | ||
} | ||
else if (numberOfClicks == 2) | ||
{ | ||
mode = ::Terminal::SelectionExpansionMode::Word; | ||
mode = ::Terminal::SelectionExpansion::Word; | ||
} | ||
else if (numberOfClicks == 3) | ||
{ | ||
mode = ::Terminal::SelectionExpansionMode::Line; | ||
mode = ::Terminal::SelectionExpansion::Line; | ||
} | ||
|
||
// Update the selection appropriately | ||
|
@@ -1458,7 +1468,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
_terminal->SetSelectionEnd(terminalPosition, mode); | ||
selectionNeedsToBeCopied = true; | ||
} | ||
else if (mode != ::Terminal::SelectionExpansionMode::Cell || shiftEnabled) | ||
else if (mode != ::Terminal::SelectionExpansion::Char || shiftEnabled) | ||
{ | ||
// If we are handling a double / triple-click or shift+single click | ||
// we establish selection using the selected mode | ||
|
@@ -1557,5 +1567,4 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
|
||
return hstring(ss.str()); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The amount of arguments our
TextBuffer
member functions have is worrying ngl. 😄)Why does "accessibility mode" mean "become exclusive" here? Don't you mean "bool exclusivePosition"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeeeeaaaaah. So, a11y and selection have a lot of similarities and very small/important differences. Some highlights include...
I just chose to name the parameter
accessibilityMode
to make it clear what and when we're only doing some extra work for a11y.