Add support for regex search to conhost and Terminal #17316

DHowett · 2024-05-23T22:17:56Z

This is broken down into individual reviewable commits.

Here is a video of it in action!

Part of #3920

src/host/res.rc

src/host/resource.h

src/interactivity/win32/find.cpp

src/interactivity/win32/resource.h

DHowett · 2024-05-23T22:21:34Z

Here's a bonus shot:

zadjii-msft · 2024-05-28T11:41:05Z

y draft

DHowett · 2024-05-28T17:18:46Z

y draft

honestly, I don't remember! Something about it made me sad. Perhaps it was the error communication.

carlos-zamora

Looks great! A few small comments I'm sure you'll get to, so I'm going ahead and approving now.

carlos-zamora · 2024-05-30T14:58:45Z

src/host/res.rc

@@ -83,6 +83,7 @@ BEGIN
    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


Curious, any reason why the & isn't before the "R" like &Regular expression?

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.

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. 👀

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. 🙂

carlos-zamora · 2024-05-30T15:09:39Z

src/buffer/out/textBuffer.cpp

 }

 // 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


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?

carlos-zamora · 2024-05-30T15:15:33Z

src/buffer/out/search.cpp

@@ -144,3 +144,8 @@ ptrdiff_t Search::CurrentMatch() const noexcept
 {
    return _index;
 }
+
+bool Search::IsOk() const noexcept


Suggested change

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.

lhecker · 2024-05-30T19:59:50Z

src/buffer/out/textBuffer.cpp


    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)


Negative statuses denote warnings or infos. You should use status > U_ZERO_ERROR here.

lhecker · 2024-05-30T20:02:20Z

src/buffer/out/textBuffer.cpp

 }

 // 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


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) overload

This would also potentially work well in other places like so:

const auto results = textBuffer.SearchText(needle, _flags, _results).value_or({});

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

github-advanced-security bot found potential problems May 23, 2024

View reviewed changes

src/host/res.rc Fixed Show fixed Hide fixed

src/host/resource.h Fixed Show fixed Hide fixed

src/interactivity/win32/find.cpp Fixed Show fixed Hide fixed

src/interactivity/win32/find.cpp Fixed Show fixed Hide fixed

src/interactivity/win32/resource.h Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

DHowett force-pushed the dev/duhowett/regex-search branch from 8803b4c to 63bbcdb Compare May 23, 2024 22:21

This comment has been minimized.

Sign in to view

servusdei2018 approved these changes May 27, 2024

View reviewed changes

DHowett marked this pull request as ready for review May 28, 2024 19:56

carlos-zamora approved these changes May 30, 2024

View reviewed changes

lhecker reviewed May 30, 2024

View reviewed changes

DHowett force-pushed the dev/duhowett/regex-search branch from 63bbcdb to ff63719 Compare May 30, 2024 21:28

search: move caseInsensitive (boolean) to a flags field

fc41d3a

DHowett force-pushed the dev/duhowett/regex-search branch from ff63719 to e5314bf Compare May 30, 2024 21:32

DHowett added 3 commits May 30, 2024 16:32

search: add SearchFlag::RegularExpression

c4390c6

If this flag is set, we will allow ICU to treat the needle as a regex

conhost: add a regex check box that enables regex search

fc8bb05

search: allow search to communicate failure

e5314bf

lhecker approved these changes May 30, 2024

View reviewed changes

zadjii-msft approved these changes May 31, 2024

View reviewed changes

zadjii-msft added this pull request to the merge queue May 31, 2024

zadjii-msft mentioned this pull request May 31, 2024

Epic: Search v2 #3920

Open

20 tasks

Merged via the queue into main with commit ecb5631 May 31, 2024
20 checks passed

zadjii-msft deleted the dev/duhowett/regex-search branch May 31, 2024 11:54

zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for regex search to conhost and Terminal #17316

Add support for regex search to conhost and Terminal #17316

DHowett commented May 23, 2024

This comment has been minimized.

DHowett commented May 23, 2024

This comment has been minimized.

zadjii-msft commented May 28, 2024

DHowett commented May 28, 2024

carlos-zamora left a comment

carlos-zamora May 30, 2024

DHowett May 30, 2024 •

edited

Loading

driver1998 Jul 8, 2024 •

edited

Loading

DHowett Jul 8, 2024

carlos-zamora May 30, 2024

carlos-zamora May 30, 2024

lhecker May 30, 2024

lhecker May 30, 2024

	bool Search::IsOk() const noexcept
	bool Search::IsSearchInvalid() const noexcept

Add support for regex search to conhost and Terminal #17316

Add support for regex search to conhost and Terminal #17316

Conversation

DHowett commented May 23, 2024

This comment has been minimized.

DHowett commented May 23, 2024

This comment has been minimized.

zadjii-msft commented May 28, 2024

DHowett commented May 28, 2024

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora May 30, 2024

Choose a reason for hiding this comment

DHowett May 30, 2024 • edited Loading

Choose a reason for hiding this comment

driver1998 Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

DHowett Jul 8, 2024

Choose a reason for hiding this comment

carlos-zamora May 30, 2024

Choose a reason for hiding this comment

carlos-zamora May 30, 2024

Choose a reason for hiding this comment

lhecker May 30, 2024

Choose a reason for hiding this comment

lhecker May 30, 2024

Choose a reason for hiding this comment

DHowett May 30, 2024 •

edited

Loading

driver1998 Jul 8, 2024 •

edited

Loading