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 search constantly triggering a scroll #17132

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Fix search constantly triggering a scroll #17132

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
162ff43
Fix search constantly triggering a scroll
lhecker Apr 25, 2024
a3b560b
Address feedback
lhecker Apr 25, 2024
430d116
Fix build
lhecker Apr 25, 2024
b605123
Fix tests
lhecker Apr 26, 2024
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
35 changes: 17 additions & 18 deletions src/buffer/out/search.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,22 @@

using namespace Microsoft::Console::Types;

bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool reverse, bool caseInsensitive, std::vector<til::point_span>* prevResults)
bool Search::IsStale(const Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool caseInsensitive) const noexcept
{
const auto& textBuffer = renderData.GetTextBuffer();
const auto lastMutationId = textBuffer.GetLastMutationId();

if (_renderData == &renderData &&
_needle == needle &&
_caseInsensitive == caseInsensitive &&
_lastMutationId == lastMutationId)
{
_step = reverse ? -1 : 1;
return false;
}
return _renderData != &renderData ||
_needle != needle ||
_caseInsensitive != caseInsensitive ||
_lastMutationId != renderData.GetTextBuffer().GetLastMutationId();
}

if (prevResults)
{
*prevResults = std::move(_results);
}
bool Search::Reset(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool caseInsensitive, bool reverse)
{
const auto& textBuffer = renderData.GetTextBuffer();

_renderData = &renderData;
_needle = needle;
_caseInsensitive = caseInsensitive;
_lastMutationId = lastMutationId;
_lastMutationId = textBuffer.GetLastMutationId();

_results = textBuffer.SearchText(needle, caseInsensitive);
_index = reverse ? gsl::narrow_cast<ptrdiff_t>(_results.size()) - 1 : 0;
Expand Down Expand Up @@ -98,8 +91,9 @@ void Search::MovePastPoint(const til::point anchor) noexcept
_index = (index + count) % count;
}

void Search::FindNext() noexcept
void Search::FindNext(bool reverse) noexcept
{
_step = reverse ? -1 : 1;
if (const auto count{ gsl::narrow_cast<ptrdiff_t>(_results.size()) })
{
_index = (_index + _step + count) % count;
Expand Down Expand Up @@ -141,6 +135,11 @@ const std::vector<til::point_span>& Search::Results() const noexcept
return _results;
}

std::vector<til::point_span>&& Search::ExtractResults() noexcept
{
return std::move(_results);
}

ptrdiff_t Search::CurrentMatch() const noexcept
{
return _index;
Expand Down
10 changes: 4 additions & 6 deletions src/buffer/out/search.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,19 @@ class Search final
public:
Search() = default;

bool ResetIfStale(Microsoft::Console::Render::IRenderData& renderData,
const std::wstring_view& needle,
bool reverse,
bool caseInsensitive,
std::vector<til::point_span>* prevResults = nullptr);
bool IsStale(const Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool caseInsensitive) const noexcept;
bool Reset(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool caseInsensitive, bool reverse);

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

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

const std::vector<til::point_span>& Results() const noexcept;
std::vector<til::point_span>&& ExtractResults() noexcept;
ptrdiff_t CurrentMatch() const noexcept;

private:
Expand Down
65 changes: 27 additions & 38 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1654,30 +1654,41 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - text: the text to search
// - goForward: boolean that represents if the current search direction is forward
// - caseSensitive: boolean that represents if the current search is case sensitive
// - resetOnly: If true, only Reset() will be called, if anything. FindNext() will never be called.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// - resetOnly: If true, only Reset() will be called, if anything. FindNext() will never be called.
// - resetOnly: If true, only Search::IsStale() will be called, if anything. FindNext() will never be called.

right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, actually the comment is correct, but the boolean logic inside the function is very difficult to read. Basically, if resetOnly is true then if (searchInvalidated || !resetOnly) is only entered if searchInvalidated is true. If searchInvalidated is true then only Reset() will be called.

Copy link
Member

Choose a reason for hiding this comment

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

oh yea that's hard to parse

// Return Value:
// - <none>
SearchResults ControlCore::Search(const std::wstring_view& text, const bool goForward, const bool caseSensitive, const bool reset)
SearchResults ControlCore::Search(const std::wstring_view& text, const bool goForward, const bool caseSensitive, const bool resetOnly)
{
const auto lock = _terminal->LockForWriting();
const auto searchInvalidated = _searcher.IsStale(*_terminal.get(), text, !caseSensitive);

bool searchInvalidated = false;
std::vector<til::point_span> oldResults;
if (_searcher.ResetIfStale(*GetRenderData(), text, !goForward, !caseSensitive, &oldResults))
if (searchInvalidated || !resetOnly)
{
searchInvalidated = true;
std::vector<til::point_span> oldResults;

_cachedSearchResultRows = {};
if (SnapSearchResultToSelection())
if (searchInvalidated)
{
_searcher.MoveToCurrentSelection();
SnapSearchResultToSelection(false);
oldResults = _searcher.ExtractResults();
_searcher.Reset(*_terminal.get(), text, !caseSensitive, !goForward);

if (SnapSearchResultToSelection())
{
_searcher.MoveToCurrentSelection();
SnapSearchResultToSelection(false);
}

_terminal->SetSearchHighlights(_searcher.Results());
}
else
{
_searcher.FindNext(!goForward);
}

_terminal->SetSearchHighlights(_searcher.Results());
}
else if (!reset)
{
_searcher.FindNext();
if (const auto idx = _searcher.CurrentMatch(); idx >= 0)
{
_terminal->SetSearchHighlightFocused(gsl::narrow<size_t>(idx));
}
_renderer->TriggerSearchHighlight(oldResults);
Copy link
Member

Choose a reason for hiding this comment

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

won't oldResults be empty if we aren't invalidating? i guess I don't understand

Copy link
Member Author

@lhecker lhecker Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You mean what happens if IsStale() returned false and we didn't call Reset()?
TriggerSearchHighlight is basically a "refresh all these areas" call. Normally we need to refresh all current areas, which is why oldResults is empty. It gets the current areas directly from IRenderData (previously set by SetSearchHighlights). During a reset oldResults contains the previous areas as well, since those aren't highlighted anymore = dirty.

Now that I think about it, I think we could also just remove the parameter to TriggerSearchHighlight and simply call it once before calling Reset() (= invalidate all the old highlights) and then once after (= all the new and current highlights).

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, I think we could also just remove the parameter to TriggerSearchHighlight and simply call it once before calling Reset() (= invalidate all the old highlights) and then once after (= all the new and current highlights).

Nice 😃

TriggerSearchHighlight() also isn't completely correct in that the old highlights are supposed to be invalidated w.r.t old line renditions, but we currently don't store old line renditions so we're just using the current line renditions. I'm sure this can be fixed with buffer snapshot based rendering where we would pass in the line rendition from the current snapshot of the buffer that render engine has access to.

}

int32_t totalMatches = 0;
Expand All @@ -1686,39 +1697,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
totalMatches = gsl::narrow<int32_t>(_searcher.Results().size());
currentMatch = gsl::narrow<int32_t>(idx);
_terminal->SetSearchHighlightFocused(gsl::narrow<size_t>(idx));
}

_renderer->TriggerSearchHighlight(oldResults);

return {
.TotalMatches = totalMatches,
.CurrentMatch = currentMatch,
.SearchInvalidated = searchInvalidated,
};
}

Windows::Foundation::Collections::IVector<int32_t> ControlCore::SearchResultRows()
const std::vector<til::point_span>& ControlCore::SearchResultRows() const noexcept
{
if (!_cachedSearchResultRows)
{
auto results = std::vector<int32_t>();
auto lastRow = til::CoordTypeMin;

for (const auto& match : _searcher.Results())
{
const auto row{ match.start.y };
if (row != lastRow)
{
results.push_back(row);
lastRow = row;
}
}

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

return _cachedSearchResultRows;
return _searcher.Results();
}

void ControlCore::ClearSearch()
Expand All @@ -1728,7 +1718,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_terminal->SetSearchHighlightFocused({});
_renderer->TriggerSearchHighlight(_searcher.Results());
_searcher = {};
_cachedSearchResultRows = {};
}

// Method Description:
Expand Down
5 changes: 1 addition & 4 deletions src/cascadia/TerminalControl/ControlCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void SetEndSelectionPoint(const til::point position);

SearchResults Search(const std::wstring_view& text, bool goForward, bool caseSensitive, bool reset);
const std::vector<til::point_span>& SearchResultRows() const noexcept;
void ClearSearch();
void SnapSearchResultToSelection(bool snap) noexcept;
bool SnapSearchResultToSelection() const noexcept;

Windows::Foundation::Collections::IVector<int32_t> SearchResultRows();

void LeftClickOnTerminal(const til::point terminalPosition,
const int numberOfClicks,
const bool altEnabled,
Expand Down Expand Up @@ -353,8 +352,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation

til::point _contextMenuBufferPosition{ 0, 0 };

Windows::Foundation::Collections::IVector<int32_t> _cachedSearchResultRows{ nullptr };

void _setupDispatcherAndCallbacks();

bool _setFontSizeUnderLock(float fontSize);
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalControl/ControlCore.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ namespace Microsoft.Terminal.Control

SearchResults Search(String text, Boolean goForward, Boolean caseSensitive, Boolean reset);
void ClearSearch();
IVector<Int32> SearchResultRows { get; };
Boolean SnapSearchResultToSelection;

Microsoft.Terminal.Core.Color ForegroundColor { get; };
Expand Down
16 changes: 10 additions & 6 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,14 +495,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation

if (_searchBox && _searchBox->Visibility() == Visibility::Visible)
{
if (const auto searchMatches = _core.SearchResultRows())
{
const til::color color{ _core.ForegroundColor() };
const auto rightAlignedOffset = (scrollBarWidthInPx - pipWidth) * sizeof(til::color);
const auto core = winrt::get_self<ControlCore>(_core);
const auto& searchMatches = core->SearchResultRows();
const auto color = core->ForegroundColor();
const auto rightAlignedOffset = (scrollBarWidthInPx - pipWidth) * sizeof(til::color);
til::CoordType lastRow = til::CoordTypeMin;

for (const auto row : searchMatches)
for (const auto& span : searchMatches)
{
if (lastRow != span.start.y)
{
const auto base = dataAt(row) + rightAlignedOffset;
lastRow = span.start.y;
const auto base = dataAt(lastRow) + rightAlignedOffset;
drawPip(base, color);
}
}
Expand Down
40 changes: 20 additions & 20 deletions src/host/ut_host/SearchTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class SearchTests
return true;
}

static void DoFoundChecks(Search& s, til::point coordStartExpected, til::CoordType lineDelta)
static void DoFoundChecks(Search& s, til::point coordStartExpected, til::CoordType lineDelta, bool reverse)
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

Expand All @@ -70,23 +70,23 @@ class SearchTests

coordStartExpected.y += lineDelta;
coordEndExpected.y += lineDelta;
s.FindNext();
s.FindNext(reverse);

VERIFY_IS_TRUE(s.SelectCurrent());
VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor());
VERIFY_ARE_EQUAL(coordEndExpected, gci.renderData.GetSelectionEnd());

coordStartExpected.y += lineDelta;
coordEndExpected.y += lineDelta;
s.FindNext();
s.FindNext(reverse);

VERIFY_IS_TRUE(s.SelectCurrent());
VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor());
VERIFY_ARE_EQUAL(coordEndExpected, gci.renderData.GetSelectionEnd());

coordStartExpected.y += lineDelta;
coordEndExpected.y += lineDelta;
s.FindNext();
s.FindNext(reverse);

VERIFY_IS_TRUE(s.SelectCurrent());
VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor());
Expand All @@ -98,64 +98,64 @@ class SearchTests
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

Search s;
s.ResetIfStale(gci.renderData, L"AB", false, false);
DoFoundChecks(s, {}, 1);
s.Reset(gci.renderData, L"AB", false, false);
DoFoundChecks(s, {}, 1, false);
}

TEST_METHOD(ForwardCaseSensitiveJapanese)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Search s;
s.ResetIfStale(gci.renderData, L"\x304b", false, false);
DoFoundChecks(s, { 2, 0 }, 1);
s.Reset(gci.renderData, L"\x304b", false, false);
DoFoundChecks(s, { 2, 0 }, 1, false);
}

TEST_METHOD(ForwardCaseInsensitive)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

Search s;
s.ResetIfStale(gci.renderData, L"ab", false, true);
DoFoundChecks(s, {}, 1);
s.Reset(gci.renderData, L"ab", true, false);
DoFoundChecks(s, {}, 1, false);
}

TEST_METHOD(ForwardCaseInsensitiveJapanese)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Search s;
s.ResetIfStale(gci.renderData, L"\x304b", false, true);
DoFoundChecks(s, { 2, 0 }, 1);
s.Reset(gci.renderData, L"\x304b", true, false);
DoFoundChecks(s, { 2, 0 }, 1, false);
}

TEST_METHOD(BackwardCaseSensitive)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Search s;
s.ResetIfStale(gci.renderData, L"AB", true, false);
DoFoundChecks(s, { 0, 3 }, -1);
s.Reset(gci.renderData, L"AB", false, true);
DoFoundChecks(s, { 0, 3 }, -1, true);
}

TEST_METHOD(BackwardCaseSensitiveJapanese)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Search s;
s.ResetIfStale(gci.renderData, L"\x304b", true, false);
DoFoundChecks(s, { 2, 3 }, -1);
s.Reset(gci.renderData, L"\x304b", false, true);
DoFoundChecks(s, { 2, 3 }, -1, true);
}

TEST_METHOD(BackwardCaseInsensitive)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Search s;
s.ResetIfStale(gci.renderData, L"ab", true, true);
DoFoundChecks(s, { 0, 3 }, -1);
s.Reset(gci.renderData, L"ab", true, true);
DoFoundChecks(s, { 0, 3 }, -1, true);
}

TEST_METHOD(BackwardCaseInsensitiveJapanese)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Search s;
s.ResetIfStale(gci.renderData, L"\x304b", true, true);
DoFoundChecks(s, { 2, 3 }, -1);
s.Reset(gci.renderData, L"\x304b", true, true);
DoFoundChecks(s, { 2, 3 }, -1, true);
}
};
5 changes: 3 additions & 2 deletions src/interactivity/win32/find.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,14 @@ INT_PTR CALLBACK FindDialogProc(HWND hWnd, UINT Message, WPARAM wParam, LPARAM l
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

if (searcher.ResetIfStale(gci.renderData, lastFindString, reverse, caseInsensitive))
if (searcher.IsStale(gci.renderData, lastFindString, caseInsensitive))
{
searcher.Reset(gci.renderData, lastFindString, caseInsensitive, reverse);
searcher.MoveToCurrentSelection();
}
else
{
searcher.FindNext();
searcher.FindNext(reverse);
}

if (searcher.SelectCurrent())
Expand Down
5 changes: 4 additions & 1 deletion src/types/UiaTextRangeBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,10 @@ try
// -> We need to turn [_beg,_end) into (_beg,_end).
exclusiveBegin.x--;

_searcher.ResetIfStale(*_pData, queryText, searchBackward, ignoreCase);
if (_searcher.IsStale(*_pData, queryText, ignoreCase))
{
_searcher.Reset(*_pData, queryText, ignoreCase, searchBackward);
}
_searcher.MovePastPoint(searchBackward ? _end : exclusiveBegin);

til::point hitBeg{ til::CoordTypeMax, til::CoordTypeMax };
Expand Down
Loading