Skip to content

Commit

Permalink
Use the "base" profile for incoming handoff and new commands (#11022)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
DHowett authored Aug 25, 2021
1 parent ee8800c commit ea58e40
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 20 deletions.
18 changes: 15 additions & 3 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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}",
Expand All @@ -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}" } },
Expand Down Expand Up @@ -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<int32_t>('F'), 0 };
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
52 changes: 37 additions & 15 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -2534,23 +2530,37 @@ 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);

// Request a summon of this window to the foreground
_SummonWindowRequestedHandlers(*this, nullptr);
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) };

_CreateNewTabWithProfileAndSettings(profile, settings, connection);

// Request a summon of this window to the foreground
_SummonWindowRequestedHandlers(*this, nullptr);
}
CATCH_RETURN();

return hr;
return S_OK;
}
else
{
til::latch latch{ 1 };
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();
});
Expand Down Expand Up @@ -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;
}
}
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 27 additions & 1 deletion src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ CascadiaSettings::CascadiaSettings(winrt::hstring json) :
{
const auto jsonString{ til::u16u8(json) };
_ParseJsonString(jsonString, false);
_ApplyDefaultsFromUserSettings();
LayerJson(_userSettings);
_ValidateSettings();
}
Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit ea58e40

Please sign in to comment.