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
DHowett-MSFT
changed the title
| @@ -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.
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.
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.
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)
)
ghost
added
Area-Settings
| // - 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.
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.
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.
You reversed the logic but forgot to update the comment here.
There was a problem hiding this comment.
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.
Need a minor update to the comment up here
| if (newTerminalArgs) | ||
| { | ||
| const auto profileString = newTerminalArgs.Profile(); | ||
| return Utils::CoalesceOptionals(profileByName, profileByIndex, _globals.GetDefaultProfile()); |
| { | ||
| const auto unparsedDefaultProfile{ GlobalSettings().GetUnparsedDefaultProfile() }; | ||
| auto maybeParsedDefaultProfile{ _GetProfileGuidByName(unparsedDefaultProfile) }; | ||
| auto defaultProfileGuid{ Utils::CoalesceOptionals(maybeParsedDefaultProfile, GUID{}) }; |
There was a problem hiding this comment.
If the user didn't supply a defaultProfile before, was the default value GUID{} or std::nullopt?
There was a problem hiding this comment.
It was "a GUID default-initialized by the GlobalAppSettings constructor" 😄
zadjii-msft
added
the
AutoMerge
|
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 (
|
ghost
merged commit
|
🎉 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.