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 some search highlights scenarios #17352

Merged
merged 4 commits into from
Jun 13, 2024
Merged

Fix some search highlights scenarios #17352

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
ba9a820
fix some search highlights scenarios
tusharsnx Jun 1, 2024
3d9d32b
format
tusharsnx Jun 1, 2024
931aed5
address feedbacks
tusharsnx Jun 5, 2024
d403470
ignore search calls from SB when it's closed/closing
tusharsnx Jun 5, 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
13 changes: 12 additions & 1 deletion src/cascadia/TerminalControl/SearchBoxControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// search box remains in Visible state (though not really *visible*) during the
// first load. So, we only need to apply this check here (after checking that
// we're done initializing).
if (Visibility() == Visibility::Visible)
if (IsOpen())
{
callback();
return;
}

// Stop ongoing close animation if any
if (CloseAnimation().GetCurrentState() == Media::Animation::ClockState::Active)
{
CloseAnimation().Stop();
}
Comment on lines +164 to +168
Copy link
Member

@lhecker lhecker Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understand how us previously ignoring the animation state would result in

Fast closing and re-opening SearchBox would leave search highlights in an inconsistent state.

If you know why, could you explain how that happens?

Copy link
Contributor Author

@tusharsnx tusharsnx Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When searchbox is in focus, if you press ESC and then quickly do a "find next/prev match" action, it will trigger this issue. The trick is to start a search when SB is closing. It's easier if you bind Find match action to a single key, eg. F3

Copy link
Contributor Author

@tusharsnx tusharsnx Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not to forget, highlights should be active (i.e. one or more search results available) when you're closing SB.


VisualStateManager::GoToState(*this, L"Opened", false);

// Call the callback only after we're in Opened state. Setting focus
Expand Down Expand Up @@ -196,6 +202,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

bool SearchBoxControl::IsOpen()
{
return Visibility() == Visibility::Visible && CloseAnimation().GetCurrentState() != Media::Animation::ClockState::Active;
}

winrt::hstring SearchBoxControl::Text()
{
return TextBox().Text();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/SearchBoxControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TextBoxKeyDown(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs& e);
void Open(std::function<void()> callback);
void Close();
bool IsOpen();

winrt::hstring Text();
bool GoForward();
Expand Down
45 changes: 37 additions & 8 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

if (_searchBox && _searchBox->Visibility() == Visibility::Visible)
if (_searchBox && _searchBox->IsOpen())
{
const auto core = winrt::get_self<ControlCore>(_core);
const auto& searchMatches = core->SearchResultRows();
Expand Down Expand Up @@ -538,6 +538,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// but since code paths differ, extra work is required to ensure correctness.
if (!_core.HasMultiLineSelection())
{
_core.SnapSearchResultToSelection(true);
const auto selectedLine{ _core.SelectedText(true) };
_searchBox->PopulateTextbox(selectedLine);
}
Expand All @@ -554,13 +555,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

// This is called when a Find Next/Previous Match action is triggered.
void TermControl::SearchMatch(const bool goForward)
{
if (_IsClosing())
{
return;
}
if (!_searchBox || _searchBox->Visibility() != Visibility::Visible)
if (!_searchBox || !_searchBox->IsOpen())
{
CreateSearchBoxControl();
}
Expand Down Expand Up @@ -598,11 +600,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const bool caseSensitive,
const bool regularExpression)
{
_handleSearchResults(_core.Search(text, goForward, caseSensitive, regularExpression, false));
if (_searchBox && _searchBox->IsOpen())
{
_handleSearchResults(_core.Search(text, goForward, caseSensitive, regularExpression, false));
}
}

// Method Description:
// - The handler for the "search criteria changed" event. Clears selection and initiates a new search.
// - The handler for the "search criteria changed" event. Initiates a new search.
// Arguments:
// - text: the text to search
// - goForward: indicates whether the search should be performed forward (if set to true) or backward
Expand All @@ -614,9 +619,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const bool caseSensitive,
const bool regularExpression)
{
if (_searchBox && _searchBox->Visibility() == Visibility::Visible)
if (_searchBox && _searchBox->IsOpen())
{
_handleSearchResults(_core.Search(text, goForward, caseSensitive, regularExpression, false));
// We only want to update the search results based on the new text. Set
// `resetOnly` to true so we don't accidentally update the current match index.
const auto result = _core.Search(text, goForward, caseSensitive, regularExpression, true);
_handleSearchResults(result);
}
}

Expand All @@ -634,6 +642,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_searchBox->Close();
_core.ClearSearch();

// Clear search highlights scroll marks (by triggering an update after closing the search box)
if (_showMarksInScrollbar)
{
const auto scrollBar = ScrollBar();
ScrollBarUpdate update{
.newValue = scrollBar.Value(),
.newMaximum = scrollBar.Maximum(),
.newMinimum = scrollBar.Minimum(),
.newViewportSize = scrollBar.ViewportSize(),
};
_updateScrollBar->Run(update);
}

// Set focus back to terminal control
this->Focus(FocusState::Programmatic);
}
Expand Down Expand Up @@ -3595,7 +3616,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void TermControl::_refreshSearch()
{
if (!_searchBox || _searchBox->Visibility() != Visibility::Visible)
if (!_searchBox || !_searchBox->IsOpen())
{
return;
}
Expand All @@ -3619,7 +3640,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return;
}

_searchBox->SetStatus(results.TotalMatches, results.CurrentMatch, results.SearchRegexInvalid);
// Only show status when we have a search term
if (_searchBox->Text().empty())
{
_searchBox->ClearStatus();
}
else
{
_searchBox->SetStatus(results.TotalMatches, results.CurrentMatch, results.SearchRegexInvalid);
}

if (results.SearchInvalidated)
{
Expand Down
Loading