Add support for regex search to conhost and Terminal#17316
Conversation
This comment has been minimized.
This comment has been minimized.
8803b4c to
63bbcdb
Compare
This comment has been minimized.
This comment has been minimized.
|
y draft |
honestly, I don't remember! Something about it made me sad. Perhaps it was the error communication. |
carlos-zamora
left a comment
There was a problem hiding this comment.
Looks great! A few small comments I'm sure you'll get to, so I'm going ahead and approving now.
| EDITTEXT ID_CONSOLE_FINDSTR, 47, 7, 128, 12, WS_GROUP | WS_TABSTOP | ES_AUTOHSCROLL | ||
|
|
||
| AUTOCHECKBOX "Match &case", ID_CONSOLE_FINDCASE, 4, 42, 64, 12 | ||
| AUTOCHECKBOX "Regula&r expression", ID_CONSOLE_FINDREGEX, 4, 28, 96, 12 |
There was a problem hiding this comment.
Curious, any reason why the & isn't before the "R" like &Regular expression?
There was a problem hiding this comment.
Yup! I had it there, but then it meant I you had to press alt+SHIFT+r to activate it! Because naturally it's a capital R. Seriously.
There was a problem hiding this comment.
That doesn't make sense, we always use capital letters with accelerators (in brackets) in CJK localization. Like "正则表达式 (&R)", and we never have to press shift to use those. 👀
There was a problem hiding this comment.
Yeah, that was surprising to me as well. I tried it, it did not work, so I moved to the nearest lowercase r. I may have had my focus in the wrong field when I did try it, but ... either way, it's one of the r keys now. 🙂
src/buffer/out/textBuffer.cpp
Outdated
| // Searches through the given rows [rowBeg,rowEnd) for `needle` and returns the coordinates in absolute coordinates. | ||
| // While the end coordinates of the returned ranges are considered inclusive, the [rowBeg,rowEnd) range is half-open. | ||
| std::vector<til::point_span> TextBuffer::SearchText(const std::wstring_view& needle, SearchFlag flags, til::CoordType rowBeg, til::CoordType rowEnd) const | ||
| bool TextBuffer::SearchText(const std::wstring_view& needle, SearchFlag flags, til::CoordType rowBeg, til::CoordType rowEnd, std::vector<til::point_span>& results) const |
There was a problem hiding this comment.
The method description could use a minor update. I'd particularly like an added description for what it means for this function to return false. Is it only if the provided regex was invalid?
| return _index; | ||
| } | ||
|
|
||
| bool Search::IsOk() const noexcept |
There was a problem hiding this comment.
| bool Search::IsOk() const noexcept | |
| bool Search::IsSearchInvalid() const noexcept |
maybe? Just something a bit more specific would be nice imo, because search->IsOk() is kinda ambiguous out-of-context.
src/buffer/out/textBuffer.cpp
Outdated
| UErrorCode status = U_ZERO_ERROR; | ||
| const auto re = ICU::CreateRegex(needle, flags, &status); | ||
| const auto re = ICU::CreateRegex(needle, icuFlags, &status); | ||
| if (status != U_ZERO_ERROR) |
There was a problem hiding this comment.
Negative statuses denote warnings or infos. You should use status > U_ZERO_ERROR here.
src/buffer/out/textBuffer.cpp
Outdated
| // Searches through the given rows [rowBeg,rowEnd) for `needle` and returns the coordinates in absolute coordinates. | ||
| // While the end coordinates of the returned ranges are considered inclusive, the [rowBeg,rowEnd) range is half-open. | ||
| std::vector<til::point_span> TextBuffer::SearchText(const std::wstring_view& needle, bool caseInsensitive, til::CoordType rowBeg, til::CoordType rowEnd) const | ||
| bool TextBuffer::SearchText(const std::wstring_view& needle, SearchFlag flags, til::CoordType rowBeg, til::CoordType rowEnd, std::vector<til::point_span>& results) const |
There was a problem hiding this comment.
nit: Instead of using results as an out parameter you could consider returnining a std::optional<std::vector<...>>, similar in spirit to a std::expected in the future (C++23).
It could then be split up here via something like
auto results = textBuffer.SearchText(needle, _flags, _results);
_ok = results.has_value();
_results = std::move(results).value_or({}); // std::optional has a && (move) overloadThis would also potentially work well in other places like so:
const auto results = textBuffer.SearchText(needle, _flags, _results).value_or({});63bbcdb to
ff63719
Compare
ff63719 to
e5314bf
Compare
If this flag is set, we will allow ICU to treat the needle as a regex
This is broken down into individual reviewable commits. [Here is](https://github.com/microsoft/terminal/assets/189190/3b2ffd50-1350-4f3c-86b0-75abbd846969) a video of it in action! Part of #3920 (cherry picked from commit ecb5631) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgTTM0g Service-Version: 1.21

This is broken down into individual reviewable commits.
Here is a video of it in action!
Part of #3920