-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Allow the default profile to be specified by name #5706
Conversation
Fix all the tests, and the schema. Summary: Total=77, Passed=75, Failed=0, Blocked=0, Not Run=0, Skipped=2
…fault-profile-by-name
@@ -867,7 +897,7 @@ namespace TerminalAppLocalTests | |||
VERIFY_IS_TRUE(settings._profiles.at(0)._guid.has_value()); | |||
VERIFY_IS_TRUE(settings._profiles.at(1)._guid.has_value()); | |||
VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(0)._name); | |||
VERIFY_ARE_EQUAL(L"cmd", settings._profiles.at(1)._name); | |||
VERIFY_ARE_EQUAL(L"Command Prompt", settings._profiles.at(1)._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.
Every one of these is because of a test I broke when I renamed cmd
to Command Prompt
:|
// Initially, I wanted to check "has_value" and short-circuit out so that we didn't | ||
// evaluate value_or for every single optional, but has_value/value emits exception handling | ||
// code that value_or doesn't. Less exception handling is cheaper than calling value_or a | ||
// few more times. |
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.
It was really cool to plug this thing into the Godbolt compiler explorer: if you give CoalesceOptionals
three optionals whose values are known at compile time, the optimizer can actually fully pre-compute the return value.
} | ||
|
||
// Method Description: | ||
// - Returns the value from the first populated optional, or a base value if none were populated. |
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.
it's pretty much opt.value_or
without having to write..
opt1.value_or(opt2.value_or(opt3.value_or(base)));
(instead:
CoalesceOptionals(opt1, opt2, opt3, base)
)
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.
Really just minor updates to comments.
// - name: a guid string _or_ the name of a profile | ||
// Return Value: | ||
// - the GUID of the profile corresponding to this name | ||
std::optional<GUID> CascadiaSettings::_GetProfileGuidByName(const std::wstring_view name) const |
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.
nit: The name of this function hides the fact that a GUID is acceptable. Maybe rename it to just _GetProfileGuid()
?
I was going to say GetProfileGuidByString
but the fact that it takes in a string makes the name kinda redundant.
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.
Side note: maybe you'd want to rename GetProfileGuidByIndex
too to make them look nicer together but eh, I don't really care about that one.
if (index) | ||
{ | ||
const auto realIndex = index.value(); | ||
const auto realIndex{ index.value() }; | ||
// If we don't have that many profiles, then do nothing. |
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.
You reversed the logic but forgot to update the comment here.
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.
The comment is durable even if the logic checks for inclusivity instead of exclusivity ;P
@@ -569,58 +555,66 @@ TerminalSettings CascadiaSettings::BuildSettings(GUID profileGuid) const | |||
// - the GUID of the profile corresponding to this combination of index and NewTerminalArgs |
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.
Need a minor update to the comment up here
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.
I can dig this
if (newTerminalArgs) | ||
{ | ||
const auto profileString = newTerminalArgs.Profile(); | ||
return Utils::CoalesceOptionals(profileByName, profileByIndex, _globals.GetDefaultProfile()); |
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.
okay this is neat
{ | ||
const auto unparsedDefaultProfile{ GlobalSettings().GetUnparsedDefaultProfile() }; | ||
auto maybeParsedDefaultProfile{ _GetProfileGuidByName(unparsedDefaultProfile) }; | ||
auto defaultProfileGuid{ Utils::CoalesceOptionals(maybeParsedDefaultProfile, GUID{}) }; |
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.
If the user didn't supply a defaultProfile
before, was the default value GUID{}
or std::nullopt
?
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.
It was "a GUID default-initialized by the GlobalAppSettings constructor" 😄
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.
(so {}
)
Hello @zadjii-msft! 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 (
|
🎉 Handy links: |
Summary of the Pull Request
This looks like a big diff, but there's a bunch of existing code that
just got moved around, and there's a cool new Utils template.
The tests all pass, and this passed manual validation. I tried weird
things like "making a profile named
{ }
"(w/ enough spaces to look like a guid), and yeah it doesn't let you
specify that one as a name, but why would you do that?!
Okay, this pull request abstracts the conversion of a profile name into
an optional profile guid out of the "New Terminal Tab Args" handler and
into a common space for all of CascadiaSettings to use.
It also cleans up the conversion of indices and names into optional
GUIDs and turns those into further helpers.
It also introduces a cool new template for running value_or multiple
times on a chain of optionals. CoalesceOptionals is a "choose first,
with fallback" for N>1 optionals.
On top of all this, I've built support for an "unparsed default GUID":
we load the user's defaultProfile as a string, and as part of settings
validation we unpack that string using the helpers outlined above.
References
Couples well with #5690.
PR Checklist
Validation Steps Performed
Added additional test collateral to make sure that this works.