Use the "base" profile for incoming handoff and new commands #11022

DHowett · 2021-08-23T21:40:29Z

This pull request introduces our first use of the "base" profile as an
actual profile. Incoming commandlines from wt foo and default
terminal 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 -p argument, as in wt -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

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2021-08-23T21:46:14Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -806,12 +806,6 @@ namespace winrt::TerminalApp::implementation
    TerminalConnection::ITerminalConnection TerminalPage::_CreateConnectionFromSettings(Profile profile,
                                                                                        TerminalSettings settings)
    {
-        if (!profile)


If we get here with a null profile, a crash report would be nice.

If you want, you could add an assert, I guess

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2021-08-25T21:49:30Z

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

            VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
-            VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
+            VERIFY_ARE_EQUAL(29, termSettings.HistorySize());


wait a minute, this has to go into the if above

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

DHowett · 2021-08-25T21:51:57Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

@@ -59,6 +59,7 @@ CascadiaSettings::CascadiaSettings(winrt::hstring json) :
 {
    const auto jsonString{ til::u16u8(json) };
    _ParseJsonString(jsonString, false);
+    _ApplyDefaultsFromUserSettings();


this method is only used in the tests; adding this ensures that ProfileDefaults() is populated :|

we gotta fix this yo

zadjii-msft · 2021-08-25T22:05:54Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

@@ -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)


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.

zadjii-msft · 2021-08-25T22:08:33Z

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

@@ -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())


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?

Correct! It's disabled in release, and somebody would have to specifically build with release branding to get this path

or, the other path

src/cascadia/TerminalApp/TerminalPage.cpp

ghost · 2021-08-25T22:11:11Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-08-31T17:12:00Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

DHowett added 3 commits August 23, 2021 12:25

Use the Base profile for incoming connections

42f53a3

Use the Base profile for unspecified NTA that only have commandlines

0b94349

Tidy for review

75d05bc

DHowett marked this pull request as draft August 23, 2021 21:40

DHowett commented Aug 23, 2021

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

DHowett commented Aug 23, 2021

View reviewed changes

DHowett added 2 commits August 24, 2021 12:49

Fix the tests for the new def. profile

452c3c6

hook up github numbers

36e202b

ghost added Area-DefApp Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 24, 2021

DHowett marked this pull request as ready for review August 24, 2021 19:54

DHowett requested review from zadjii-msft and carlos-zamora and removed request for zadjii-msft August 24, 2021 19:55

carlos-zamora approved these changes Aug 24, 2021

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

FR Peedback

d50050a

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 25, 2021

ghost requested review from miniksa, leonMSFT, PankajBhojwani and lhecker August 25, 2021 17:34

DHowett commented Aug 25, 2021

View reviewed changes

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp Show resolved Hide resolved

DHowett commented Aug 25, 2021

View reviewed changes

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp Show resolved Hide resolved

DHowett commented Aug 25, 2021

View reviewed changes

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review (thanks dustin)

77e9a81

DHowett commented Aug 25, 2021

View reviewed changes

zadjii-msft approved these changes Aug 25, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 25, 2021

ghost merged commit ea58e40 into main Aug 25, 2021

ghost deleted the dev/duhowett/profiles-defaults branch August 25, 2021 22:41

ghost mentioned this pull request Aug 31, 2021

Launching any cli app(such as powershell, wsl) sets the tab's icon as the default profile's icon #10669

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the "base" profile for incoming handoff and new commands #11022

Use the "base" profile for incoming handoff and new commands #11022

DHowett commented Aug 23, 2021 •

edited

Loading

DHowett Aug 23, 2021

carlos-zamora Aug 24, 2021

DHowett Aug 25, 2021

DHowett Aug 25, 2021

zadjii-msft Aug 25, 2021

DHowett Aug 25, 2021

zadjii-msft Aug 25, 2021

DHowett Aug 25, 2021

DHowett Aug 25, 2021

ghost commented Aug 25, 2021

ghost commented Aug 31, 2021

Use the "base" profile for incoming handoff and new commands #11022

Use the "base" profile for incoming handoff and new commands #11022

Conversation

DHowett commented Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 25, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

DHowett commented Aug 23, 2021 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.