Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
7 commits merged into from
Aug 25, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,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 @@ -82,6 +82,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 @@ -217,9 +220,17 @@ 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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or, the other path

{
// This action specified a command but no profile; it gets reassigned to the base profile
VERIFY_ARE_EQUAL(settings.ProfileDefaults(), profile);
DHowett marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
VERIFY_ARE_EQUAL(guid0, profile.Guid());
DHowett marked this conversation as resolved.
Show resolved Hide resolved
}
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
VERIFY_ARE_EQUAL(29, termSettings.HistorySize());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DHowett marked this conversation as resolved.
Show resolved Hide resolved
}
{
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 @@ -806,12 +806,6 @@ namespace winrt::TerminalApp::implementation
TerminalConnection::ITerminalConnection TerminalPage::_CreateConnectionFromSettings(Profile profile,
TerminalSettings settings)
{
if (!profile)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// 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 @@ -1361,6 +1355,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 @@ -2518,23 +2514,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");
DHowett marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -3032,4 +3042,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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

we gotta fix this yo

LayerJson(_userSettings);
_ValidateSettings();
}
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

{
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