Use the "base" profile for incoming handoff and new commands #11022
Conversation
| @@ -806,12 +806,6 @@ namespace winrt::TerminalApp::implementation | |||
| TerminalConnection::ITerminalConnection TerminalPage::_CreateConnectionFromSettings(Profile profile, | |||
| TerminalSettings settings) | |||
| { | |||
| if (!profile) | |||
There was a problem hiding this comment.
If we get here with a null profile, a crash report would be nice.
There was a problem hiding this comment.
If you want, you could add an assert, I guess
| VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline()); | ||
| VERIFY_ARE_EQUAL(1, termSettings.HistorySize()); | ||
| VERIFY_ARE_EQUAL(29, termSettings.HistorySize()); |
There was a problem hiding this comment.
wait a minute, this has to go into the if above
src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
Outdated
Show resolved
Hide resolved
| @@ -59,6 +59,7 @@ CascadiaSettings::CascadiaSettings(winrt::hstring json) : | |||
| { | |||
| const auto jsonString{ til::u16u8(json) }; | |||
| _ParseJsonString(jsonString, false); | |||
| _ApplyDefaultsFromUserSettings(); | |||
There was a problem hiding this comment.
this method is only used in the tests; adding this ensures that ProfileDefaults() is populated :|
we gotta fix this yo
| @@ -817,7 +818,32 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::GetProfil | |||
| profileByName = _GetProfileGuidByName(newTerminalArgs.Profile()); | |||
| } | |||
|
|
|||
| return FindProfile(til::coalesce_value(profileByName, profileByIndex, _globals->DefaultProfile())); | |||
| if (profileByName) | |||
There was a problem hiding this comment.
Okay so _GetProfileGuidByName is going to return nullopt because the profile is L"". profileByIndex is definitely nullopt, so we fall through.
there are new terminal args, and the commandline isn't empty, so we use the base layer.
| @@ -217,9 +220,18 @@ namespace SettingsModelLocalTests | |||
| const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) }; | |||
| const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) }; | |||
| const auto termSettings = settingsStruct.DefaultSettings(); | |||
| VERIFY_ARE_EQUAL(guid0, profile.Guid()); | |||
| if constexpr (Feature_ShowProfileDefaultsInSettings::IsEnabled()) | |||
There was a problem hiding this comment.
is it even possible to run these tests in both configurations? These things get compiled in presumably. Huh. Interesting. I'm not even sure which one would get built by default, presumably, the same as a dev build? Every feature on?
There was a problem hiding this comment.
Correct! It's disabled in release, and somebody would have to specifically build with release branding to get this path
|
Hello @DHowett! 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: |
This pull request introduces our first use of the "base" profile as an
actual profile. Incoming commandlines from
wt fooand defaultterminal handoffs will be hosted in the base profile.
THIS IS A BREAKING CHANGE for user behavior.
The original behavior where commandlines were hosted in the "default"
profile (in most cases, Windows PowerShell) led to user confusion: "why
does cmd use my powershell icon?" and "why does the title say
PowerShell?". Making this change unifies the user experience so that we
can land commandline detection in #10952.
Users who want the original behavior can get it back for commandline
invocation by specifying a profile using the
-pargument, as inwt -p PowerShell -- cmd.As a temporary stopgap, users who attempt to duplicate the base profile
will get their specified default profile until we land #5047.
This feature is hidden behind the same feature flag that controls the
visibility of base/"Defaults" in the settings UI.
Fixes #10669
Related to #6776