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.

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

Merged
merged 10 commits into from
Sep 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@
}
],
"required": [ "actions" ]
},
},
"CommandPaletteAction": {
"description": "Arguments for a commandPalette action",
"allOf": [
Expand Down
10 changes: 7 additions & 3 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1430,12 +1430,13 @@ const til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional<t
}

// Method Description:
// - Update pos to be the end of the current glyph/character. This is used for accessibility
// - Update pos to be the end of the current glyph/character.
// Arguments:
// - pos - a COORD on the word you are currently on
// - accessibilityMode - this is being used for accessibility; make the end exclusive.
// Return Value:
// - pos - The COORD for the last cell of the current glyph (exclusive)
const til::point TextBuffer::GetGlyphEnd(const til::point pos, std::optional<til::point> limitOptional) const
const til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode, std::optional<til::point> limitOptional) const
Copy link
Member

@lhecker lhecker Sep 22, 2021

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"?

Copy link
Member Author

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...

Selection A11y
endpoint representation both endpoints are inclusive; start <= end inclusive start; exclusive end; start <= end
definition of a word a run of legible characters run of legible characters + run of whitespace after it (until next word starts)

I just chose to name the parameter accessibilityMode to make it clear what and when we're only doing some extra work for a11y.

{
COORD resultPos = pos;
const auto bufferSize = GetSize();
Expand All @@ -1453,7 +1454,10 @@ const til::point TextBuffer::GetGlyphEnd(const til::point pos, std::optional<til
}

// increment one more time to become exclusive
bufferSize.IncrementInBounds(resultPos, true);
if (accessibilityMode)
{
bufferSize.IncrementInBounds(resultPos, true);
}
return resultPos;
}

Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class TextBuffer final
bool MoveToPreviousWord(COORD& pos, const std::wstring_view wordDelimiters) const;

const til::point GetGlyphStart(const til::point pos, std::optional<til::point> limitOptional = std::nullopt) const;
const til::point GetGlyphEnd(const til::point pos, std::optional<til::point> limitOptional = std::nullopt) const;
const til::point GetGlyphEnd(const til::point pos, bool accessibilityMode = false, std::optional<til::point> limitOptional = std::nullopt) const;
bool MoveToNextGlyph(til::point& pos, bool allowBottomExclusive = false, std::optional<til::point> limitOptional = std::nullopt) const;
bool MoveToPreviousGlyph(til::point& pos, std::optional<til::point> limitOptional = std::nullopt) const;

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/PublicTerminalCore/HwndTerminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -549,11 +549,11 @@ try

if (multiClickMapper == 3)
{
_terminal->MultiClickSelection(cursorPosition / fontSize, ::Terminal::SelectionExpansionMode::Line);
_terminal->MultiClickSelection(cursorPosition / fontSize, ::Terminal::SelectionExpansion::Line);
}
else if (multiClickMapper == 2)
{
_terminal->MultiClickSelection(cursorPosition / fontSize, ::Terminal::SelectionExpansionMode::Word);
_terminal->MultiClickSelection(cursorPosition / fontSize, ::Terminal::SelectionExpansion::Word);
}
else
{
Expand Down
31 changes: 20 additions & 11 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 markMode to shift+left, and have the shift+left keydown enter mark mode, and the keyup extend the selection to the left? Essentially getting quickedit-like selection?

This is of course, crazy, but something to think about

Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -1557,5 +1567,4 @@ namespace winrt::Microsoft::Terminal::Control::implementation

return hstring(ss.str());
}

}
30 changes: 24 additions & 6 deletions src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,16 +227,30 @@ class Microsoft::Terminal::Core::Terminal final :

#pragma region TextSelection
// These methods are defined in TerminalSelection.cpp
enum class SelectionExpansionMode
enum class SelectionDirection
{
Cell,
Left,
Right,
Up,
Down
};

enum class SelectionExpansion
{
Char,
Word,
Line
Line, // Mouse selection only!
Viewport,
Buffer
};
void MultiClickSelection(const COORD viewportPos, SelectionExpansionMode expansionMode);
void MultiClickSelection(const COORD viewportPos, SelectionExpansion expansionMode);
void SetSelectionAnchor(const COORD position);
void SetSelectionEnd(const COORD position, std::optional<SelectionExpansionMode> newExpansionMode = std::nullopt);
void SetSelectionEnd(const COORD position, std::optional<SelectionExpansion> newExpansionMode = std::nullopt);
void SetBlockSelection(const bool isEnabled) noexcept;
void UpdateSelection(SelectionDirection direction, SelectionExpansion mode);

using UpdateSelectionParams = std::optional<std::pair<SelectionDirection, SelectionExpansion>>;
static UpdateSelectionParams ConvertKeyEventToUpdateSelectionParams(const ControlKeyStates mods, const WORD vkey);

const TextBuffer::TextAndColor RetrieveSelectedTextFromBuffer(bool trimTrailingWhitespace);
#pragma endregion
Expand Down Expand Up @@ -308,7 +322,7 @@ class Microsoft::Terminal::Core::Terminal final :
std::optional<SelectionAnchors> _selection;
bool _blockSelection;
std::wstring _wordDelimiters;
SelectionExpansionMode _multiClickSelectionMode;
SelectionExpansion _multiClickSelectionMode;
#pragma endregion

// TODO: These members are not shared by an alt-buffer. They should be
Expand Down Expand Up @@ -375,6 +389,10 @@ class Microsoft::Terminal::Core::Terminal final :
std::pair<COORD, COORD> _PivotSelection(const COORD targetPos, bool& targetStart) const;
std::pair<COORD, COORD> _ExpandSelectionAnchors(std::pair<COORD, COORD> anchors) const;
COORD _ConvertToBufferCell(const COORD viewportPos) const;
void _MoveByChar(SelectionDirection direction, COORD& pos);
void _MoveByWord(SelectionDirection direction, COORD& pos);
void _MoveByViewport(SelectionDirection direction, COORD& pos);
void _MoveByBuffer(SelectionDirection direction, COORD& pos);
#pragma endregion

Microsoft::Console::VirtualTerminal::SgrStack _sgrStack;
Expand Down
Loading