Skip to content

Add support for regex search to conhost and Terminal#17316

Merged
zadjii-msft merged 4 commits intomainfrom
dev/duhowett/regex-search
May 31, 2024
Merged

Add support for regex search to conhost and Terminal#17316
zadjii-msft merged 4 commits intomainfrom
dev/duhowett/regex-search

Conversation

@DHowett
Copy link
Member

@DHowett DHowett commented May 23, 2024

This is broken down into individual reviewable commits.

Here is a video of it in action!

Part of #3920

@github-actions

This comment has been minimized.

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

DHowett commented May 23, 2024

Here's a bonus shot:
image

@github-actions

This comment has been minimized.

@zadjii-msft
Copy link
Member

y draft

@DHowett
Copy link
Member Author

DHowett commented May 28, 2024

y draft

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

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

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@DHowett DHowett May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@driver1998 driver1998 Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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({});

@DHowett DHowett force-pushed the dev/duhowett/regex-search branch from 63bbcdb to ff63719 Compare May 30, 2024 21:28
@DHowett DHowett force-pushed the dev/duhowett/regex-search branch from ff63719 to e5314bf Compare May 30, 2024 21:32
@zadjii-msft zadjii-msft added this pull request to the merge queue May 31, 2024
@zadjii-msft zadjii-msft mentioned this pull request May 31, 2024
20 tasks
Merged via the queue into main with commit ecb5631 May 31, 2024
@zadjii-msft zadjii-msft deleted the dev/duhowett/regex-search branch May 31, 2024 11:54
@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024
DHowett added a commit that referenced this pull request Sep 24, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

6 participants