Skip to content

Commit

Permalink
Rely more on profile objects and less on GUIDs (#10982)
Browse files Browse the repository at this point in the history 
Right now, we store GUIDs in panes and most of the functions for interacting
with profiles on the settings model take GUIDs and look up profiles.

This pull request changes how we store and look up profiles to prefer profile
objects. Panes store strong references to their originating profiles, which
simplifies settings lookup for CloseOnExit and the bell settings. In fact,
deleting a pane's profile no longer causes it to forget which CloseOnExit
setting applies to it. Duplicating a pane that is hosting a deleted profile
(#5047) now duplicates the profile, even though it is otherwise unreachable.

This makes the world more consistent and allows us to _eventually_ support panes
hosting profiles that do not have GUIDs that can be looked up in the profile
list. This is a gateway to #6776 and #10669, and consolidating the profile
lookup logic will help with #10952.

PR #10588 introduced TerminalSettings::CreateWithProfile and made
...CreateWithProfileByID a thin wrapper over top it, which looked up the profile
by GUID before proceeding. It has also been removed, as its last caller is gone.

Closes #5047
  • Loading branch information
@DHowett
DHowett authored Aug 23, 2021
1 parent f681d3a commit f6f5598
Show file tree
Hide file tree
Showing 16 changed files with 142 additions and 183 deletions.
68 changes: 34 additions & 34 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace SettingsModelLocalTests

TEST_METHOD(TestTerminalArgsForBinding);

TEST_METHOD(MakeSettingsForProfileThatDoesntExist);
TEST_METHOD(MakeSettingsForProfile);
TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);

TEST_METHOD(TestLayerProfileOnColorScheme);
Expand Down Expand Up @@ -126,10 +126,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty());
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
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, guid);
VERIFY_ARE_EQUAL(guid0, profile.Guid());
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
}
Expand All @@ -148,10 +148,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}", realArgs.TerminalArgs().Profile());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(guid1, guid);
VERIFY_ARE_EQUAL(guid1, profile.Guid());
VERIFY_ARE_EQUAL(L"pwsh.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
}
Expand All @@ -170,10 +170,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"profile1", realArgs.TerminalArgs().Profile());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(guid1, guid);
VERIFY_ARE_EQUAL(guid1, profile.Guid());
VERIFY_ARE_EQUAL(L"pwsh.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
}
Expand All @@ -192,10 +192,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"profile2", realArgs.TerminalArgs().Profile());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(profile2Guid, guid);
VERIFY_ARE_EQUAL(profile2Guid, profile.Guid());
VERIFY_ARE_EQUAL(L"wsl.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(3, termSettings.HistorySize());
}
Expand All @@ -214,10 +214,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"foo.exe", realArgs.TerminalArgs().Commandline());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
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, guid);
VERIFY_ARE_EQUAL(guid0, profile.Guid());
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
}
Expand All @@ -237,10 +237,10 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(L"profile1", realArgs.TerminalArgs().Profile());
VERIFY_ARE_EQUAL(L"foo.exe", realArgs.TerminalArgs().Commandline());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(guid1, guid);
VERIFY_ARE_EQUAL(guid1, profile.Guid());
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
}
Expand All @@ -257,10 +257,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty());
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
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, guid);
VERIFY_ARE_EQUAL(guid0, profile.Guid());
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
}
Expand All @@ -278,10 +278,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"c:\\foo", realArgs.TerminalArgs().StartingDirectory());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
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, guid);
VERIFY_ARE_EQUAL(guid0, profile.Guid());
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(L"c:\\foo", termSettings.StartingDirectory());
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
Expand All @@ -301,10 +301,10 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(L"c:\\foo", realArgs.TerminalArgs().StartingDirectory());
VERIFY_ARE_EQUAL(L"profile2", realArgs.TerminalArgs().Profile());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(profile2Guid, guid);
VERIFY_ARE_EQUAL(profile2Guid, profile.Guid());
VERIFY_ARE_EQUAL(L"wsl.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(L"c:\\foo", termSettings.StartingDirectory());
VERIFY_ARE_EQUAL(3, termSettings.HistorySize());
Expand All @@ -323,10 +323,10 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"bar", realArgs.TerminalArgs().TabTitle());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
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, guid);
VERIFY_ARE_EQUAL(guid0, profile.Guid());
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(L"bar", termSettings.StartingTitle());
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
Expand All @@ -346,10 +346,10 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(L"bar", realArgs.TerminalArgs().TabTitle());
VERIFY_ARE_EQUAL(L"profile2", realArgs.TerminalArgs().Profile());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(profile2Guid, guid);
VERIFY_ARE_EQUAL(profile2Guid, profile.Guid());
VERIFY_ARE_EQUAL(L"wsl.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(L"bar", termSettings.StartingTitle());
VERIFY_ARE_EQUAL(3, termSettings.HistorySize());
Expand All @@ -371,20 +371,20 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(L"bar", realArgs.TerminalArgs().TabTitle());
VERIFY_ARE_EQUAL(L"profile1", realArgs.TerminalArgs().Profile());

const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
const auto termSettings = settingsStruct.DefaultSettings();
VERIFY_ARE_EQUAL(guid1, guid);
VERIFY_ARE_EQUAL(guid1, profile.Guid());
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
VERIFY_ARE_EQUAL(L"bar", termSettings.StartingTitle());
VERIFY_ARE_EQUAL(L"c:\\foo", termSettings.StartingDirectory());
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
}
}

void TerminalSettingsTests::MakeSettingsForProfileThatDoesntExist()
void TerminalSettingsTests::MakeSettingsForProfile()
{
// Test that making settings throws when the GUID doesn't exist
// Test that making settings generally works.
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
Expand All @@ -405,32 +405,32 @@ namespace SettingsModelLocalTests

const auto guid1 = ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
const auto guid2 = ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
const auto guid3 = ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");

const auto profile1 = settings.FindProfile(guid1);
const auto profile2 = settings.FindProfile(guid2);

try
{
auto terminalSettings{ TerminalSettings::CreateWithProfileByID(settings, guid1, nullptr) };
auto terminalSettings{ TerminalSettings::CreateWithProfile(settings, profile1, nullptr) };
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(1, terminalSettings.DefaultSettings().HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to CreateWithProfileByID should succeed");
VERIFY_IS_TRUE(false, L"This call to CreateWithProfile should succeed");
}

try
{
auto terminalSettings{ TerminalSettings::CreateWithProfileByID(settings, guid2, nullptr) };
auto terminalSettings{ TerminalSettings::CreateWithProfile(settings, profile2, nullptr) };
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(2, terminalSettings.DefaultSettings().HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to CreateWithProfileByID should succeed");
VERIFY_IS_TRUE(false, L"This call to CreateWithProfile should succeed");
}

VERIFY_THROWS(auto terminalSettings = TerminalSettings::CreateWithProfileByID(settings, guid3, nullptr), wil::ResultException, L"This call to constructor should fail");

try
{
const auto termSettings{ TerminalSettings::CreateWithNewTerminalArgs(settings, nullptr, nullptr) };
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ namespace TerminalAppLocalTests
Log::Comment(L"Duplicate the tab, and don't crash");
result = RunOnUIThread([&page]() {
page->_DuplicateFocusedTab();
VERIFY_ARE_EQUAL(2u, page->_tabs.Size(), L"We should gracefully do nothing here - the profile no longer exists.");
VERIFY_ARE_EQUAL(3u, page->_tabs.Size(), L"We should successfully duplicate a tab hosting a deleted profile.");
});
VERIFY_SUCCEEDED(result);
}
Expand Down Expand Up @@ -530,9 +530,9 @@ namespace TerminalAppLocalTests

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
VERIFY_ARE_EQUAL(2,
VERIFY_ARE_EQUAL(3,
tab->GetLeafPaneCount(),
L"We should gracefully do nothing here - the profile no longer exists.");
L"We should successfully duplicate a pane hosting a deleted profile.");
});
VERIFY_SUCCEEDED(result);

Expand Down
5 changes: 2 additions & 3 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,11 +753,10 @@ namespace winrt::TerminalApp::implementation
newTerminalArgs = NewTerminalArgs();
}

const auto profileGuid{ _settings.GetProfileForArgs(newTerminalArgs) };
const auto settings{ TerminalSettings::CreateWithNewTerminalArgs(_settings, newTerminalArgs, *_bindings) };
const auto profile{ _settings.GetProfileForArgs(newTerminalArgs) };

// Manually fill in the evaluated profile.
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profileGuid));
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profile.Guid()));
_OpenNewWindow(false, newTerminalArgs);
actionArgs.Handled(true);
}
Expand Down
Loading

0 comments on commit f6f5598

Please sign in to comment.