From ea58e4036b6583fa318415d3c2c993e12f9c9abf Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Wed, 25 Aug 2021 15:41:42 -0700 Subject: [PATCH] Use the "base" profile for incoming handoff and new commands (#11022) 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 --- .../TerminalSettingsTests.cpp | 18 +++++-- src/cascadia/TerminalApp/TabManagement.cpp | 4 +- src/cascadia/TerminalApp/TerminalPage.cpp | 50 +++++++++++++------ src/cascadia/TerminalApp/TerminalPage.h | 2 + .../CascadiaSettings.cpp | 28 ++++++++++- 5 files changed, 83 insertions(+), 19 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp index cecfade05..3b2ed8d14 100644 --- a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp @@ -65,7 +65,7 @@ namespace SettingsModelLocalTests const std::string settingsJson{ R"( { "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", - "profiles": [ + "profiles": { "list": [ { "name": "profile0", "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", @@ -84,6 +84,9 @@ namespace SettingsModelLocalTests "commandline": "wsl.exe" } ], + "defaults": { + "historySize": 29 + } }, "keybindings": [ { "keys": ["ctrl+a"], "command": { "action": "splitPane", "split": "vertical" } }, { "keys": ["ctrl+b"], "command": { "action": "splitPane", "split": "vertical", "profile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" } }, @@ -219,9 +222,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()) + { + // This action specified a command but no profile; it gets reassigned to the base profile + VERIFY_ARE_EQUAL(settings.ProfileDefaults(), profile); + VERIFY_ARE_EQUAL(29, termSettings.HistorySize()); + } + else + { + VERIFY_ARE_EQUAL(guid0, profile.Guid()); + VERIFY_ARE_EQUAL(1, termSettings.HistorySize()); + } VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline()); - VERIFY_ARE_EQUAL(1, termSettings.HistorySize()); } { KeyChord kc{ true, false, false, false, static_cast('F'), 0 }; diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp index 16149d550..849bfcc69 100644 --- a/src/cascadia/TerminalApp/TabManagement.cpp +++ b/src/cascadia/TerminalApp/TabManagement.cpp @@ -350,8 +350,10 @@ namespace winrt::TerminalApp::implementation // current control's live settings (which will include changes // made through VT). - if (const auto profile = tab.GetFocusedProfile()) + if (auto profile = tab.GetFocusedProfile()) { + // TODO GH#5047 If we cache the NewTerminalArgs, we no longer need to do this. + profile = GetClosestProfileForDuplicationOfProfile(profile); const auto settingsCreateResult{ TerminalSettings::CreateWithProfile(_settings, profile, *_bindings) }; const auto workingDirectory = tab.GetActiveTerminalControl().WorkingDirectory(); const auto validWorkingDirectory = !workingDirectory.empty(); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 6a0fcea87..8ae486548 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -814,12 +814,6 @@ namespace winrt::TerminalApp::implementation TerminalConnection::ITerminalConnection TerminalPage::_CreateConnectionFromSettings(Profile profile, TerminalSettings settings) { - if (!profile) - { - // Use the default profile if we didn't get one as an argument. - profile = _settings.FindProfile(_settings.GlobalSettings().DefaultProfile()); - } - TerminalConnection::ITerminalConnection connection{ nullptr }; winrt::guid connectionType = profile.ConnectionType(); @@ -1369,6 +1363,8 @@ namespace winrt::TerminalApp::implementation profile = tab.GetFocusedProfile(); if (profile) { + // TODO GH#5047 If we cache the NewTerminalArgs, we no longer need to do this. + profile = GetClosestProfileForDuplicationOfProfile(profile); controlSettings = TerminalSettings::CreateWithProfile(_settings, profile, *_bindings); const auto workingDirectory = tab.GetActiveTerminalControl().WorkingDirectory(); const auto validWorkingDirectory = !workingDirectory.empty(); @@ -2534,13 +2530,28 @@ namespace winrt::TerminalApp::implementation // and wait on it hence the locking mechanism. if (Dispatcher().HasThreadAccess()) { - // TODO: GH 9458 will give us more context so we can try to choose a better profile. - auto hr = _OpenNewTab(nullptr, connection); + try + { + NewTerminalArgs newTerminalArgs{}; + // TODO GH#10952: When we pass the actual commandline (or originating application), the + // settings model can choose the right settings based on command matching, or synthesize + // a profile from the registry/link settings (TODO GH#9458). + // TODO GH#9458: Get and pass the LNK/EXE filenames. + // Passing in a commandline forces GetProfileForArgs to use Base Layer instead of Default Profile; + // in the future, it can make a better decision based on the value we pull out of the process handle. + // TODO GH#5047: When we hang on to the N.T.A., try not to spawn "default... .exe" :) + newTerminalArgs.Commandline(L"default-terminal-invocation-placeholder"); + const auto profile{ _settings.GetProfileForArgs(newTerminalArgs) }; + const auto settings{ TerminalSettings::CreateWithProfile(_settings, profile, *_bindings) }; - // Request a summon of this window to the foreground - _SummonWindowRequestedHandlers(*this, nullptr); + _CreateNewTabWithProfileAndSettings(profile, settings, connection); - return hr; + // Request a summon of this window to the foreground + _SummonWindowRequestedHandlers(*this, nullptr); + } + CATCH_RETURN(); + + return S_OK; } else { @@ -2548,9 +2559,8 @@ namespace winrt::TerminalApp::implementation HRESULT finalVal = S_OK; Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [&]() { - finalVal = _OpenNewTab(nullptr, connection); - - _SummonWindowRequestedHandlers(*this, nullptr); + // Re-running ourselves under the dispatcher will cause us to take the first branch above. + finalVal = _OnNewConnection(connection); latch.count_down(); }); @@ -3048,4 +3058,16 @@ namespace winrt::TerminalApp::implementation { return WindowName() == QuakeWindowName; } + + // Method Description: + // - This function stops people from duplicating the base profile, because + // it gets ~ ~ weird ~ ~ when they do. Remove when TODO GH#5047 is done. + Profile TerminalPage::GetClosestProfileForDuplicationOfProfile(const Profile& profile) const noexcept + { + if (profile == _settings.ProfileDefaults()) + { + return _settings.FindProfile(_settings.GlobalSettings().DefaultProfile()); + } + return profile; + } } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index ae3b8c012..0bdcf4aae 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -358,6 +358,8 @@ namespace winrt::TerminalApp::implementation void _SetFocusMode(const bool inFocusMode); + winrt::Microsoft::Terminal::Settings::Model::Profile GetClosestProfileForDuplicationOfProfile(const winrt::Microsoft::Terminal::Settings::Model::Profile& profile) const noexcept; + #pragma region ActionHandlers // These are all defined in AppActionHandlers.cpp #define ON_ALL_ACTIONS(action) DECLARE_ACTION_HANDLER(action); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 052026fb9..aff7cba52 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -59,6 +59,7 @@ CascadiaSettings::CascadiaSettings(winrt::hstring json) : { const auto jsonString{ til::u16u8(json) }; _ParseJsonString(jsonString, false); + _ApplyDefaultsFromUserSettings(); LayerJson(_userSettings); _ValidateSettings(); } @@ -832,7 +833,32 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::GetProfil profileByName = _GetProfileGuidByName(newTerminalArgs.Profile()); } - return FindProfile(til::coalesce_value(profileByName, profileByIndex, _globals->DefaultProfile())); + if (profileByName) + { + return FindProfile(*profileByName); + } + + if (profileByIndex) + { + return FindProfile(*profileByIndex); + } + + if constexpr (Feature_ShowProfileDefaultsInSettings::IsEnabled()) + { + // If the user has access to the "Defaults" profile, and no profile was otherwise specified, + // what we do is dependent on whether there was a commandline. + // If there was a commandline (case 1), we we'll launch in the "Defaults" profile. + // If there wasn't a commandline or there wasn't a NewTerminalArgs (case 2), we'll + // launch in the user's actual default profile. + // Case 2 above could be the result of a "nt" or "sp" invocation that doesn't specify anything. + // TODO GH#10952: Detect the profile based on the commandline (add matching support) + return (!newTerminalArgs || newTerminalArgs.Commandline().empty()) ? + FindProfile(GlobalSettings().DefaultProfile()) : + ProfileDefaults(); + } + + // For compatibility with the stable version's behavior, return the default by GUID in all other cases. + return FindProfile(GlobalSettings().DefaultProfile()); } // Method Description: