Introduce search status #8588
Introduce search status #8588
Conversation
ghost
added
Area-Accessibility
|
@DHowett - this is just preliminary submission, but please review (especially the performance part) to know if the direction is OK |
|
@Don-Vito I haven't forgotten, so I'm going to get to this tomorrow. Thanks for being patient 😄 |
|
Okay so I'm just playing with this in debug - haven't looked at the code too carefully. It seems like leaving the find dialog open while running something like I wonder, are we doing the search on the UI thread? maybe there's a way to dispatch the searches to a background thread, and then call back to the foregroud thread with the results? |
|
@zadjii-msft - yeah, we definitely need to run it asynchronously. I mean - search few results immediately and the rest in background (to some limit). Can you share your test setup? |
|
@zadjii-msft, @DHowett - I am converting this to a draft to do the first set of optimizations, as the current version seems to fail the basic performance test @zadjii-msft did 😄 |
|
I mean, my test setup was literally just "checkout the branch, play with it". I wasn't doing any quantitative measuring, just qualitative.
Hopefully in that gif you can see a bit of what I was hitting. The scrolling absolutely hangs every so often as it processes the search. I typed "asd" pretty quickly after opening the dialog. |
|
@zadjii-msft - yeah. I just tried to understand what was the search (aka how many matches). I started without background thread just to get a feedback on experience. I have a local branch with background thread, that does much better, but still not sufficiently performant IMHO. I plan a bunch of optimizations, including cancelling jobs, limiting the pre-search, etc. Hope to get to it next week. |
|
@DHowett, @zadjii-msft - I did some optimizations to prevent terminal from hanging:
I am not a great master of winrt co-routines, so I believe I will need your further advice (e.g., right now I am doing a lot of switches between background and foreground, but the alternative is synchronization, and I am not sure what is better). |
|
Okay so this definitely feels better, even in Debug. I think the main weirdness I'm still seeing is what happens when more text is output to the buffer. While there's output, it just displays "Searching...", and it seems like the buttons are disabled. If you stop the output (like with Maybe in that case it should just not display anything there? Or something like I suppose the "searching doesn't seem to work while text is being output" issue is fine for now, since it's better than the current behavior. Trying to compare to a preview build - the terminal just crashed on me entirely while trying to search while scrolling 🤣 I'll take a spin though the code later, but that's my thoughts from a purely qualitative perspective |
What you describe here - is what I exactly planned as expected product behavior 😆
|
|
It's been almost a week - so scheduled bump 😊 |
|
@DHowett, @zadjii-msft - Disclaimer before you read the bellow: I really love the team, the technology and the product (so please don't hate me 👼) You are working on a huge stuff nowadays (RPC, buffer rewrite, settings UI, state storage, and so much more) and I really don't want to delay you, but I feel somewhat frustrated lately and want to share it. My major issue is that after a week of inactivity, PR goes off my L1 cache and restoring it there requires an effort. I tried different approaches:
Don't get me wrong - I know that you are putting a lot of effort in reviewing my commits and in majority of cases your feedback is very timely. I totally value that! And yet, even few PRs that are stuck / having no assignee / reviewer spoil the impression as it feels as irrational use of time. I still continue contributing, as I really believe in you and the product I use every day, but this frustration persists and I haven't found a cure myself yet 😊 I absolutely understand the challenge you are facing as a small team under MSFT's quality requirements and I am not coming with a specific suggestion of how and whether to solve this, but just wanted to raise a flag that if a contributor needs to wait a week for a follow up / several weeks to close all required cycles, this can make an establishment of robust community challenging. Another disclaimer: This above is probably my personal issue and other contributors don't feel so. In addition, I know I asked this is in the past, but if there are specific areas that can make my contributions more impactful (increasing the ROI of more frequent reviews) please let me know. I tried to solve P1s or tickets with many up-votes - but empirically it hardly increases the odds 😊 Sorry if it was emotional - simply we are reaching another week-end where I don't know how to advance.. |
|
Hey @Don-Vito. I'm really sorry that we've frustrated you like this. You put together some amazing PRs, and so consistently, and you deserve better than that. I could try and give excuses for taking so long (SUI sprint, baby not sleeping, attempted overthrow of democracy), but at the end of the day, this is on me. I dropped the ball by not prioritizing reivews fast enough, and I'm sorry for that. Trying to clear out the PR backlog has been a personal quest of mine for a while. Since coming back from the holidays, I've been more distracted and just haven't given it the time it needs. I'll do better. I wanted to get the team to give me a second qualitative opinion on this particular PR during our team sync this week, but that got cancelled, (and now that I think on it, next week's needs to be moved too, @DHowett). I'll summon the rest of the team this morning to see if they can give some feedback (when they wake up). Broader though, I'll try and be better about tapping other members of the team in on community PRs, and faster, so in the future you're not blocked on just mine and Dustin's time. As far as PR scope goes, just keep doing what you're doing honestly. Big ones like this, like the startup actions, like some of the cmdpal work, they're all really impressive and end up moving the needle in big jumps. The small fixes are still great too! The selection dismissing for example - that's a great example of something quick that just needed someone's eyes to find the right place to fix it. In my (very specific) opinion, the best thing you can do to get more timely reviews is find someway to make my baby sleep 😆. Seriously though, there's a lot of work to in the search dialog, and this PR is a great example of making that experience a lot better. Now it's on us to make sure that your experience as a contributor is better. Thanks for sharing. We'll do better 😊 |
| std::vector<std::pair<COORD, COORD>> matches; | ||
| if (!_searchState.value().Text.empty()) | ||
| { | ||
| const Search::Sensitivity sensitivity = _searchState.value().CaseSensitive ? |
There was a problem hiding this comment.
Should SearchState just store a Search::Sensitivity instead of a bool? </nit>
| Search::Sensitivity::CaseInsensitive; | ||
|
|
||
| Search search(*GetUiaData(), _searchState.value().Text.c_str(), Search::Direction::Forward, sensitivity); | ||
| while (co_await _SearchOne(search)) |
There was a problem hiding this comment.
/note this while loop collects up all the search matches, on a BG thread. If a new search is started, then it stops collecting matches
There was a problem hiding this comment.
oh but _SearchOne returns to the FG thread. Hmm. Does it need to do that? Could we do the whole search on one pass on the BG thread, then hop back to finish the UI bits?
There was a problem hiding this comment.
@zadjii-msft - yeah.. I switched to the foreground thread to avoid synchronization. An alternative is to do everything in BG and synchronize the state
| co_return; | ||
| } | ||
| } | ||
| _searchState.value().Matches.emplace(matches); |
There was a problem hiding this comment.
do we need to like, std::move the matches? I never remember with these STL containers.
| found = search.FindNext(); | ||
| } | ||
|
|
||
| co_await winrt::resume_foreground(Dispatcher()); |
There was a problem hiding this comment.
I'm mildly worried that this means for a buffer with 1000 matches, we'll need to hop back and forth between the UI thread and a BG thread 1000 times before we can display the results.
It seems fine qualitatively, but still...
There was a problem hiding this comment.
Hmm. We should be able to return the match on whatever queue, and the caller can be in charge of moving to the appropriate queue when it's necessary.
| @@ -269,6 +435,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation | |||
| void TermControl::_CloseSearchBoxControl(const winrt::Windows::Foundation::IInspectable& /*sender*/, RoutedEventArgs const& /*args*/) | |||
| { | |||
| _searchBox->Visibility(Visibility::Collapsed); | |||
| _searchState.reset(); | |||
There was a problem hiding this comment.
This is absolutely a future brainstorm, so feel free to ignore me.
But it might be neat to be able to set up a search string, then close the dialog, then like, hit F3 to just perform a new search and hop to the search result. So you could search once, close the dialog, then do something in the terminal, and quickly repeat the search, if that makes sense.
That would involve a lot less "check if the search box is open" code. And here, we'd just keep the search state around. Though, that would mean we'd keep searching always on output, which is maybe not what's expected. We'd need to not update the search results until the action was triggered.
</brainstorm>
There was a problem hiding this comment.
Yeah.. I see your point. This can be a cool optimization to preserve the search results between dialog usages, but I guess we will still need to know if there is an active search or not to avoid being triggered on output.
Probably should still introduce as a follow-up
@zadjii-msft - thanks for the kind words! And you absolutely should not be sorry - my feeling are subjective and I never shared them till now! 😊 Moreover, everything is a matter of expectations, this is my first open source project and I am still calibrating 😊 In any case, there's no urgency to cleanup the backlog right now - I was addressing the future work, and not just for me but probably for other contributors as well (which might feel similar). The Terminal community is great with tons of ideas and reports, but I do see only several robust external code contributors (and j4james who is not a contributor but a code monster). I believe that the project deserves a bigger code contributing community: after all this is the product we use every day, the standards are great and there are enough low hanging fruits to start contributing (though I still don't know how to approach some of your "Easy Starters"). At least for me the major challenge is around interaction. I know that you have SMEs, thus I am not sure if it is applicable, but making more devs engaged in PRs can help. In addition, even if there is no PR capacity, setting an assignee or giving some ack / keep-alive can improve the clarity... and I am already starting to suggest solutions, though I promised not to 😄 Finally, regarding the baby who refuses to sleep - my suggestion is to have a second baby. Usually this leads to two babies who do not sleep, but.. you stop caring about that anymore 😄 |
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
DHowett
removed
the
AutoMerge
There was a problem hiding this comment.
This is very clever, and I like it. It looks like it won't harm performance, but I do think that some caution is called-for around all the thread hopping.
@Don-Vito: thanks for always cutting through our layers of crap (both organizationally and code-wise) and getting good shit done 😄
| // - the size in pixels | ||
| double SearchBoxControl::_TextWidth(winrt::hstring text, double fontSize) | ||
| { | ||
| auto t = winrt::Windows::UI::Xaml::Controls::TextBlock(); |
There was a problem hiding this comment.
nit: whenever you find yourself writing auto f = type();, just prefer type f; -- it's shorter and has a better information locality!
| if (requiredWidth > StatusBox().Width()) | ||
| { | ||
| StatusBox().Width(requiredWidth); | ||
| } |
There was a problem hiding this comment.
This seems like a complicated version of MaxWidth. Any opportunity for simplification?
| @@ -181,7 +189,8 @@ | |||
|
|
|||
| <ToggleButton x:Name="CaseSensitivityButton" | |||
| x:Uid="SearchBox_CaseSensitivity" | |||
| Style="{StaticResource ToggleButtonStyle}"> | |||
| Style="{StaticResource ToggleButtonStyle}" | |||
| Click="CaseSensitivityButtonClicked"> | |||
There was a problem hiding this comment.
does this also trigger for keyboard activation? is there another event on ToggleButton that we can listen to for it being toggled?
| const winrt::hstring Text; | ||
| const bool CaseSensitive; | ||
| const size_t SearchId; |
There was a problem hiding this comment.
nit: we should enforce constness on members through constness on the SearchState itself. Class members who are themselves const are "special" (not copyable, not really moveable if I recall?)
| Search::Sensitivity::CaseSensitive : | ||
| Search::Sensitivity::CaseInsensitive; | ||
| // If no matches were computed it means we need to perform the search | ||
| if (!_searchState.value().Matches.has_value()) |
There was a problem hiding this comment.
nit: for conciseness, once you're sure that _searchState has a value, you can use _searchState-> instead of _searchState.value().
| if (_searchBox) | ||
| { | ||
| _searchBox->SetStatus(gsl::narrow<int32_t>(matches.size()), state.CurrentMatchIndex); | ||
| _searchBox->SetNavigationEnabled(!_searchState.value().Matches.value().empty()); |
There was a problem hiding this comment.
Reading this, I got the feeling "value value value value value"
| { | ||
| // We don't lock the terminal for the duration of the entire search, | ||
| // since if the terminal was modified the search ID will be updated. | ||
| auto lock = _terminal->LockForWriting(); |
There was a problem hiding this comment.
Does this need a write lock? or just a read lock
| found = search.FindNext(); | ||
| } | ||
|
|
||
| co_await winrt::resume_foreground(Dispatcher()); |
There was a problem hiding this comment.
Hmm. We should be able to return the match on whatever queue, and the caller can be in charge of moving to the appropriate queue when it's necessary.
|
|
||
| const SearchState searchState{ text, caseSensitive }; | ||
| _searchState.emplace(searchState); | ||
| _SearchAsync(goForward, SearchAfterChangeDelay); |
There was a problem hiding this comment.
Careful- this is directly hooked up to the search textbox change handler, right? It will enqueue one search for every character typed, and then all of them will wait delay and then abort due to the IDs not matching. Ideally, we'd put this in a throttled function as well, so that it only gets invoked once per delay rather than invoking N times and noping out after delay.
ghost
added
the
Needs-Author-Feedback
Thanks! I want to be super cautious here - as the potential damage is... nice - and I am absolutely not in hurry to commit this one before we are fully confident! Just want to make sure we keep working on it - as it gets bigger and I am quite senile 😊 I any case I found another concerning case:
|
ghost
removed
the
Needs-Author-Feedback
|
(Not sure where the next action item is here -- @Don-Vito, is it on me to review again, or were there additional concerns that warranted it staying in Draft status?) |
|
@DHowett - it's on me, sorry. I took a break from it, hope to return to it soon! |
ghost
dismissed
This is a resurrection of #8588. That PR became painfully stale after the `ControlCore` split. Original description: > ## Summary of the Pull Request > This is a PoC for: > * Search status in SearchBox (aka number of matches + index of the current match) > * Live search (aka search upon typing) > ## Detailed Description of the Pull Request / Additional comments > * Introduced this optionally (global setting to enable it) > * The approach is following: > * Every time the filter changes, enumerate all matches > * Upon navigation just take the relevant match and select it > I cleaned it up a bit, and added support for also displaying the positions of the matches in the scrollbar (if `showMarksOnScrollbar` is also turned on). It's also been made SUBSTANTIALLY easier after #15858 was merged. Similar to before, searching while there's piles of output running isn't _perfect_. But it's pretty awful currently, so that's not the end of the world. Gifs below. * closes #8631 (which is a bullet point in #3920) * closes #6319 Co-authored-by: Don-Vito <[email protected]> --------- Co-authored-by: khvitaly <[email protected]>
|
Oh hey, we resurrected this over to #14045 and merged it in like, 1.19. Thanks again for the contribution here! |
Summary of the Pull Request
This is a PoC for:
Highlighting all matches(probably should try later on separately)PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed