Skip to content

Commit

Permalink
Port selection in conhost and Terminal to use til::generational (#17676)
Browse files Browse the repository at this point in the history 
In #17638, I am moving selection to an earlier phase of rendering (so
that further phases can take it into account). Since I am drafting off
the design of search highlights, one of the required changes is moving
to passing `span`s of `point_span`s around to make selection effectively
zero-copy.

We can't easily have zero-copy selection propagation without caching,
and we can't have caching without mandatory cache invalidation.

This pull request moves both conhost and Terminal to use
`til::generational` for all selection members that impact the ranges
that would be produced from `GetSelectionRects`.

This required a move from `std::optional<>` to a boolean to determine
whether a selection was active in Terminal.

We will no longer regenerate the selection rects from the selection
anchors plus the text buffer *every single frame*.

Apart from being annoying to read, there is one downside.

If you begin a selection on a narrow character, _and that narrow
character later turns into a wide character_, we will show it as
half-selected.

This should be a rare-enough case that we can accept it as a regression.
  • Loading branch information
@DHowett
DHowett authored Aug 9, 2024
1 parent 7c0d6d9 commit 0199ca3
Show file tree
Hide file tree
Showing 8 changed files with 369 additions and 264 deletions.
9 changes: 6 additions & 3 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "../../types/inc/GlyphWidth.hpp"
#include "../../cascadia/terminalcore/ITerminalInput.hpp"

#include <til/generational.h>
#include <til/ticket_lock.h>
#include <til/winrt.h>

Expand Down Expand Up @@ -382,14 +383,15 @@ class Microsoft::Terminal::Core::Terminal final :
// the pivot is the til::point that remains selected when you extend a selection in any direction
// this is particularly useful when a word selection is extended over its starting point
// see TerminalSelection.cpp for more information
struct SelectionAnchors
struct SelectionInfo
{
til::point start;
til::point end;
til::point pivot;
bool blockSelection = false;
bool active = false;
};
std::optional<SelectionAnchors> _selection;
bool _blockSelection = false;
til::generational<SelectionInfo> _selection{};
std::wstring _wordDelimiters;
SelectionExpansion _multiClickSelectionMode = SelectionExpansion::Char;
SelectionInteractionMode _selectionMode = SelectionInteractionMode::None;
Expand Down Expand Up @@ -474,6 +476,7 @@ class Microsoft::Terminal::Core::Terminal final :
void _MoveByWord(SelectionDirection direction, til::point& pos);
void _MoveByViewport(SelectionDirection direction, til::point& pos) noexcept;
void _MoveByBuffer(SelectionDirection direction, til::point& pos) noexcept;
void _SetSelectionEnd(SelectionInfo* selection, const til::point position, std::optional<SelectionExpansion> newExpansionMode = std::nullopt);
#pragma endregion

#ifdef UNIT_TESTING
Expand Down
16 changes: 9 additions & 7 deletions src/cascadia/TerminalCore/TerminalApi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,23 +339,25 @@ void Terminal::SearchMissingCommand(const std::wstring_view command)
void Terminal::NotifyBufferRotation(const int delta)
{
// Update our selection, so it doesn't move as the buffer is cycled
if (_selection)
if (_selection->active)
{
auto selection{ _selection.write() };
wil::hide_name _selection;
// If the end of the selection will be out of range after the move, we just
// clear the selection. Otherwise we move both the start and end points up
// by the given delta and clamp to the first row.
if (_selection->end.y < delta)
if (selection->end.y < delta)
{
_selection.reset();
selection->active = false;
}
else
{
// Stash this, so we can make sure to update the pivot to match later.
const auto pivotWasStart = _selection->start == _selection->pivot;
_selection->start.y = std::max(_selection->start.y - delta, 0);
_selection->end.y = std::max(_selection->end.y - delta, 0);
const auto pivotWasStart = selection->start == selection->pivot;
selection->start.y = std::max(selection->start.y - delta, 0);
selection->end.y = std::max(selection->end.y - delta, 0);
// Make sure to sync the pivot with whichever value is the right one.
_selection->pivot = pivotWasStart ? _selection->start : _selection->end;
selection->pivot = pivotWasStart ? selection->start : selection->end;
}
}

Expand Down
115 changes: 72 additions & 43 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ std::vector<til::inclusive_rect> Terminal::_GetSelectionRects() const noexcept

try
{
return _activeBuffer().GetTextRects(_selection->start, _selection->end, _blockSelection, false);
return _activeBuffer().GetTextRects(_selection->start, _selection->end, _selection->blockSelection, false);
}
CATCH_LOG();
return result;
Expand All @@ -78,7 +78,7 @@ std::vector<til::point_span> Terminal::_GetSelectionSpans() const noexcept

try
{
return _activeBuffer().GetTextSpans(_selection->start, _selection->end, _blockSelection, false);
return _activeBuffer().GetTextSpans(_selection->start, _selection->end, _selection->blockSelection, false);
}
CATCH_LOG();
return result;
Expand Down Expand Up @@ -158,13 +158,13 @@ const Terminal::SelectionEndpoint Terminal::SelectionEndpointTarget() const noex
const bool Terminal::IsSelectionActive() const noexcept
{
_assertLocked();
return _selection.has_value();
return _selection->active;
}

const bool Terminal::IsBlockSelection() const noexcept
{
_assertLocked();
return _blockSelection;
return _selection->blockSelection;
}

// Method Description:
Expand All @@ -175,15 +175,18 @@ const bool Terminal::IsBlockSelection() const noexcept
void Terminal::MultiClickSelection(const til::point viewportPos, SelectionExpansion expansionMode)
{
// set the selection pivot to expand the selection using SetSelectionEnd()
_selection = SelectionAnchors{};
_selection->pivot = _ConvertToBufferCell(viewportPos);
auto selection{ _selection.write() };
wil::hide_name _selection;

selection->pivot = _ConvertToBufferCell(viewportPos);
selection->active = true;

_multiClickSelectionMode = expansionMode;
SetSelectionEnd(viewportPos);

// we need to set the _selectionPivot again
// for future shift+clicks
_selection->pivot = _selection->start;
selection->pivot = selection->start;
}

// Method Description:
Expand All @@ -194,13 +197,21 @@ void Terminal::SetSelectionAnchor(const til::point viewportPos)
{
_assertLocked();

_selection = SelectionAnchors{};
_selection->pivot = _ConvertToBufferCell(viewportPos);
auto selection{ _selection.write() };
wil::hide_name _selection;

selection->pivot = _ConvertToBufferCell(viewportPos);
selection->active = true;

_multiClickSelectionMode = SelectionExpansion::Char;
SetSelectionEnd(viewportPos);

_selection->start = _selection->pivot;
selection->start = selection->pivot;
}

void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional<SelectionExpansion> newExpansionMode)
{
_SetSelectionEnd(_selection.write(), viewportPos, newExpansionMode);
}

// Method Description:
Expand All @@ -209,9 +220,10 @@ void Terminal::SetSelectionAnchor(const til::point viewportPos)
// Arguments:
// - viewportPos: the (x,y) coordinate on the visible viewport
// - newExpansionMode: overwrites the _multiClickSelectionMode for this function call. Used for ShiftClick
void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional<SelectionExpansion> newExpansionMode)
void Terminal::_SetSelectionEnd(SelectionInfo* selection, const til::point viewportPos, std::optional<SelectionExpansion> newExpansionMode)
{
if (!IsSelectionActive())
wil::hide_name _selection;
if (!selection->active)
{
// capture a log for spurious endpoint sets without an active selection
LOG_HR(E_ILLEGAL_STATE_CHANGE);
Expand All @@ -231,17 +243,17 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional<Selec
if (newExpansionMode.has_value())
{
// shift-click operations only expand the target side
auto& anchorToExpand = targetStart ? _selection->start : _selection->end;
auto& anchorToExpand = targetStart ? selection->start : selection->end;
anchorToExpand = targetStart ? expandedAnchors.first : expandedAnchors.second;

// the other anchor should then become the pivot (we don't expand it)
auto& anchorToPivot = targetStart ? _selection->end : _selection->start;
anchorToPivot = _selection->pivot;
auto& anchorToPivot = targetStart ? selection->end : selection->start;
anchorToPivot = selection->pivot;
}
else
{
// expand both anchors
std::tie(_selection->start, _selection->end) = expandedAnchors;
std::tie(selection->start, selection->end) = expandedAnchors;
}
_selectionMode = SelectionInteractionMode::Mouse;
_selectionIsTargetingUrl = false;
Expand Down Expand Up @@ -307,7 +319,7 @@ std::pair<til::point, til::point> Terminal::_ExpandSelectionAnchors(std::pair<ti
// - isEnabled: new value for _blockSelection
void Terminal::SetBlockSelection(const bool isEnabled) noexcept
{
_blockSelection = isEnabled;
_selection.write()->blockSelection = isEnabled;
}

Terminal::SelectionInteractionMode Terminal::SelectionMode() const noexcept
Expand All @@ -331,11 +343,13 @@ void Terminal::ToggleMarkMode()
{
// No selection --> start one at the cursor
const auto cursorPos{ _activeBuffer().GetCursor().GetPosition() };
_selection = SelectionAnchors{};
_selection->start = cursorPos;
_selection->end = cursorPos;
_selection->pivot = cursorPos;
_blockSelection = false;
*_selection.write() = SelectionInfo{
.start = cursorPos,
.end = cursorPos,
.pivot = cursorPos,
.blockSelection = false,
.active = true,
};
WI_SetAllFlags(_selectionEndpoint, SelectionEndpoint::Start | SelectionEndpoint::End);
}
else if (WI_AreAllFlagsClear(_selectionEndpoint, SelectionEndpoint::Start | SelectionEndpoint::End))
Expand Down Expand Up @@ -365,13 +379,13 @@ void Terminal::SwitchSelectionEndpoint() noexcept
{
// moving end --> now we're moving start
_selectionEndpoint = SelectionEndpoint::Start;
_selection->pivot = _selection->end;
_selection.write()->pivot = _selection->end;
}
else if (WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::Start))
{
// moving start --> now we're moving end
_selectionEndpoint = SelectionEndpoint::End;
_selection->pivot = _selection->start;
_selection.write()->pivot = _selection->start;
}
}
}
Expand All @@ -381,9 +395,11 @@ void Terminal::ExpandSelectionToWord()
if (IsSelectionActive())
{
const auto& buffer = _activeBuffer();
_selection->start = buffer.GetWordStart(_selection->start, _wordDelimiters);
_selection->pivot = _selection->start;
_selection->end = buffer.GetWordEnd(_selection->end, _wordDelimiters);
auto selection{ _selection.write() };
wil::hide_name _selection;
selection->start = buffer.GetWordStart(selection->start, _wordDelimiters);
selection->pivot = selection->start;
selection->end = buffer.GetWordEnd(selection->end, _wordDelimiters);

// if we're targeting both endpoints, instead just target "end"
if (WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::Start) && WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::End))
Expand Down Expand Up @@ -528,12 +544,16 @@ void Terminal::SelectHyperlink(const SearchDirection dir)
}

// 2. Select the hyperlink
_selection->start = result->first;
_selection->pivot = result->first;
_selection->end = result->second;
bufferSize.DecrementInBounds(_selection->end);
_selectionIsTargetingUrl = true;
_selectionEndpoint = SelectionEndpoint::End;
{
auto selection{ _selection.write() };
wil::hide_name _selection;
selection->start = result->first;
selection->pivot = result->first;
selection->end = result->second;
bufferSize.DecrementInBounds(selection->end);
_selectionIsTargetingUrl = true;
_selectionEndpoint = SelectionEndpoint::End;
}

// 3. Scroll to the selected area (if necessary)
_ScrollToPoint(_selection->end);
Expand Down Expand Up @@ -646,19 +666,23 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion
// 3. Actually modify the selection state
_selectionIsTargetingUrl = false;
_selectionMode = std::max(_selectionMode, SelectionInteractionMode::Keyboard);

auto selection{ _selection.write() };
wil::hide_name _selection;

if (shouldMoveBothEndpoints)
{
// [Mark Mode] + shift unpressed --> move all three (i.e. just use arrow keys)
_selection->start = targetPos;
_selection->end = targetPos;
_selection->pivot = targetPos;
selection->start = targetPos;
selection->end = targetPos;
selection->pivot = targetPos;
}
else
{
// [Mark Mode] + shift --> updating a standard selection
// This is also standard quick-edit modification
bool targetStart = false;
std::tie(_selection->start, _selection->end) = _PivotSelection(targetPos, targetStart);
std::tie(selection->start, selection->end) = _PivotSelection(targetPos, targetStart);

// IMPORTANT! Pivoting the selection here might have changed which endpoint we're targeting.
// So let's set the targeted endpoint again.
Expand All @@ -673,10 +697,15 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion
void Terminal::SelectAll()
{
const auto bufferSize{ _activeBuffer().GetSize() };
_selection = SelectionAnchors{};
_selection->start = bufferSize.Origin();
_selection->end = { bufferSize.RightInclusive(), _GetMutableViewport().BottomInclusive() };
_selection->pivot = _selection->end;
const til::point end{ bufferSize.RightInclusive(), _GetMutableViewport().BottomInclusive() };
*_selection.write() = SelectionInfo{
.start = bufferSize.Origin(),
.end = end,
.pivot = end,
.blockSelection = false,
.active = true,
};

_selectionMode = SelectionInteractionMode::Keyboard;
_selectionIsTargetingUrl = false;
_ScrollToPoint(_selection->start);
Expand Down Expand Up @@ -824,7 +853,7 @@ void Terminal::_MoveByBuffer(SelectionDirection direction, til::point& pos) noex
void Terminal::ClearSelection()
{
_assertLocked();
_selection = std::nullopt;
_selection.write()->active = false;
_selectionMode = SelectionInteractionMode::None;
_selectionIsTargetingUrl = false;
_selectionEndpoint = static_cast<SelectionEndpoint>(0);
Expand Down Expand Up @@ -858,7 +887,7 @@ Terminal::TextCopyData Terminal::RetrieveSelectedTextFromBuffer(const bool singl

const auto& textBuffer = _activeBuffer();

const auto req = TextBuffer::CopyRequest::FromConfig(textBuffer, _selection->start, _selection->end, singleLine, _blockSelection, _trimBlockSelection);
const auto req = TextBuffer::CopyRequest::FromConfig(textBuffer, _selection->start, _selection->end, singleLine, _selection->blockSelection, _trimBlockSelection);
data.plainText = textBuffer.GetPlainText(req);

if (html || rtf)
Expand Down
Loading

0 comments on commit 0199ca3

Please sign in to comment.