Skip to content

Commit 92c43c8

Browse files
DHowettDHowett
committed
Reduce the number of GUID round trips when looking up profiles.
This commit also moves to storing a reference to the active profile inside Pane and propagating it out of Pane through Tab. This lets us duplicate a pane even if its profile is missing, and gives us the freedom in the future to return a "base" profile (;)) Related to #5047.
1 parent 638c6d0 commit 92c43c8

File tree

15 files changed

+128
-173
lines changed

15 files changed

+128
-173
lines changed

‎src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp‎

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ namespace SettingsModelLocalTests
3737

3838
TEST_METHOD(TestTerminalArgsForBinding);
3939

40-
TEST_METHOD(MakeSettingsForProfileThatDoesntExist);
40+
TEST_METHOD(MakeSettingsForProfile);
4141
TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);
4242

4343
TEST_METHOD(TestLayerProfileOnColorScheme);
@@ -126,10 +126,10 @@ namespace SettingsModelLocalTests
126126
VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty());
127127
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
128128

129-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
129+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
130130
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
131131
const auto termSettings = settingsStruct.DefaultSettings();
132-
VERIFY_ARE_EQUAL(guid0, guid);
132+
VERIFY_ARE_EQUAL(guid0, profile.Guid());
133133
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
134134
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
135135
}
@@ -148,10 +148,10 @@ namespace SettingsModelLocalTests
148148
VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty());
149149
VERIFY_ARE_EQUAL(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}", realArgs.TerminalArgs().Profile());
150150

151-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
151+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
152152
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
153153
const auto termSettings = settingsStruct.DefaultSettings();
154-
VERIFY_ARE_EQUAL(guid1, guid);
154+
VERIFY_ARE_EQUAL(guid1, profile.Guid());
155155
VERIFY_ARE_EQUAL(L"pwsh.exe", termSettings.Commandline());
156156
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
157157
}
@@ -170,10 +170,10 @@ namespace SettingsModelLocalTests
170170
VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty());
171171
VERIFY_ARE_EQUAL(L"profile1", realArgs.TerminalArgs().Profile());
172172

173-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
173+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
174174
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
175175
const auto termSettings = settingsStruct.DefaultSettings();
176-
VERIFY_ARE_EQUAL(guid1, guid);
176+
VERIFY_ARE_EQUAL(guid1, profile.Guid());
177177
VERIFY_ARE_EQUAL(L"pwsh.exe", termSettings.Commandline());
178178
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
179179
}
@@ -192,10 +192,10 @@ namespace SettingsModelLocalTests
192192
VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty());
193193
VERIFY_ARE_EQUAL(L"profile2", realArgs.TerminalArgs().Profile());
194194

195-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
195+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
196196
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
197197
const auto termSettings = settingsStruct.DefaultSettings();
198-
VERIFY_ARE_EQUAL(profile2Guid, guid);
198+
VERIFY_ARE_EQUAL(profile2Guid, profile.Guid());
199199
VERIFY_ARE_EQUAL(L"wsl.exe", termSettings.Commandline());
200200
VERIFY_ARE_EQUAL(3, termSettings.HistorySize());
201201
}
@@ -214,10 +214,10 @@ namespace SettingsModelLocalTests
214214
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
215215
VERIFY_ARE_EQUAL(L"foo.exe", realArgs.TerminalArgs().Commandline());
216216

217-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
217+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
218218
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
219219
const auto termSettings = settingsStruct.DefaultSettings();
220-
VERIFY_ARE_EQUAL(guid0, guid);
220+
VERIFY_ARE_EQUAL(guid0, profile.Guid());
221221
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
222222
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
223223
}
@@ -237,10 +237,10 @@ namespace SettingsModelLocalTests
237237
VERIFY_ARE_EQUAL(L"profile1", realArgs.TerminalArgs().Profile());
238238
VERIFY_ARE_EQUAL(L"foo.exe", realArgs.TerminalArgs().Commandline());
239239

240-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
240+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
241241
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
242242
const auto termSettings = settingsStruct.DefaultSettings();
243-
VERIFY_ARE_EQUAL(guid1, guid);
243+
VERIFY_ARE_EQUAL(guid1, profile.Guid());
244244
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
245245
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
246246
}
@@ -257,10 +257,10 @@ namespace SettingsModelLocalTests
257257
VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty());
258258
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
259259

260-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
260+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
261261
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
262262
const auto termSettings = settingsStruct.DefaultSettings();
263-
VERIFY_ARE_EQUAL(guid0, guid);
263+
VERIFY_ARE_EQUAL(guid0, profile.Guid());
264264
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
265265
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
266266
}
@@ -278,10 +278,10 @@ namespace SettingsModelLocalTests
278278
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
279279
VERIFY_ARE_EQUAL(L"c:\\foo", realArgs.TerminalArgs().StartingDirectory());
280280

281-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
281+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
282282
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
283283
const auto termSettings = settingsStruct.DefaultSettings();
284-
VERIFY_ARE_EQUAL(guid0, guid);
284+
VERIFY_ARE_EQUAL(guid0, profile.Guid());
285285
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
286286
VERIFY_ARE_EQUAL(L"c:\\foo", termSettings.StartingDirectory());
287287
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
@@ -301,10 +301,10 @@ namespace SettingsModelLocalTests
301301
VERIFY_ARE_EQUAL(L"c:\\foo", realArgs.TerminalArgs().StartingDirectory());
302302
VERIFY_ARE_EQUAL(L"profile2", realArgs.TerminalArgs().Profile());
303303

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

326-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
326+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
327327
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
328328
const auto termSettings = settingsStruct.DefaultSettings();
329-
VERIFY_ARE_EQUAL(guid0, guid);
329+
VERIFY_ARE_EQUAL(guid0, profile.Guid());
330330
VERIFY_ARE_EQUAL(L"cmd.exe", termSettings.Commandline());
331331
VERIFY_ARE_EQUAL(L"bar", termSettings.StartingTitle());
332332
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
@@ -346,10 +346,10 @@ namespace SettingsModelLocalTests
346346
VERIFY_ARE_EQUAL(L"bar", realArgs.TerminalArgs().TabTitle());
347347
VERIFY_ARE_EQUAL(L"profile2", realArgs.TerminalArgs().Profile());
348348

349-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
349+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
350350
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
351351
const auto termSettings = settingsStruct.DefaultSettings();
352-
VERIFY_ARE_EQUAL(profile2Guid, guid);
352+
VERIFY_ARE_EQUAL(profile2Guid, profile.Guid());
353353
VERIFY_ARE_EQUAL(L"wsl.exe", termSettings.Commandline());
354354
VERIFY_ARE_EQUAL(L"bar", termSettings.StartingTitle());
355355
VERIFY_ARE_EQUAL(3, termSettings.HistorySize());
@@ -371,20 +371,20 @@ namespace SettingsModelLocalTests
371371
VERIFY_ARE_EQUAL(L"bar", realArgs.TerminalArgs().TabTitle());
372372
VERIFY_ARE_EQUAL(L"profile1", realArgs.TerminalArgs().Profile());
373373

374-
const auto guid{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
374+
const auto profile{ settings.GetProfileForArgs(realArgs.TerminalArgs()) };
375375
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, realArgs.TerminalArgs(), nullptr) };
376376
const auto termSettings = settingsStruct.DefaultSettings();
377-
VERIFY_ARE_EQUAL(guid1, guid);
377+
VERIFY_ARE_EQUAL(guid1, profile.Guid());
378378
VERIFY_ARE_EQUAL(L"foo.exe", termSettings.Commandline());
379379
VERIFY_ARE_EQUAL(L"bar", termSettings.StartingTitle());
380380
VERIFY_ARE_EQUAL(L"c:\\foo", termSettings.StartingDirectory());
381381
VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
382382
}
383383
}
384384

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

406406
const auto guid1 = ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
407407
const auto guid2 = ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
408-
const auto guid3 = ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");
408+
409+
const auto profile1 = settings.FindProfile(guid1);
410+
const auto profile2 = settings.FindProfile(guid2);
409411

410412
try
411413
{
412-
auto terminalSettings{ TerminalSettings::CreateWithProfileByID(settings, guid1, nullptr) };
414+
auto terminalSettings{ TerminalSettings::CreateWithProfile(settings, profile1, nullptr) };
413415
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
414416
VERIFY_ARE_EQUAL(1, terminalSettings.DefaultSettings().HistorySize());
415417
}
416418
catch (...)
417419
{
418-
VERIFY_IS_TRUE(false, L"This call to CreateWithProfileByID should succeed");
420+
VERIFY_IS_TRUE(false, L"This call to CreateWithProfile should succeed");
419421
}
420422

421423
try
422424
{
423-
auto terminalSettings{ TerminalSettings::CreateWithProfileByID(settings, guid2, nullptr) };
425+
auto terminalSettings{ TerminalSettings::CreateWithProfile(settings, profile2, nullptr) };
424426
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
425427
VERIFY_ARE_EQUAL(2, terminalSettings.DefaultSettings().HistorySize());
426428
}
427429
catch (...)
428430
{
429-
VERIFY_IS_TRUE(false, L"This call to CreateWithProfileByID should succeed");
431+
VERIFY_IS_TRUE(false, L"This call to CreateWithProfile should succeed");
430432
}
431433

432-
VERIFY_THROWS(auto terminalSettings = TerminalSettings::CreateWithProfileByID(settings, guid3, nullptr), wil::ResultException, L"This call to constructor should fail");
433-
434434
try
435435
{
436436
const auto termSettings{ TerminalSettings::CreateWithNewTerminalArgs(settings, nullptr, nullptr) };

‎src/cascadia/TerminalApp/AppActionHandlers.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -753,11 +753,10 @@ namespace winrt::TerminalApp::implementation
753753
newTerminalArgs = NewTerminalArgs();
754754
}
755755

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

759758
// Manually fill in the evaluated profile.
760-
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profileGuid));
759+
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profile.Guid()));
761760
_OpenNewWindow(false, newTerminalArgs);
762761
actionArgs.Handled(true);
763762
}

‎src/cascadia/TerminalApp/Pane.cpp‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Wi
3434
winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_focusedBorderBrush = { nullptr };
3535
winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_unfocusedBorderBrush = { nullptr };
3636

37-
Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocused) :
37+
Pane::Pane(const Profile& profile, const TermControl& control, const bool lastFocused) :
3838
_control{ control },
3939
_lastActive{ lastFocused },
4040
_profile{ profile }
@@ -758,11 +758,9 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
758758
return;
759759
}
760760

761-
const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
762-
auto paneProfile = settings.FindProfile(_profile.value());
763-
if (paneProfile)
761+
if (_profile)
764762
{
765-
auto mode = paneProfile.CloseOnExit();
763+
auto mode = _profile.CloseOnExit();
766764
if ((mode == CloseOnExitMode::Always) ||
767765
(mode == CloseOnExitMode::Graceful && newConnectionState == ConnectionState::Closed))
768766
{
@@ -786,27 +784,25 @@ void Pane::_ControlWarningBellHandler(const winrt::Windows::Foundation::IInspect
786784
{
787785
return;
788786
}
789-
const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
790-
auto paneProfile = settings.FindProfile(_profile.value());
791-
if (paneProfile)
787+
if (_profile)
792788
{
793789
// We don't want to do anything if nothing is set, so check for that first
794-
if (static_cast<int>(paneProfile.BellStyle()) != 0)
790+
if (static_cast<int>(_profile.BellStyle()) != 0)
795791
{
796-
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
792+
if (WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
797793
{
798794
// Audible is set, play the sound
799795
const auto soundAlias = reinterpret_cast<LPCTSTR>(SND_ALIAS_SYSTEMHAND);
800796
PlaySound(soundAlias, NULL, SND_ALIAS_ID | SND_ASYNC | SND_SENTRY);
801797
}
802798

803-
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
799+
if (WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
804800
{
805801
_control.BellLightOn();
806802
}
807803

808804
// raise the event with the bool value corresponding to the taskbar flag
809-
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
805+
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
810806
}
811807
}
812808
}
@@ -955,10 +951,10 @@ void Pane::SetActive()
955951
// Return Value:
956952
// - nullopt if no children of this pane were the last control to be
957953
// focused, else the GUID of the profile of the last control to be focused
958-
std::optional<GUID> Pane::GetFocusedProfile()
954+
Profile Pane::GetFocusedProfile()
959955
{
960956
auto lastFocused = GetActivePane();
961-
return lastFocused ? lastFocused->_profile : std::nullopt;
957+
return lastFocused ? lastFocused->_profile : nullptr;
962958
}
963959

964960
// Method Description:
@@ -1062,7 +1058,7 @@ void Pane::_FocusFirstChild()
10621058
// - profile: The GUID of the profile these settings should apply to.
10631059
// Return Value:
10641060
// - <none>
1065-
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GUID& profile)
1061+
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile)
10661062
{
10671063
if (!_IsLeaf())
10681064
{
@@ -1071,8 +1067,13 @@ void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GU
10711067
}
10721068
else
10731069
{
1074-
if (profile == _profile)
1070+
// Because this call may be coming in with a different settings tree,
1071+
// we want to map the incoming profile based on its GUID.
1072+
// Failure to find a matching profile will result in a pane holding
1073+
// a reference to a deleted profile (which is okay!).
1074+
if (profile.Guid() == _profile.Guid())
10751075
{
1076+
_profile = profile;
10761077
auto controlSettings = _control.Settings().as<TerminalSettings>();
10771078
// Update the parent of the control's settings object (and not the object itself) so
10781079
// that any overrides made by the control don't get affected by the reload
@@ -1885,7 +1886,7 @@ std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> targe
18851886
// - The two newly created Panes
18861887
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::Split(SplitState splitType,
18871888
const float splitSize,
1888-
const GUID& profile,
1889+
const Profile& profile,
18891890
const TermControl& control)
18901891
{
18911892
if (!_IsLeaf())
@@ -2015,9 +2016,9 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
20152016
// Create two new Panes
20162017
// Move our control, guid into the first one.
20172018
// Move the new guid, control into the second.
2018-
_firstChild = std::make_shared<Pane>(_profile.value(), _control);
2019+
_firstChild = std::make_shared<Pane>(_profile, _control);
20192020
_firstChild->_connectionState = std::exchange(_connectionState, ConnectionState::NotConnected);
2020-
_profile = std::nullopt;
2021+
_profile = nullptr;
20212022
_control = { nullptr };
20222023
_secondChild = newPane;
20232024

0 commit comments

Comments
 (0)
⚡