Use the "base" profile for incoming handoff and new commands by DHowett · Pull Request #11022 · microsoft/terminal

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

    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

 {
    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

    }

-    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

            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 carlos-zamora and zadjii-msft 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 PankajBhojwani, leonMSFT, lhecker and miniksa 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.

Conversation

DHowett commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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.

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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.