-
Notifications
You must be signed in to change notification settings - Fork 8.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace New Tab Menu Match Profiles functionality with regex support #18654
base: main
Are you sure you want to change the base?
Conversation
if (!_Name.empty()) | ||
{ | ||
isMatching = { isMatching.value_or(true) && _Name == profile.Name() }; | ||
isMatching = { isMatching.value_or(true) && isMatch(_Name, profile.Name()) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optional-bool code seems like a roundabout way to say "return early if true". 😅
if (!field.empty() && isMatch(field, ...)) {
return true;
}
if (!field.empty() && isMatch(field, ...)) {
return true;
}
if (!field.empty() && isMatch(field, ...)) {
return true;
}
return false;
{ | ||
try | ||
{ | ||
std::wregex{ regex.cbegin(), regex.cend() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AGH! STD REGEX MY MORTAL ENEMY! 😄
That said, regex construction can be fairly costly in general, and we should not treat this as some kind of cheap validation. It involves constructing DFAs, etc., after all. I'm not entirely sure anymore where MatchProfilesEntry
fits into the callstack, but seeing that it's also part of TerminalSettingsModel
, I'm sure that we can make it cache every regex instance after load and store any warnings into a list. We can then splice its list into the overall SettingsLoadWarnings
list (or store it directly there, or something like that). Basically, a way to only construct these once.
FWIW if you want to give icu.h
a try:
#include "path/to/buffer/out/UTextAdapter.h"
// Cache this
UErrorCode status = U_ZERO_ERROR;
const auto re = Microsoft::Console::ICU::CreateRegex(pattern, 0, &status);
if (status > U_ZERO_ERROR) {
// bad regex
}
// Run this
UErrorCode status = U_ZERO_ERROR;
uregex_setText(re.get(), hstring.data(), hstring.size(), &status);
const auto match = uregex_matches(re.get(), 0, &status);
return status == U_ZERO_ERROR && match;
Summary of the Pull Request
Updates the New Tab Menu's Match Profiles entry to support regex instead of doing a direct match. Also adds validation to ensure the regex is valid. Updated the UI to help make it more clear that this supports regexes and even added a link to some helpful docs.
Validation Steps Performed
✅ Invalid regex displays a warning
✅ Valid regex works nicely
PR Checklist
Closes #18553