Avoid covering current search highlight with search box #17516
Conversation
microsoft-github-policy-service
bot
added
Issue-Task
| @@ -1751,7 +1751,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
| { | |||
| const auto lock = _terminal->LockForWriting(); | |||
| _terminal->SetSearchHighlights({}); | |||
| _terminal->SetSearchHighlightFocused({}); | |||
| _terminal->SetSearchHighlightFocused({}, _searchScrollOffset); | |||
There was a problem hiding this comment.
I suppose you can just pass 0 instead of _searchScrollOffset here?
|
(you need to do a tools\runformat.cmd` to fix the code formatter) |
| { | ||
| const auto displayInfo = DisplayInformation::GetForCurrentView(); | ||
| const auto scaleFactor = _core.FontSize().Height / displayInfo.RawPixelsPerViewPixel(); | ||
| const auto searchBoxRows = _searchBox->ActualHeight() / scaleFactor; |
There was a problem hiding this comment.
📝: is this ever null? Like, if we f3/findNext immediately before actually opening the search box?
There was a problem hiding this comment.
conclusion: yes it's null-initialized, but everywhere that uses this first checks that it's non-null, so it's fine
There was a problem hiding this comment.
updated, I ended up having to add the null check with the updates for only calculating when the font size or dpi changed, the calculate method was now being called in initialize when the _searchBox was null.
| @@ -3947,6 +3951,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
| SearchMissingCommand.raise(*this, args); | |||
| } | |||
|
|
|||
| til::CoordType TermControl::_searchScrollOffset() const | |||
There was a problem hiding this comment.
kinda seems weird that we have to recalculate this every time, even if the font size doesn't change.
I know it doesn't currently, but I suppose it makes enough sense that the search box might get taller. But the DPI and font size don't change frequently.
I feel like there's gotta be a way to just figure this out once per font size change (which I think also does happen on a dpi change) and just cache that value...
There was a problem hiding this comment.
updated to only be calculated in _coreFontSizeChanged. Testing locally it got called every time I changed the font size or the DPI. It looked like the scrolling also got updated afterwards by _coreOutputIdle
| @@ -1689,22 +1689,22 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
| // - resetOnly: If true, only Reset() will be called, if anything. FindNext() will never be called. | |||
| // Return Value: | |||
| // - <none> | |||
| SearchResults ControlCore::Search(const std::wstring_view& text, const bool goForward, const bool caseSensitive, const bool regularExpression, const bool resetOnly) | |||
| SearchResults ControlCore::Search(SearchRequest request) | |||
There was a problem hiding this comment.
i like this. This was starting to be too many args in too many places
|
Thanks so much for the fix! Sorry it took so long to review. |
|
Thanks! no complaints from me about the review time, I know you guys have priorities etc. |
Adds a scroll offset to avoid hiding the current search highlight with the search box. - Offset is based on the number of rows that the search box takes up. (I am not totally sure I am calculating this right) - This won't help when the current highlight is in the first couple rows of the buffer. Fixes: #4407 (cherry picked from commit 0bafab9) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgTTNTE Service-Version: 1.21
Summary of the Pull Request
Adds a scroll offset to avoid hiding the current search highlight with
the search box.
Fixes: #4407