Avoid covering current search highlight with search box#17516
Avoid covering current search highlight with search box#17516lhecker merged 4 commits intomicrosoft:mainfrom
Conversation
microsoft-github-policy-service
bot
added
Issue-Task
| 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.
| 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
| // 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. |
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
(reviewed after merging, love it!)
|
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 Fixes a bug where search would not scroll to results just below the viewport. This was caused by code intended to scroll the search result in such a way that it isn't covered by the search box. The scroll offset is calculated in `TermControl::_calculateSearchScrollOffset()` then handed down in the `SearchRequest` when conducting a search. This would get to `Terminal::ScrollToSearchHighlight()` where the offset is applied to the search result's position so that we would scroll to the adjusted position. The adjustment was overly aggressive in that it would apply it to both "start" and "end". In reality, we don't need to apply it to "end" because it wouldn't be covered by the search box (we only scroll to end if it's past the end of the current view anyways). The fix applies the adjustment only to "start" and only does so if it's actually in the first few rows that would be covered by the search box. That unveiled another bug where `Terminal::_ScrollToPoints()` would also be too aggressive about scrolling the "end" into view. In some testing, it would generally end up scrolling to the end of the buffer. To fix this cascading bug, I just had `_ScrollToPoints()` just call `Terminal::_ScrollToPoint()` (singular, not plural) which is consistently used throughout the Terminal code for selection (so it's battle tested). `_ScrollToPoints()` was kept since it's still used for accessibility when selecting a new region to keep the new selection in view. It's also just a nice wrapper that ensures a range is visible (or at least as much as it could be). ## References and Relevant Issues Scroll offset was added in #17516 ## Validation Steps Performed ✅ search results that would be covered by the search box are still adjusted ✅ search results that are past the end of the view become visible ✅ UIA still selects properly and brings the selection into view ## PR Checklist Duncan reported this bug internally, but there doesn't seem to be one on the repo.
5 participants
Summary of the Pull Request
Adds a scroll offset to avoid hiding the current search highlight with
the search box.
Fixes: #4407