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

Jump to bottom

Fix search highlights during reflow #17092

Merged
merged 6 commits into from
Apr 23, 2024
Merged

Fix search highlights during reflow #17092

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea40774
Fix search highlights during reflow
lhecker Apr 20, 2024
1df0efa
Fix build
lhecker Apr 20, 2024
c7f2ae3
Remove unused declaration
lhecker Apr 20, 2024
435f66b
Revert accidental change
lhecker Apr 20, 2024
070edeb
Merge remote-tracking branch 'origin/main' into dev/lhecker/17073-sea…
lhecker Apr 23, 2024
00073e4
Address feedback
lhecker Apr 23, 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
14 changes: 8 additions & 6 deletions src/buffer/out/search.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,25 @@ bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, c
const auto& textBuffer = renderData.GetTextBuffer();
const auto lastMutationId = textBuffer.GetLastMutationId();

if (_needle == needle &&
if (_renderData == &renderData &&
_needle == needle &&
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 made this change because I grew a little worried about the robustness of the _lastMutationId == lastMutationId check.

_caseInsensitive == caseInsensitive &&
_lastMutationId == lastMutationId)
{
_step = reverse ? -1 : 1;
return false;
}

if (prevResults)
{
*prevResults = std::move(_results);
}

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

if (prevResults)
{
*prevResults = std::move(_results);
}
_results = textBuffer.SearchText(needle, caseInsensitive);
_index = reverse ? gsl::narrow_cast<ptrdiff_t>(_results.size()) - 1 : 0;
_step = reverse ? -1 : 1;
Expand Down Expand Up @@ -142,4 +144,4 @@ const std::vector<til::point_span>& Search::Results() const noexcept
ptrdiff_t Search::CurrentMatch() const noexcept
{
return _index;
}
}
130 changes: 55 additions & 75 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,6 @@ using namespace winrt::Windows::Graphics::Display;
using namespace winrt::Windows::System;
using namespace winrt::Windows::ApplicationModel::DataTransfer;

// The minimum delay between updates to the scroll bar's values.
// The updates are throttled to limit power usage.
constexpr const auto ScrollBarUpdateInterval = std::chrono::milliseconds(8);

// The minimum delay between updating the TSF input control.
constexpr const auto TsfRedrawInterval = std::chrono::milliseconds(100);

// The minimum delay between updating the locations of regex patterns
constexpr const auto UpdatePatternLocationsInterval = std::chrono::milliseconds(500);

// The delay before performing the search after change of search criteria
constexpr const auto SearchAfterChangeDelay = std::chrono::milliseconds(200);

namespace winrt::Microsoft::Terminal::Control::implementation
{
static winrt::Microsoft::Terminal::Core::OptionalColor OptionalFromColor(const til::color& c) noexcept
Expand Down Expand Up @@ -117,9 +104,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto pfnShowWindowChanged = std::bind(&ControlCore::_terminalShowWindowChanged, this, std::placeholders::_1);
_terminal->SetShowWindowCallback(pfnShowWindowChanged);

auto pfnTextLayoutUpdated = std::bind(&ControlCore::_terminalTextLayoutUpdated, this);
_terminal->SetTextLayoutUpdatedCallback(pfnTextLayoutUpdated);

auto pfnPlayMidiNote = std::bind(&ControlCore::_terminalPlayMidiNote, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3);
_terminal->SetPlayMidiNoteCallback(pfnPlayMidiNote);

Expand Down Expand Up @@ -167,31 +151,33 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_dispatcher = controller.DispatcherQueue();
}

// A few different events should be throttled, so they don't fire absolutely all the time:
// * _updatePatternLocations: When there's new output, or we scroll the
// viewport, we should re-check if there are any visible hyperlinks.
// But we don't really need to do this every single time text is
// output, we can limit this update to once every 500ms.
// * _updateScrollBar: Same idea as the TSF update - we don't _really_
// need to hop across the process boundary every time text is output.
// We can throttle this to once every 8ms, which will get us out of
// the way of the main output & rendering threads.
const auto shared = _shared.lock();
// Raises an OutputIdle event once there hasn't been any output for at least 100ms.
// It also updates all regex patterns in the viewport.
//
// NOTE: Calling UpdatePatternLocations from a background
// thread is a workaround for us to hit GH#12607 less often.
shared->updatePatternLocations = std::make_unique<til::throttled_func_trailing<>>(
UpdatePatternLocationsInterval,
[weakTerminal = std::weak_ptr{ _terminal }]() {
shared->outputIdle = std::make_unique<til::debounced_func_trailing<>>(
std::chrono::milliseconds{ 100 },
[weakTerminal = std::weak_ptr{ _terminal }, weakThis = get_weak(), dispatcher = _dispatcher]() {
dispatcher.TryEnqueue(DispatcherQueuePriority::Normal, [weakThis]() {
if (const auto self = weakThis.get(); !self->_IsClosing())
{
self->OutputIdle.raise(*self, nullptr);
}
});
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 can imagine that this debounced event may be quite useful for other things in the future.


if (const auto t = weakTerminal.lock())
{
const auto lock = t->LockForWriting();
t->UpdatePatternsUnderLock();
}
});

// Scrollbar updates are also expensive (XAML), so we'll throttle them as well.
shared->updateScrollBar = std::make_shared<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>>(
_dispatcher,
ScrollBarUpdateInterval,
std::chrono::milliseconds{ 8 },
[weakThis = get_weak()](const auto& update) {
if (auto core{ weakThis.get() }; !core->_IsClosing())
{
Expand All @@ -218,7 +204,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// thread. These will be recreated in _setupDispatcherAndCallbacks, when
// we're re-attached to a new control (on a possibly new UI thread).
const auto shared = _shared.lock();
shared->updatePatternLocations.reset();
shared->outputIdle.reset();
shared->updateScrollBar.reset();
}

Expand Down Expand Up @@ -671,9 +657,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

const auto shared = _shared.lock_shared();
if (shared->updatePatternLocations)
if (shared->outputIdle)
{
(*shared->updatePatternLocations)();
(*shared->outputIdle)();
}
}

Expand Down Expand Up @@ -1104,12 +1090,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// If this function succeeds with S_FALSE, then the terminal didn't
// actually change size. No need to notify the connection of this no-op.
const auto hr = _terminal->UserResize({ vp.Width(), vp.Height() });
if (SUCCEEDED(hr) && hr != S_FALSE)
if (FAILED(hr) || hr == S_FALSE)
{
_connection.Resize(vp.Height(), vp.Width());
return;
}

_connection.Resize(vp.Height(), vp.Width());

// let the UI know that the text layout has been updated
_terminal->NotifyTextLayoutUpdated();
// TermControl will call Search() once the OutputIdle even fires after 100ms.
// Until then we need to hide the now-stale search results from the renderer.
ClearSearch();
const auto shared = _shared.lock_shared();
if (shared->outputIdle)
{
(*shared->outputIdle)();
}
}

Expand Down Expand Up @@ -1607,16 +1601,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
ShowWindowChanged.raise(*this, *showWindow);
}

void ControlCore::_terminalTextLayoutUpdated()
{
ClearSearch();

// send an UpdateSearchResults event to the UI to put the Search UI into inactive state.
auto evArgs = winrt::make_self<implementation::UpdateSearchResultsEventArgs>();
evArgs->State(SearchState::Inactive);
UpdateSearchResults.raise(*this, *evArgs);
}

// Method Description:
// - Plays a single MIDI note, blocking for the duration.
// Arguments:
Expand Down Expand Up @@ -1676,13 +1660,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - caseSensitive: boolean that represents if the current search is case sensitive
// Return Value:
// - <none>
void ControlCore::Search(const winrt::hstring& text, const bool goForward, const bool caseSensitive)
SearchResults ControlCore::Search(const std::wstring_view& text, const bool goForward, const bool caseSensitive, const bool reset)
{
const auto lock = _terminal->LockForWriting();

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

_cachedSearchResultRows = {};
if (SnapSearchResultToSelection())
{
Expand All @@ -1691,30 +1678,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

_terminal->SetSearchHighlights(_searcher.Results());
_terminal->SetSearchHighlightFocused(_searcher.CurrentMatch());
_renderer->TriggerSearchHighlight(oldResults);
Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can tell, we only need to call this function if the search got reset so I moved it inside this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

This results in focused search highlight to be not invalidated even though it might change in the below else block.

I know it sounds inefficient to invalidate all highlights when just the focused one is changed, but to fix this inefficiency I guess you have to add TriggerSearchHighlightFocused() or something along those lines. And the Atlas engine also doesn't know which one got invalidated so it currently just re-draws both 🤕🥲

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. I think it's fine to have a bit of inefficiency here. I bet that the invalidation finishes in less than 1ms even if you had a million search hits - and we only refresh it 10 times per second at most.

If we get to the point where we have buffer snapshotting in the renderer, we can simply scribble the highlights into the snapshot directly. Then we simply check if the text or attributes of each row have changed compared to the last snapshot. (= We have 2 snapshots which we swap back and forth every other frame.) I'm pretty excited for that, as all those subtle issues like this one will just instantly vanish.

}
else
else if (!reset)
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 allows us to ensure that we don't accidentally call FindNext() just because the OutputIdle event got triggered, which results in a Search() call.

{
_searcher.FindNext();
_terminal->SetSearchHighlightFocused(_searcher.CurrentMatch());
}
_renderer->TriggerSearchHighlight(oldResults);

auto evArgs = winrt::make_self<implementation::UpdateSearchResultsEventArgs>();
if (!text.empty())
int32_t totalMatches = 0;
int32_t currentMatch = 0;
if (const auto idx = _searcher.CurrentMatch(); idx >= 0)
{
evArgs->State(SearchState::Active);
if (_searcher.GetCurrent())
{
evArgs->FoundMatch(true);
evArgs->TotalMatches(gsl::narrow<int32_t>(_searcher.Results().size()));
evArgs->CurrentMatch(gsl::narrow<int32_t>(_searcher.CurrentMatch()));
}
totalMatches = gsl::narrow<int32_t>(_searcher.Results().size());
currentMatch = gsl::narrow<int32_t>(idx);
_terminal->SetSearchHighlightFocused(gsl::narrow<size_t>(idx));
}

// Raise an UpdateSearchResults event, which the control will use to update the
// UI and notify the narrator about the updated search results in the buffer
UpdateSearchResults.raise(*this, *evArgs);
return {
.TotalMatches = totalMatches,
.CurrentMatch = currentMatch,
.SearchInvalidated = searchInvalidated,
};
}

Windows::Foundation::Collections::IVector<int32_t> ControlCore::SearchResultRows()
Expand Down Expand Up @@ -1742,16 +1726,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void ControlCore::ClearSearch()
{
// nothing to clear if there's no results
if (_searcher.GetCurrent())
{
const auto lock = _terminal->LockForWriting();
_terminal->SetSearchHighlights({});
_terminal->SetSearchHighlightFocused({});
_renderer->TriggerSearchHighlight(_searcher.Results());
_searcher = {};
_cachedSearchResultRows = {};
}
const auto lock = _terminal->LockForWriting();
_terminal->SetSearchHighlights({});
_terminal->SetSearchHighlightFocused({});
_renderer->TriggerSearchHighlight(_searcher.Results());
_searcher = {};
_cachedSearchResultRows = {};
Comment on lines +1727 to +1731
Copy link
Member Author

Choose a reason for hiding this comment

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

We can't check _searcher.GetCurrent() here because otherwise we don't reset the _lastMutationId of a Searcher that found nothing. This is relevant in case you open the search box (the Ctrl+Shift+F one), enter something that doesn't exist, close it, and reopen it. If it gets closed and doesn't get reset here, then reopening it won't trigger a search otherwise.

}

// Method Description:
Expand Down Expand Up @@ -2134,9 +2114,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// Start the throttled update of where our hyperlinks are.
const auto shared = _shared.lock_shared();
if (shared->updatePatternLocations)
if (shared->outputIdle)
{
(*shared->updatePatternLocations)();
(*shared->outputIdle)();
}
}
catch (...)
Expand Down
8 changes: 3 additions & 5 deletions src/cascadia/TerminalControl/ControlCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void SetSelectionAnchor(const til::point position);
void SetEndSelectionPoint(const til::point position);

void Search(const winrt::hstring& text, const bool goForward, const bool caseSensitive);
SearchResults Search(const std::wstring_view& text, bool goForward, bool caseSensitive, bool reset);
void ClearSearch();
void SnapSearchResultToSelection(bool snap) noexcept;
bool SnapSearchResultToSelection() const noexcept;
Expand Down Expand Up @@ -279,8 +279,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
til::typed_event<IInspectable, Control::RendererWarningArgs> RendererWarning;
til::typed_event<IInspectable, Control::NoticeEventArgs> RaiseNotice;
til::typed_event<IInspectable, Control::TransparencyChangedEventArgs> TransparencyChanged;
til::typed_event<> ReceivedOutput;
til::typed_event<IInspectable, Control::UpdateSearchResultsEventArgs> UpdateSearchResults;
til::typed_event<> OutputIdle;
til::typed_event<IInspectable, Control::ShowWindowArgs> ShowWindowChanged;
til::typed_event<IInspectable, Control::UpdateSelectionMarkersEventArgs> UpdateSelectionMarkers;
til::typed_event<IInspectable, Control::OpenHyperlinkEventArgs> OpenHyperlink;
Expand All @@ -295,7 +294,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
struct SharedState
{
std::unique_ptr<til::throttled_func_trailing<>> updatePatternLocations;
std::unique_ptr<til::debounced_func_trailing<>> outputIdle;
std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar;
};

Expand Down Expand Up @@ -376,7 +375,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const int bufferSize);
void _terminalTaskbarProgressChanged();
void _terminalShowWindowChanged(bool showOrHide);
void _terminalTextLayoutUpdated();
void _terminalPlayMidiNote(const int noteNumber,
const int velocity,
const std::chrono::microseconds duration);
Expand Down
12 changes: 9 additions & 3 deletions src/cascadia/TerminalControl/ControlCore.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ namespace Microsoft.Terminal.Control
Boolean EndAtRightBoundary;
};

struct SearchResults
{
Int32 TotalMatches;
Int32 CurrentMatch;
Boolean SearchInvalidated;
};

[default_interface] runtimeclass SelectionColor
{
SelectionColor();
Expand Down Expand Up @@ -127,7 +134,7 @@ namespace Microsoft.Terminal.Control
void ResumeRendering();
void BlinkAttributeTick();

void Search(String text, Boolean goForward, Boolean caseSensitive);
SearchResults Search(String text, Boolean goForward, Boolean caseSensitive, Boolean reset);
void ClearSearch();
IVector<Int32> SearchResultRows { get; };
Boolean SnapSearchResultToSelection;
Expand Down Expand Up @@ -177,8 +184,7 @@ namespace Microsoft.Terminal.Control
event Windows.Foundation.TypedEventHandler<Object, RendererWarningArgs> RendererWarning;
event Windows.Foundation.TypedEventHandler<Object, NoticeEventArgs> RaiseNotice;
event Windows.Foundation.TypedEventHandler<Object, TransparencyChangedEventArgs> TransparencyChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> ReceivedOutput;
event Windows.Foundation.TypedEventHandler<Object, UpdateSearchResultsEventArgs> UpdateSearchResults;
event Windows.Foundation.TypedEventHandler<Object, Object> OutputIdle;
event Windows.Foundation.TypedEventHandler<Object, UpdateSelectionMarkersEventArgs> UpdateSelectionMarkers;
event Windows.Foundation.TypedEventHandler<Object, OpenHyperlinkEventArgs> OpenHyperlink;
event Windows.Foundation.TypedEventHandler<Object, Object> CloseTerminalRequested;
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalControl/EventArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "ScrollPositionChangedArgs.g.cpp"
#include "RendererWarningArgs.g.cpp"
#include "TransparencyChangedEventArgs.g.cpp"
#include "UpdateSearchResultsEventArgs.g.cpp"
#include "ShowWindowArgs.g.cpp"
#include "UpdateSelectionMarkersEventArgs.g.cpp"
#include "CompletionsChangedEventArgs.g.cpp"
Expand Down
12 changes: 0 additions & 12 deletions src/cascadia/TerminalControl/EventArgs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "ScrollPositionChangedArgs.g.h"
#include "RendererWarningArgs.g.h"
#include "TransparencyChangedEventArgs.g.h"
#include "UpdateSearchResultsEventArgs.g.h"
#include "ShowWindowArgs.g.h"
#include "UpdateSelectionMarkersEventArgs.g.h"
#include "CompletionsChangedEventArgs.g.h"
Expand Down Expand Up @@ -141,17 +140,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
WINRT_PROPERTY(double, Opacity);
};

struct UpdateSearchResultsEventArgs : public UpdateSearchResultsEventArgsT<UpdateSearchResultsEventArgs>
{
public:
UpdateSearchResultsEventArgs() = default;

WINRT_PROPERTY(SearchState, State, SearchState::Inactive);
WINRT_PROPERTY(bool, FoundMatch);
WINRT_PROPERTY(int32_t, TotalMatches);
WINRT_PROPERTY(int32_t, CurrentMatch);
};

struct ShowWindowArgs : public ShowWindowArgsT<ShowWindowArgs>
{
public:
Expand Down
8 changes: 0 additions & 8 deletions src/cascadia/TerminalControl/EventArgs.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,6 @@ namespace Microsoft.Terminal.Control
Active = 1,
};

runtimeclass UpdateSearchResultsEventArgs
{
SearchState State { get; };
Boolean FoundMatch { get; };
Int32 TotalMatches { get; };
Int32 CurrentMatch { get; };
}

runtimeclass ShowWindowArgs
{
Boolean ShowOrHide { get; };
Expand Down
Loading
Loading