From a8bd5036f0d85d9910e56afb86664b8bca9c0d4e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 11:34:14 -0600 Subject: [PATCH 1/8] I believe this is the bugfix --- src/cascadia/TerminalApp/TerminalPage.cpp | 112 +++++++++++++++------- 1 file changed, 75 insertions(+), 37 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index bc85e5da2ee..82249475566 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -468,23 +468,32 @@ namespace winrt::TerminalApp::implementation // configurations. See CascadiaSettings::BuildSettings for more details. void TerminalPage::_OpenNewTab(const winrt::TerminalApp::NewTerminalArgs& newTerminalArgs) { - const auto [profileGuid, settings] = _settings->BuildSettings(newTerminalArgs); - - _CreateNewTabFromSettings(profileGuid, settings); - - const int tabCount = static_cast(_tabs.size()); - const bool usedManualProfile = (newTerminalArgs != nullptr) && - (newTerminalArgs.ProfileIndex() != nullptr || - newTerminalArgs.Profile().empty()); - TraceLoggingWrite( - g_hTerminalAppProvider, // handle to TerminalApp tracelogging provider - "TabInformation", - TraceLoggingDescription("Event emitted upon new tab creation in TerminalApp"), - TraceLoggingInt32(tabCount, "TabCount", "Count of tabs curently opened in TerminalApp"), - TraceLoggingBool(usedManualProfile, "ProfileSpecified", "Whether the new tab specified a profile explicitly"), - TraceLoggingGuid(profileGuid, "ProfileGuid", "The GUID of the profile spawned in the new tab"), - TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), - TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance)); + try + { + const auto [profileGuid, settings] = _settings->BuildSettings(newTerminalArgs); + + _CreateNewTabFromSettings(profileGuid, settings); + + const int tabCount = static_cast(_tabs.size()); + const bool usedManualProfile = (newTerminalArgs != nullptr) && + (newTerminalArgs.ProfileIndex() != nullptr || + newTerminalArgs.Profile().empty()); + TraceLoggingWrite( + g_hTerminalAppProvider, // handle to TerminalApp tracelogging provider + "TabInformation", + TraceLoggingDescription("Event emitted upon new tab creation in TerminalApp"), + TraceLoggingInt32(tabCount, "TabCount", "Count of tabs curently opened in TerminalApp"), + TraceLoggingBool(usedManualProfile, "ProfileSpecified", "Whether the new tab specified a profile explicitly"), + TraceLoggingGuid(profileGuid, "ProfileGuid", "The GUID of the profile spawned in the new tab"), + TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), + TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance)); + } + CATCH_LOG(); + // TODO: Should we display a dialog when we do nothing because we + // couldn't find the associated profile? This can only really happen in + // the duplicate tab scenario currently, but maybe in the future of + // keybindings with args, someone could manually open a tab/pane with + // specific a GUID. } winrt::fire_and_forget TerminalPage::_RemoveOnCloseRoutine(Microsoft::UI::Xaml::Controls::TabViewItem tabViewItem, winrt::com_ptr page) @@ -798,8 +807,17 @@ namespace winrt::TerminalApp::implementation const auto& profileGuid = _tab->GetFocusedProfile(); if (profileGuid.has_value()) { - const auto settings = _settings->BuildSettings(profileGuid.value()); - _CreateNewTabFromSettings(profileGuid.value(), settings); + try + { + const auto settings = _settings->BuildSettings(profileGuid.value()); + _CreateNewTabFromSettings(profileGuid.value(), settings); + } + CATCH_LOG(); + // TODO: Should we display a dialog when we do nothing because we + // couldn't find the associated profile? This can only really happen in + // the duplicate tab scenario currently, but maybe in the future of + // keybindings with args, someone could manually open a tab/pane with + // specific a GUID. } } @@ -1049,26 +1067,35 @@ namespace winrt::TerminalApp::implementation return; } - const auto [realGuid, controlSettings] = _settings->BuildSettings(newTerminalArgs); + try + { + const auto [realGuid, controlSettings] = _settings->BuildSettings(newTerminalArgs); - const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings); + const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings); - const int focusedTabIndex = _GetFocusedTabIndex(); - auto focusedTab = _tabs[focusedTabIndex]; + const int focusedTabIndex = _GetFocusedTabIndex(); + auto focusedTab = _tabs[focusedTabIndex]; - const auto canSplit = focusedTab->CanSplitPane(splitType); + const auto canSplit = focusedTab->CanSplitPane(splitType); - if (!canSplit) - { - return; - } + if (!canSplit) + { + return; + } - TermControl newControl{ controlSettings, controlConnection }; + TermControl newControl{ controlSettings, controlConnection }; - // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(newControl, focusedTab); + // Hookup our event handlers to the new terminal + _RegisterTerminalEvents(newControl, focusedTab); - focusedTab->SplitPane(splitType, realGuid, newControl); + focusedTab->SplitPane(splitType, realGuid, newControl); + } + CATCH_LOG(); + // TODO: Should we display a dialog when we do nothing because we + // couldn't find the associated profile? This can only really happen in + // the duplicate tab scenario currently, but maybe in the future of + // keybindings with args, someone could manually open a tab/pane with + // specific a GUID. } // Method Description: @@ -1471,13 +1498,24 @@ namespace winrt::TerminalApp::implementation for (auto& profile : profiles) { const GUID profileGuid = profile.GetGuid(); - const auto settings = _settings->BuildSettings(profileGuid); - - for (auto& tab : _tabs) + try { - // Attempt to reload the settings of any panes with this profile - tab->UpdateSettings(settings, profileGuid); + // BuildSettings can throw an exception if the profileGuid does + // not belong to an actual profile in the list of profiles. + const auto settings = _settings->BuildSettings(profileGuid); + + for (auto& tab : _tabs) + { + // Attempt to reload the settings of any panes with this profile + tab->UpdateSettings(settings, profileGuid); + } } + CATCH_LOG(); + // TODO: Should we display a dialog when we do nothing because we + // couldn't find the associated profile? This can only really happen in + // the duplicate tab scenario currently, but maybe in the future of + // keybindings with args, someone could manually open a tab/pane with + // specific a GUID. } // Update the icon of the tab for the currently focused profile in that tab. From 648373d05cb24aadd49d522f2348994bf0bbd248 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 11:35:06 -0600 Subject: [PATCH 2/8] Try writing tests You need to initialize a TerminalPage weirdly in the test, including _not_ checking App.Logic(), since the App doesn't have a Logic() --- .../LocalTests_TerminalApp/SettingsTests.cpp | 152 ++++++++++++++++++ .../LocalTests_TerminalApp/TabTests.cpp | 95 +++++++++++ src/cascadia/TerminalApp/TerminalPage.cpp | 7 +- 3 files changed, 253 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index c00dd15a31d..15ddbadcb4a 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -70,6 +70,10 @@ namespace TerminalAppLocalTests TEST_METHOD(TestTerminalArgsForBinding); + TEST_METHOD(FindMissingProfile); + TEST_METHOD(MakeSettingsForProfileThatDoesntExist); + TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist); + TEST_CLASS_SETUP(ClassSetup) { InitializeJsonReader(); @@ -2089,4 +2093,152 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(2, termSettings.HistorySize()); } } + void SettingsTests::FindMissingProfile() + { + // Test that CascadiaSettings::FindProfile returns null for a GUID that + // doesn't exist + const std::string settingsString{ R"( + { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + const auto settingsJsonObj = VerifyParseSucceeded(settingsString); + auto settings = CascadiaSettings::FromJson(settingsJsonObj); + + 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 Profile* const profile1 = settings->FindProfile(guid1); + const Profile* const profile2 = settings->FindProfile(guid2); + const Profile* const profile3 = settings->FindProfile(guid3); + + VERIFY_IS_NOT_NULL(profile1); + VERIFY_IS_NOT_NULL(profile2); + VERIFY_IS_NULL(profile3); + + VERIFY_ARE_EQUAL(L"profile0", profile1->GetName()); + VERIFY_ARE_EQUAL(L"profile1", profile2->GetName()); + } + + void SettingsTests::MakeSettingsForProfileThatDoesntExist() + { + // Test that MakeSettings throws when the GUID doesn't exist + const std::string settingsString{ R"( + { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 1 + }, + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "historySize": 2 + } + ] + })" }; + const auto settingsJsonObj = VerifyParseSucceeded(settingsString); + auto settings = CascadiaSettings::FromJson(settingsJsonObj); + + 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}"); + + try + { + auto terminalSettings = settings->BuildSettings(guid1); + VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings); + VERIFY_ARE_EQUAL(1, terminalSettings.HistorySize()); + } + catch (...) + { + VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed"); + } + + try + { + auto terminalSettings = settings->BuildSettings(guid2); + VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings); + VERIFY_ARE_EQUAL(2, terminalSettings.HistorySize()); + } + catch (...) + { + VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed"); + } + + try + { + auto terminalSettings = settings->BuildSettings(guid3); + VERIFY_IS_TRUE(false, L"This call to BuildSettings should fail"); + } + catch (...) + { + VERIFY_IS_TRUE(true, L"This call to BuildSettings successfully failed"); + } + + try + { + const auto [guid, termSettings] = settings->BuildSettings(nullptr); + VERIFY_ARE_NOT_EQUAL(nullptr, termSettings); + VERIFY_ARE_EQUAL(1, termSettings.HistorySize()); + } + catch (...) + { + VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed"); + } + } + + void SettingsTests::MakeSettingsForDefaultProfileThatDoesntExist() + { + // Test that MakeSettings _doesnt_ throw when we load settings with a + // defaultProfile that's not in the list, we validate the settings, and + // then call MakeSettings(nullopt). The validation should ensure that + // the default profile is something reasonable + const std::string settingsString{ R"( + { + "defaultProfile": "{6239a42c-3333-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 1 + }, + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "historySize": 2 + } + ] + })" }; + const auto settingsJsonObj = VerifyParseSucceeded(settingsString); + auto settings = CascadiaSettings::FromJson(settingsJsonObj); + settings->_ValidateSettings(); + + VERIFY_ARE_EQUAL(1u, settings->_warnings.size()); + VERIFY_ARE_EQUAL(2u, settings->_profiles.size()); + VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid()); + try + { + const auto [guid, termSettings] = settings->BuildSettings(nullptr); + VERIFY_ARE_NOT_EQUAL(nullptr, termSettings); + VERIFY_ARE_EQUAL(1, termSettings.HistorySize()); + } + catch (...) + { + VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed"); + } + } + } diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index f900c4a4047..72b055654c8 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -53,6 +53,8 @@ namespace TerminalAppLocalTests TEST_METHOD(CreateSimpleTerminalXamlType); TEST_METHOD(CreateTerminalMuxXamlType); + TEST_METHOD(TryDuplicateBadTab); + TEST_CLASS_SETUP(ClassSetup) { InitializeJsonReader(); @@ -164,4 +166,97 @@ namespace TerminalAppLocalTests VERIFY_SUCCEEDED(result); } + void TabTests::TryDuplicateBadTab() + { + // Create a tab with a profile with GUID A + // Reload the settings so that GUID A is no longer in the list of profiles + // Try calling _DuplicateTabViewItem on tab A + // No new tab should be created (and more importantly, the app should not crash) + + // This is a tests that was inspired by GH#2455, but at the time, + // GH#2472 was still not solved, so this test was not possible to be + // authored. + + const std::string settingsJson0{ R"( + { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 1 + }, + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "historySize": 2 + } + ] + })" }; + // const auto settings0JsonObj = VerifyParseSucceeded(settings0String); + // auto settings0 = CascadiaSettings::FromJson(settings0JsonObj); + + const std::string settingsJson1{ R"( + { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "historySize": 2 + } + ] + })" }; + // const auto settings1JsonObj = VerifyParseSucceeded(settings1String); + // auto settings1 = CascadiaSettings::FromJson(settings1JsonObj); + + VerifyParseSucceeded(settingsJson0); + auto settings0 = std::make_shared(false); + VERIFY_IS_NOT_NULL(settings0); + settings0->_ParseJsonString(settingsJson0, false); + settings0->LayerJson(settings0->_userSettings); + settings0->_ValidateSettings(); + + VerifyParseSucceeded(settingsJson1); + auto settings1 = std::make_shared(false); + VERIFY_IS_NOT_NULL(settings1); + settings1->_ParseJsonString(settingsJson1, false); + settings1->LayerJson(settings1->_userSettings); + settings1->_ValidateSettings(); + + 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}"); + + winrt::com_ptr page{ nullptr }; + + auto result = RunOnUIThread([&page, settings0]() { + // auto foo{ winrt::make_self() }; + winrt::TerminalApp::TerminalPage foo{}; + // VERIFY_IS_NOT_NULL(foo); + page.copy_from(winrt::get_self(foo)); + page->_settings = settings0; + }); + VERIFY_SUCCEEDED(result); + + VERIFY_IS_NOT_NULL(page); + VERIFY_IS_NOT_NULL(page->_settings); + + winrt::TerminalApp::TerminalPage projectedPage = *page; + + // page->Create(); + result = RunOnUIThread([&page]() { + VERIFY_IS_NOT_NULL(page); + VERIFY_IS_NOT_NULL(page->_settings); + DebugBreak(); + page->Create(); + }); + VERIFY_SUCCEEDED(result); + + result = RunOnUIThread([&page]() { + VERIFY_ARE_EQUAL(1u, page->_tabs.size()); + }); + VERIFY_SUCCEEDED(result); + } + } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 82249475566..d8f3b3501db 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -397,7 +397,12 @@ namespace winrt::TerminalApp::implementation // add static items { - const auto isUwp = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsUwp(); + auto isUwp = false; + try + { + isUwp = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsUwp(); + } + CATCH_LOG(); if (!isUwp) { From 029b67633718ca525e313fdca20729d5f1519f61 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 12:04:24 -0600 Subject: [PATCH 3/8] clean up these tests for PR --- .../LocalTests_TerminalApp/TabTests.cpp | 69 +++++++++++++------ 1 file changed, 49 insertions(+), 20 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 72b055654c8..fe6e67ef444 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -168,14 +168,12 @@ namespace TerminalAppLocalTests void TabTests::TryDuplicateBadTab() { - // Create a tab with a profile with GUID A - // Reload the settings so that GUID A is no longer in the list of profiles - // Try calling _DuplicateTabViewItem on tab A + // Create a tab with a profile with GUID 1 + // Reload the settings so that GUID 1 is no longer in the list of profiles + // Try calling _DuplicateTabViewItem on tab 1 // No new tab should be created (and more importantly, the app should not crash) - - // This is a tests that was inspired by GH#2455, but at the time, - // GH#2472 was still not solved, so this test was not possible to be - // authored. + // + // Created to test GH#2455 const std::string settingsJson0{ R"( { @@ -193,8 +191,6 @@ namespace TerminalAppLocalTests } ] })" }; - // const auto settings0JsonObj = VerifyParseSucceeded(settings0String); - // auto settings0 = CascadiaSettings::FromJson(settings0JsonObj); const std::string settingsJson1{ R"( { @@ -207,8 +203,6 @@ namespace TerminalAppLocalTests } ] })" }; - // const auto settings1JsonObj = VerifyParseSucceeded(settings1String); - // auto settings1 = CascadiaSettings::FromJson(settings1JsonObj); VerifyParseSucceeded(settingsJson0); auto settings0 = std::make_shared(false); @@ -228,13 +222,21 @@ namespace TerminalAppLocalTests 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}"); + // This is super wacky, but we can't just initialize the + // com_ptr in the lambda and assign it back out of + // the lambda. We'll crash trying to get a weak_ref to the TerminalPage + // during TerminalPage::Create() below. + // + // Instead, create the winrt object, then get a com_ptr to the + // implementation _from_ the winrt object. This seems to work, even if + // it's weird. + winrt::TerminalApp::TerminalPage projectedPage{ nullptr }; winrt::com_ptr page{ nullptr }; - auto result = RunOnUIThread([&page, settings0]() { - // auto foo{ winrt::make_self() }; - winrt::TerminalApp::TerminalPage foo{}; - // VERIFY_IS_NOT_NULL(foo); - page.copy_from(winrt::get_self(foo)); + Log::Comment(NoThrowString().Format(L"Construct the TerminalPage")); + auto result = RunOnUIThread([&projectedPage, &page, settings0]() { + projectedPage = winrt::TerminalApp::TerminalPage(); + page.copy_from(winrt::get_self(projectedPage)); page->_settings = settings0; }); VERIFY_SUCCEEDED(result); @@ -242,14 +244,19 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); - winrt::TerminalApp::TerminalPage projectedPage = *page; - - // page->Create(); + Log::Comment(NoThrowString().Format(L"Create() the TerminalPage")); result = RunOnUIThread([&page]() { VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); - DebugBreak(); page->Create(); + + // I think in the tests, we don't always set the focused tab on + // creation. Doesn't seem to be a problem in the real app, but + // probably indicative of a problem. + // + // Manually set it here, so that later, the _GetFocusedTabIndex call + // in _DuplicateTabViewItem will have a sensible value. + page->_SetFocusedTabIndex(0); }); VERIFY_SUCCEEDED(result); @@ -257,6 +264,28 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(1u, page->_tabs.size()); }); VERIFY_SUCCEEDED(result); + + Log::Comment(NoThrowString().Format(L"Duplicate the first tab")); + result = RunOnUIThread([&page]() { + page->_DuplicateTabViewItem(); + VERIFY_ARE_EQUAL(2u, page->_tabs.size()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(NoThrowString().Format( + L"Change the settings of the TerminalPage so the first profile is " + L"no longer in the list of profiles")); + result = RunOnUIThread([&page, settings1]() { + page->_settings = settings1; + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(NoThrowString().Format(L"Duplicate the tab, and don't crash")); + result = RunOnUIThread([&page]() { + page->_DuplicateTabViewItem(); + VERIFY_ARE_EQUAL(2u, page->_tabs.size(), L"We should gracefully do nothing here - the profile no longer exists."); + }); + VERIFY_SUCCEEDED(result); } } From 0a6f72c6dea793f8ea0f2c229f07e8af98ebf8c5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 12:17:53 -0600 Subject: [PATCH 4/8] Fix the tests o__o --- src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp | 2 +- src/cascadia/LocalTests_TerminalApp/TabTests.cpp | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index 15ddbadcb4a..393ffe4184d 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -2226,7 +2226,7 @@ namespace TerminalAppLocalTests auto settings = CascadiaSettings::FromJson(settingsJsonObj); settings->_ValidateSettings(); - VERIFY_ARE_EQUAL(1u, settings->_warnings.size()); + VERIFY_ARE_EQUAL(2u, settings->_warnings.size()); VERIFY_ARE_EQUAL(2u, settings->_profiles.size()); VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid()); try diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index fe6e67ef444..9df3dfb34f4 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -15,6 +15,7 @@ using namespace Microsoft::Console; using namespace TerminalApp; using namespace WEX::Logging; using namespace WEX::TestExecution; +using namespace WEX::Common; namespace TerminalAppLocalTests { From 963a83024142d5ebabcafef0e64115f23ce62631 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 16:43:36 -0600 Subject: [PATCH 5/8] Add the VERIFY_THROWS for dustin --- src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index 393ffe4184d..fd91be52b48 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -2178,15 +2178,7 @@ namespace TerminalAppLocalTests VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed"); } - try - { - auto terminalSettings = settings->BuildSettings(guid3); - VERIFY_IS_TRUE(false, L"This call to BuildSettings should fail"); - } - catch (...) - { - VERIFY_IS_TRUE(true, L"This call to BuildSettings successfully failed"); - } + VERIFY_THROWS(auto terminalSettings = settings->BuildSettings(guid3), wil::ResultException, L"This call to BuildSettings should fail"); try { From 82ad8bba3c62996af805256661aad617ede1f2b5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 12:49:42 -0500 Subject: [PATCH 6/8] I _think_ this will pull the settings from the control, but I haven't tested yet --- src/cascadia/TerminalApp/TerminalPage.cpp | 14 +++++++++++--- src/cascadia/TerminalControl/TermControl.cpp | 5 +++++ src/cascadia/TerminalControl/TermControl.h | 2 ++ src/cascadia/TerminalControl/TermControl.idl | 6 +++--- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 8eedf1abcc1..7c70703ef37 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -827,11 +827,19 @@ namespace winrt::TerminalApp::implementation { auto focusedTab = _GetStrongTabImpl(*index); const auto& profileGuid = focusedTab->GetFocusedProfile(); - if (profileGuid.has_value()) + auto activeControl = focusedTab->GetActiveTerminalControl(); + auto iControlSettings = activeControl.Settings(); + if (profileGuid.has_value() && iControlSettings) { - const auto settings = _settings->BuildSettings(profileGuid.value()); - _CreateNewTabFromSettings(profileGuid.value(), settings); + if (auto settings{ iControlSettings.as() }) + { + _CreateNewTabFromSettings(profileGuid.value(), settings); + } } + // if (profileGuid.has_value()) + // { + // const auto settings = _settings->BuildSettings(profileGuid.value()); + // } } CATCH_LOG(); // TODO: Should we display a dialog when we do nothing because we diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 83df752073f..d75e149f304 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2352,6 +2352,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation e.DragUIOverride().IsGlyphVisible(false); } + winrt::Microsoft::Terminal::Settings::IControlSettings TermControl::Settings() const + { + return _settings; + } + // -------------------------------- WinRT Events --------------------------------- // Winrt events need a method for adding a callback to the event and removing the callback. // These macros will define them both for you. diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 57606506bee..469bd46ece3 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -94,6 +94,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation static Windows::Foundation::Point GetProposedDimensions(Microsoft::Terminal::Settings::IControlSettings const& settings, const uint32_t dpi); + Settings::IControlSettings Settings() const; + // clang-format off // -------------------------------- WinRT Events --------------------------------- DECLARE_EVENT(TitleChanged, _titleChangedHandlers, TerminalControl::TitleChangedEventArgs); diff --git a/src/cascadia/TerminalControl/TermControl.idl b/src/cascadia/TerminalControl/TermControl.idl index a005ac16d5a..d56c3f39acb 100644 --- a/src/cascadia/TerminalControl/TermControl.idl +++ b/src/cascadia/TerminalControl/TermControl.idl @@ -12,9 +12,7 @@ namespace Microsoft.Terminal.TerminalControl // If you update this one, please update TerminalApp\IF7Listener.idl. // If you change this interface, please update the guid. // If you press F7 and get a runtime error, go make sure both copies are the same. - [uuid("339e1a87-5315-4da6-96f0-565549b6472b")] - interface IF7Listener - { + [uuid("339e1a87-5315-4da6-96f0-565549b6472b")] interface IF7Listener { Boolean OnF7Pressed(); } @@ -68,5 +66,7 @@ namespace Microsoft.Terminal.TerminalControl void AdjustFontSize(Int32 fontSizeDelta); void ResetFontSize(); + + Microsoft.Terminal.Settings.IControlSettings Settings { get; }; } } From eafe9c8fbce329988d5708c8a80bad8be3d2b079 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 15:23:01 -0500 Subject: [PATCH 7/8] Wrap Application::Current in try/catch since that breaks tests --- src/cascadia/TerminalApp/TerminalPage.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 7c70703ef37..d8b0b1bd672 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -63,11 +63,20 @@ namespace winrt::TerminalApp::implementation _tabView = _tabRow.TabView(); _rearranging = false; - // GH#3581 - There's a platform limitation that causes us to crash when we rearrange tabs. - // Xaml tries to send a drag visual (to wit: a screenshot) to the drag hosting process, - // but that process is running at a different IL than us. - // For now, we're disabling elevated drag. - const auto isElevated = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsElevated(); + // GH#2455 - Make sure to try/catch calls to Application::Current, + // because that _won't_ be an instance of TerminalApp::App in the + // LocalTests + auto isElevated = false; + try + { + // GH#3581 - There's a platform limitation that causes us to crash when we rearrange tabs. + // Xaml tries to send a drag visual (to wit: a screenshot) to the drag hosting process, + // but that process is running at a different IL than us. + // For now, we're disabling elevated drag. + isElevated = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsUwp(); + } + CATCH_LOG(); + _tabView.CanReorderTabs(!isElevated); _tabView.CanDragTabs(!isElevated); @@ -406,6 +415,9 @@ namespace winrt::TerminalApp::implementation // add static items { + // GH#2455 - Make sure to try/catch calls to Application::Current, + // because that _won't_ be an instance of TerminalApp::App in the + // LocalTests auto isUwp = false; try { From 908fd74c6762dae3cf7d1b0b545c7a1ae01b5d4d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 15:23:27 -0500 Subject: [PATCH 8/8] Try to clone the settings directly from the control Okay this is harder, but maybe not impossible, but probably impossible in v1. The Profile knows how to build the connection settings, and the ControlSettings, etc. However, when the settings reloads like in this bug, we'll remove all the old profiles, and build a new list. If we wanted the case 1 behavior, we could wait until the Profile is a winrt object, then have each Pane hold a strong ref to the Profile that it's hosting. Then it would be trivial to be able to duplicate the settings from it. I believe that work is tracked in #3998. How palatable is doing case 3 now (to fix the crash), then filing a follow up to do case 1 once #3998 is done? --- .../LocalTests_TerminalApp/TabTests.cpp | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 285b9df105a..45ed557a342 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -169,10 +169,11 @@ namespace TerminalAppLocalTests void TabTests::TryDuplicateBadTab() { - // Create a tab with a profile with GUID 1 - // Reload the settings so that GUID 1 is no longer in the list of profiles - // Try calling _DuplicateTabViewItem on tab 1 - // No new tab should be created (and more importantly, the app should not crash) + // * Create a tab with a profile with GUID 1 + // * Reload the settings so that GUID 1 is no longer in the list of profiles + // * Try calling _DuplicateTabViewItem on tab 1 + // * The tab that's created should use the old historySize value from the + // duplicated tab. More importantly, the app should not crash. // // Created to test GH#2455 @@ -249,7 +250,9 @@ namespace TerminalAppLocalTests result = RunOnUIThread([&page]() { VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); + page->Create(); + Log::Comment(NoThrowString().Format(L"Create()'d")); // I think in the tests, we don't always set the focused tab on // creation. Doesn't seem to be a problem in the real app, but @@ -258,18 +261,19 @@ namespace TerminalAppLocalTests // Manually set it here, so that later, the _GetFocusedTabIndex call // in _DuplicateTabViewItem will have a sensible value. page->_SetFocusedTabIndex(0); + Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()'d")); }); VERIFY_SUCCEEDED(result); result = RunOnUIThread([&page]() { - VERIFY_ARE_EQUAL(1u, page->_tabs.size()); + VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); }); VERIFY_SUCCEEDED(result); Log::Comment(NoThrowString().Format(L"Duplicate the first tab")); result = RunOnUIThread([&page]() { page->_DuplicateTabViewItem(); - VERIFY_ARE_EQUAL(2u, page->_tabs.size()); + VERIFY_ARE_EQUAL(2u, page->_tabs.Size()); }); VERIFY_SUCCEEDED(result); @@ -284,7 +288,14 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Duplicate the tab, and don't crash")); result = RunOnUIThread([&page]() { page->_DuplicateTabViewItem(); - 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 duplicate the settings of the second " + L"tab here, despite the profile no longer existing"); + + auto control = page->_GetStrongTabImpl(2)->GetActiveTerminalControl(); + VERIFY_IS_NOT_NULL(control); + VERIFY_ARE_EQUAL(1, control.Settings().HistorySize()); }); VERIFY_SUCCEEDED(result); }