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

Avoid moving the selection while typing a search query #15998

Merged
merged 2 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
52 changes: 34 additions & 18 deletions src/buffer/out/search.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,21 @@

using namespace Microsoft::Console::Types;

bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData)
{
return ResetIfStale(renderData,
_needle,
_step == -1, // this is the opposite of the initializer below
_caseInsensitive);
}

bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool reverse, bool caseInsensitive)
{
const auto& textBuffer = renderData.GetTextBuffer();
const auto lastMutationId = textBuffer.GetLastMutationId();

if (_needle == needle &&
_reverse == reverse &&
_caseInsensitive == caseInsensitive &&
_lastMutationId == lastMutationId)
{
_step = reverse ? -1 : 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the new MoveToCurrentSelection function would not move past the current selection anymore, changing the direction would not move past the current result either. To fix this, we now don't invalidate the search cache when changing the direction.

return false;
}

_renderData = &renderData;
_needle = needle;
_reverse = reverse;
_caseInsensitive = caseInsensitive;
_lastMutationId = lastMutationId;

Expand All @@ -42,12 +33,38 @@ bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, c
return true;
}

void Search::MovePastCurrentSelection()
void Search::MoveToCurrentSelection()
{
if (_renderData->IsSelectionActive())
{
MovePastPoint(_renderData->GetTextBuffer().ScreenToBufferPosition(_renderData->GetSelectionAnchor()));
MoveToPoint(_renderData->GetTextBuffer().ScreenToBufferPosition(_renderData->GetSelectionAnchor()));
}
}

void Search::MoveToPoint(const til::point anchor) noexcept
{
if (_results.empty())
{
return;
}

const auto count = gsl::narrow_cast<ptrdiff_t>(_results.size());
ptrdiff_t index = 0;

if (_step < 0)
{
for (index = count - 1; index >= 0 && til::at(_results, index).start > anchor; --index)
{
}
}
else
{
for (index = 0; index < count && til::at(_results, index).start < anchor; ++index)
{
}
}

_index = (index + count) % count;
}
Comment on lines +44 to 68
Copy link
Member Author

Choose a reason for hiding this comment

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

This new function is a copy of the existing one below but with < instead of <=.


void Search::MovePastPoint(const til::point anchor) noexcept
Expand All @@ -58,18 +75,17 @@ void Search::MovePastPoint(const til::point anchor) noexcept
}

const auto count = gsl::narrow_cast<ptrdiff_t>(_results.size());
const auto highestIndex = count - 1;
auto index = _reverse ? highestIndex : 0;
ptrdiff_t index = 0;

if (_reverse)
if (_step < 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

I figured that if we don't invalidate on _reverse anymore we can use _step as the indicator for whether it's in reverse mode. That's why I slightly moved these variables around because it's now a bit neater.

{
for (; index >= 0 && til::at(_results, index).start >= anchor; --index)
for (index = count - 1; index >= 0 && til::at(_results, index).start >= anchor; --index)
{
}
}
else
{
for (; index <= highestIndex && til::at(_results, index).start <= anchor; ++index)
for (index = 0; index < count && til::at(_results, index).start <= anchor; ++index)
{
}
}
Expand Down Expand Up @@ -119,7 +135,7 @@ const std::vector<til::point_span>& Search::Results() const noexcept
return _results;
}

size_t Search::CurrentMatch() const noexcept
ptrdiff_t Search::CurrentMatch() const noexcept
Copy link
Member Author

Choose a reason for hiding this comment

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

_index is a ptrdiff_t.

{
return _index;
}
8 changes: 3 additions & 5 deletions src/buffer/out/search.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,23 @@ class Search final
public:
Search() = default;

bool ResetIfStale(Microsoft::Console::Render::IRenderData& renderData);
bool ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool reverse, bool caseInsensitive);

void MovePastCurrentSelection();
void MoveToCurrentSelection();
void MoveToPoint(til::point anchor) noexcept;
void MovePastPoint(til::point anchor) noexcept;
void FindNext() noexcept;

const til::point_span* GetCurrent() const noexcept;
bool SelectCurrent() const;

const std::vector<til::point_span>& Results() const noexcept;
size_t CurrentMatch() const noexcept;
bool CurrentDirection() const noexcept;
Copy link
Member Author

Choose a reason for hiding this comment

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

CurrentDirection() had no definition?

ptrdiff_t CurrentMatch() const noexcept;

private:
// _renderData is a pointer so that Search() is constexpr default constructable.
Microsoft::Console::Render::IRenderData* _renderData = nullptr;
std::wstring _needle;
bool _reverse = false;
bool _caseInsensitive = false;
uint64_t _lastMutationId = 0;

Expand Down
38 changes: 17 additions & 21 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1569,7 +1569,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation

if (_searcher.ResetIfStale(*GetRenderData(), text, !goForward, !caseSensitive))
{
_searcher.MovePastCurrentSelection();
_searcher.MoveToCurrentSelection();
_cachedSearchResultRows = {};
}
else
{
Expand Down Expand Up @@ -1600,12 +1601,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation

Windows::Foundation::Collections::IVector<int32_t> ControlCore::SearchResultRows()
{
auto lock = _terminal->LockForWriting();
if (_searcher.ResetIfStale(*GetRenderData()))
const auto lock = _terminal->LockForReading();

if (!_cachedSearchResultRows)
{
auto results = std::vector<int32_t>();

auto lastRow = til::CoordTypeMin;

for (const auto& match : _searcher.Results())
{
const auto row{ match.start.y };
Expand All @@ -1615,6 +1617,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
lastRow = row;
}
}

_cachedSearchResultRows = winrt::single_threaded_vector<int32_t>(std::move(results));
}

Expand Down Expand Up @@ -2245,27 +2248,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation

Windows::Foundation::Collections::IVector<Control::ScrollMark> ControlCore::ScrollMarks() const
{
auto internalMarks{ _terminal->GetScrollMarks() };
auto v = winrt::single_threaded_observable_vector<Control::ScrollMark>();
for (const auto& mark : internalMarks)
{
Control::ScrollMark m{};
const auto& internalMarks = _terminal->GetScrollMarks();
std::vector<Control::ScrollMark> v;

// sneaky: always evaluate the color of the mark to a real value
// before shoving it into the optional. If the mark doesn't have a
// specific color set, we'll use the value from the color table
// that's appropriate for this category of mark. If we do have a
// color set, then great we'll use that. The TermControl can then
// always use the value in the Mark regardless if it was actually
// set or not.
m.Color = OptionalFromColor(_terminal->GetColorForMark(mark));
m.Start = mark.start.to_core_point();
m.End = mark.end.to_core_point();
v.reserve(internalMarks.size());

v.Append(m);
for (const auto& mark : internalMarks)
{
v.emplace_back(
mark.start.to_core_point(),
mark.end.to_core_point(),
OptionalFromColor(_terminal->GetColorForMark(mark)));
}

return v;
return winrt::single_threaded_vector(std::move(v));
Comment on lines +2271 to +2284
Copy link
Member Author

@lhecker lhecker Sep 19, 2023

Choose a reason for hiding this comment

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

Adding this to the PR was a bit of a mistake (I did git restore from another branch). But it might be something we'd want to keep anyways - up to you all! 😅
The benefits are:

  • Much much faster vector creation (without WinRT indirection)
  • Reserve the right amount of memory
  • emplace_back constructs the trivial struct in-place instead of constructing it on the stack first and then copying it over to the heap

BTW I think this method is missing lock guards. Scroll marks are being written by the VT thread after all. I'm sure my other PR adds them. ^^ Merged the fix from main.

Copy link
Member

Choose a reason for hiding this comment

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

absolutely fine with me

}

void ControlCore::AddMark(const Control::ScrollMark& mark)
Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/win32/find.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ INT_PTR CALLBACK FindDialogProc(HWND hWnd, UINT Message, WPARAM wParam, LPARAM l

if (searcher.ResetIfStale(gci.renderData, lastFindString, reverse, caseInsensitive))
{
searcher.MovePastCurrentSelection();
searcher.MoveToCurrentSelection();
}
else
{
Expand Down
Loading