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.

Sign up for GitHub

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

Jump to bottom

Rely more on profile objects and less on GUIDs #10982

Merged
merged 3 commits into from
Aug 23, 2021
Merged

Rely more on profile objects and less on GUIDs #10982

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92c43c8
Reduce the number of GUID round trips when looking up profiles.
DHowett Aug 18, 2021
4cd3443
PR Feedback
DHowett Aug 20, 2021
8f57014
Fix the TryDuplicateBadThing tests
DHowett Aug 20, 2021
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
68 changes: 34 additions & 34 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
View file
Open in desktop
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
5 changes: 2 additions & 3 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
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
39 changes: 20 additions & 19 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Wi
winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_focusedBorderBrush = { nullptr };
winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_unfocusedBorderBrush = { nullptr };

Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocused) :
Pane::Pane(const Profile& profile, const TermControl& control, const bool lastFocused) :
_control{ control },
_lastActive{ lastFocused },
_profile{ profile }
Expand Down Expand Up @@ -758,11 +758,9 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
return;
}

const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
auto paneProfile = settings.FindProfile(_profile.value());
if (paneProfile)
if (_profile)
DHowett marked this conversation as resolved.
Show resolved Hide resolved
{
auto mode = paneProfile.CloseOnExit();
auto mode = _profile.CloseOnExit();
DHowett marked this conversation as resolved.
Show resolved Hide resolved
if ((mode == CloseOnExitMode::Always) ||
(mode == CloseOnExitMode::Graceful && newConnectionState == ConnectionState::Closed))
{
Expand All @@ -786,27 +784,25 @@ void Pane::_ControlWarningBellHandler(const winrt::Windows::Foundation::IInspect
{
return;
}
const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
auto paneProfile = settings.FindProfile(_profile.value());
if (paneProfile)
if (_profile)
{
// We don't want to do anything if nothing is set, so check for that first
if (static_cast<int>(paneProfile.BellStyle()) != 0)
if (static_cast<int>(_profile.BellStyle()) != 0)
{
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
if (WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
{
// Audible is set, play the sound
const auto soundAlias = reinterpret_cast<LPCTSTR>(SND_ALIAS_SYSTEMHAND);
PlaySound(soundAlias, NULL, SND_ALIAS_ID | SND_ASYNC | SND_SENTRY);
}

if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
if (WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
{
_control.BellLightOn();
}

// raise the event with the bool value corresponding to the taskbar flag
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
}
}
}
Expand Down Expand Up @@ -955,10 +951,10 @@ void Pane::SetActive()
// Return Value:
// - nullopt if no children of this pane were the last control to be
// focused, else the GUID of the profile of the last control to be focused
DHowett marked this conversation as resolved.
Show resolved Hide resolved
std::optional<GUID> Pane::GetFocusedProfile()
Profile Pane::GetFocusedProfile()
{
auto lastFocused = GetActivePane();
return lastFocused ? lastFocused->_profile : std::nullopt;
return lastFocused ? lastFocused->_profile : nullptr;
}

// Method Description:
Expand Down Expand Up @@ -1062,7 +1058,7 @@ void Pane::_FocusFirstChild()
// - profile: The GUID of the profile these settings should apply to.
DHowett marked this conversation as resolved.
Show resolved Hide resolved
// Return Value:
// - <none>
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GUID& profile)
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile)
{
if (!_IsLeaf())
{
Expand All @@ -1071,8 +1067,13 @@ void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GU
}
else
{
if (profile == _profile)
// Because this call may be coming in with a different settings tree,
// we want to map the incoming profile based on its GUID.
// Failure to find a matching profile will result in a pane holding
// a reference to a deleted profile (which is okay!).
if (profile.Guid() == _profile.Guid())
{
_profile = profile;
auto controlSettings = _control.Settings().as<TerminalSettings>();
// Update the parent of the control's settings object (and not the object itself) so
// that any overrides made by the control don't get affected by the reload
Expand Down Expand Up @@ -1885,7 +1886,7 @@ std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> targe
// - The two newly created Panes
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::Split(SplitState splitType,
const float splitSize,
const GUID& profile,
const Profile& profile,
DHowett marked this conversation as resolved.
Show resolved Hide resolved
const TermControl& control)
{
if (!_IsLeaf())
Expand Down Expand Up @@ -2015,9 +2016,9 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
// Create two new Panes
// Move our control, guid into the first one.
// Move the new guid, control into the second.
_firstChild = std::make_shared<Pane>(_profile.value(), _control);
_firstChild = std::make_shared<Pane>(_profile, _control);
_firstChild->_connectionState = std::exchange(_connectionState, ConnectionState::NotConnected);
_profile = std::nullopt;
_profile = nullptr;
_control = { nullptr };
_secondChild = newPane;

Expand Down
Loading