From a8bd5036f0d85d9910e56afb86664b8bca9c0d4e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 11:34:14 -0600 Subject: [PATCH 01/32] 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 02/32] 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 03/32] 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 04/32] 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 05/32] 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 06/32] 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 07/32] 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 08/32] 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); } From 4561b9a29cf3283a58d7e0b60573ff492f0ea302 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 16:13:34 -0500 Subject: [PATCH 09/32] Revert "I _think_ this will pull the settings from the control, but I haven't tested yet" This reverts commit 82ad8bba3c62996af805256661aad617ede1f2b5. --- 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, 6 insertions(+), 21 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index d8b0b1bd672..2b734081795 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -839,19 +839,11 @@ namespace winrt::TerminalApp::implementation { auto focusedTab = _GetStrongTabImpl(*index); const auto& profileGuid = focusedTab->GetFocusedProfile(); - auto activeControl = focusedTab->GetActiveTerminalControl(); - auto iControlSettings = activeControl.Settings(); - if (profileGuid.has_value() && iControlSettings) + if (profileGuid.has_value()) { - if (auto settings{ iControlSettings.as() }) - { - _CreateNewTabFromSettings(profileGuid.value(), settings); - } + const auto settings = _settings->BuildSettings(profileGuid.value()); + _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 d75e149f304..83df752073f 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2352,11 +2352,6 @@ 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 469bd46ece3..57606506bee 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -94,8 +94,6 @@ 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 d56c3f39acb..a005ac16d5a 100644 --- a/src/cascadia/TerminalControl/TermControl.idl +++ b/src/cascadia/TerminalControl/TermControl.idl @@ -12,7 +12,9 @@ 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(); } @@ -66,7 +68,5 @@ namespace Microsoft.Terminal.TerminalControl void AdjustFontSize(Int32 fontSizeDelta); void ResetFontSize(); - - Microsoft.Terminal.Settings.IControlSettings Settings { get; }; } } From 9eaee5297ff5965617563c7149a4d07cae5b643b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 16:13:53 -0500 Subject: [PATCH 10/32] Revert "Try to clone the settings directly from the control" This reverts commit 908fd74c6762dae3cf7d1b0b545c7a1ae01b5d4d. --- .../LocalTests_TerminalApp/TabTests.cpp | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 45ed557a342..285b9df105a 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -169,11 +169,10 @@ 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 - // * The tab that's created should use the old historySize value from the - // duplicated tab. 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 + // No new tab should be created (and more importantly, the app should not crash) // // Created to test GH#2455 @@ -250,9 +249,7 @@ 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 @@ -261,19 +258,18 @@ 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); @@ -288,14 +284,7 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Duplicate the tab, and don't crash")); result = RunOnUIThread([&page]() { page->_DuplicateTabViewItem(); - 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_ARE_EQUAL(2u, page->_tabs.size(), L"We should gracefully do nothing here - the profile no longer exists."); }); VERIFY_SUCCEEDED(result); } From e952c0d392cb6bf48083c50c3069e691711c64bf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 16:20:02 -0500 Subject: [PATCH 11/32] fix the test so it compiles again --- src/cascadia/LocalTests_TerminalApp/TabTests.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 285b9df105a..ea7f322bbbf 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -169,10 +169,10 @@ 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 + // * No new tab should be created (and more importantly, the app should not crash) // // Created to test GH#2455 @@ -262,14 +262,14 @@ namespace TerminalAppLocalTests 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 +284,7 @@ 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(2u, page->_tabs.Size(), L"We should gracefully do nothing here - the profile no longer exists."); }); VERIFY_SUCCEEDED(result); } From 07050b5bec9ef1d7ab0ef0462b3c2ce546595959 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 19 Mar 2020 09:34:24 -0500 Subject: [PATCH 12/32] add a note on what's going wrong here --- .../LocalTests_TerminalApp/TabTests.cpp | 140 ++++++++++++++++++ src/cascadia/TerminalApp/Pane.cpp | 5 + src/cascadia/TerminalApp/Pane.h | 2 + src/cascadia/TerminalApp/Tab.cpp | 5 + src/cascadia/TerminalApp/Tab.h | 10 ++ 5 files changed, 162 insertions(+) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index ea7f322bbbf..9593c475c54 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -13,6 +13,7 @@ using namespace Microsoft::Console; using namespace TerminalApp; +using namespace winrt::TerminalApp; using namespace WEX::Logging; using namespace WEX::TestExecution; using namespace WEX::Common; @@ -55,6 +56,7 @@ namespace TerminalAppLocalTests TEST_METHOD(CreateTerminalMuxXamlType); TEST_METHOD(TryDuplicateBadTab); + TEST_METHOD(TryDuplicateBadPane); TEST_CLASS_SETUP(ClassSetup) { @@ -289,4 +291,142 @@ namespace TerminalAppLocalTests VERIFY_SUCCEEDED(result); } + void TabTests::TryDuplicateBadPane() + { + // * 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 _SplitPane(Duplicate) on tab 1 + // * No new pane should be created (and more importantly, the app should not crash) + // + // Created to test GH#2455 + + 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 std::string settingsJson1{ R"( + { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "historySize": 2 + } + ] + })" }; + + 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}"); + + // 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 }; + + 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); + + VERIFY_IS_NOT_NULL(page); + VERIFY_IS_NOT_NULL(page->_settings); + + Log::Comment(NoThrowString().Format(L"Create() the TerminalPage")); + result = RunOnUIThread([&page]() { + VERIFY_IS_NOT_NULL(page); + VERIFY_IS_NOT_NULL(page->_settings); + 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); + + result = RunOnUIThread([&page]() { + VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); + auto tab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(1, tab->_GetLeafPaneCount()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(NoThrowString().Format(L"Duplicate the first pane")); + result = RunOnUIThread([&page]() { + // Oh no I didn't actually duplicate the pane, what did I do + DebugBreak(); + // The problem here is that the pane doesn't actually have a real + // size yet. It thinks it's 0x0, which it is. We either need to + // - 1. trick the test into thinking the pane has a real size + // - 2. allow panes to be split regardless of their minimum size + page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, nullptr); + + VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); + auto tab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, tab->_GetLeafPaneCount()); + }); + 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 pane, and don't crash")); + result = RunOnUIThread([&page]() { + page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, nullptr); + + VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); + auto tab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, + tab->_GetLeafPaneCount(), + L"We should gracefully do nothing here - the profile no longer exists."); + }); + VERIFY_SUCCEEDED(result); + } + } diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 56c13bf0d14..a9b1aa18db6 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1522,4 +1522,9 @@ void Pane::_SetupResources() } } +int Pane::GetLeafPaneCount() const noexcept +{ + return _IsLeaf() ? 1 : (_firstChild->GetLeafPaneCount() + _secondChild->GetLeafPaneCount()); +} + DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate>); diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 84bf041fe31..7a97c2116c2 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -67,6 +67,8 @@ class Pane : public std::enable_shared_from_this void Shutdown(); void Close(); + int GetLeafPaneCount() const noexcept; + WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); DECLARE_EVENT(GotFocus, _GotFocusHandlers, winrt::delegate>); diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index e2377508815..ccc27636f1c 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -420,5 +420,10 @@ namespace winrt::TerminalApp::implementation }); } + int Tab::_GetLeafPaneCount() const noexcept + { + return _rootPane->GetLeafPaneCount(); + } + DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); } diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index af72398ffa0..ab0998a116a 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -5,6 +5,12 @@ #include "Pane.h" #include "Tab.g.h" +// fwdecl unittest classes +namespace TerminalAppLocalTests +{ + class TabTests; +}; + namespace winrt::TerminalApp::implementation { struct Tab : public TabT @@ -64,5 +70,9 @@ namespace winrt::TerminalApp::implementation void _AttachEventHandlersToControl(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); void _AttachEventHandlersToPane(std::shared_ptr pane); + + int _GetLeafPaneCount() const noexcept; + + friend class ::TerminalAppLocalTests::TabTests; }; } From 4bb680362416776dc6b4e750a0a64d2e2f667ca5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 19 Mar 2020 11:47:58 -0500 Subject: [PATCH 13/32] I have no idea why but this seemed to work to initialize the panes --- src/cascadia/TerminalApp/Pane.cpp | 17 +++-- src/cascadia/TerminalApp/TerminalPage.cpp | 86 ++++++++++++++++++----- src/cascadia/TerminalApp/TerminalPage.h | 10 +++ src/cascadia/WindowsTerminal/AppHost.cpp | 11 ++- 4 files changed, 101 insertions(+), 23 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index a9b1aa18db6..636e3bec1bf 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -795,6 +795,8 @@ void Pane::_SetupChildCloseHandlers() // - void Pane::_CreateRowColDefinitions(const Size& rootSize) { + auto first = _desiredSplitPosition * 100.0f; + auto second = 100.0f - first; if (_splitState == SplitState::Vertical) { _root.ColumnDefinitions().Clear(); @@ -803,10 +805,12 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize) const auto paneSizes = _CalcChildrenSizes(rootSize.Width); auto firstColDef = Controls::ColumnDefinition(); - firstColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); + // firstColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); + firstColDef.Width(GridLengthHelper::FromValueAndType(first, GridUnitType::Star)); auto secondColDef = Controls::ColumnDefinition(); - secondColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.second, GridUnitType::Star)); + // secondColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.second, GridUnitType::Star)); + secondColDef.Width(GridLengthHelper::FromValueAndType(second, GridUnitType::Star)); _root.ColumnDefinitions().Append(firstColDef); _root.ColumnDefinitions().Append(secondColDef); @@ -819,10 +823,12 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize) const auto paneSizes = _CalcChildrenSizes(rootSize.Height); auto firstRowDef = Controls::RowDefinition(); - firstRowDef.Height(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); + firstRowDef.Height(GridLengthHelper::FromValueAndType(first, GridUnitType::Star)); + // firstRowDef.Height(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); auto secondRowDef = Controls::RowDefinition(); - secondRowDef.Height(GridLengthHelper::FromValueAndType(paneSizes.second, GridUnitType::Star)); + secondRowDef.Height(GridLengthHelper::FromValueAndType(second, GridUnitType::Star)); + // secondRowDef.Height(GridLengthHelper::FromValueAndType(paneSizes.second, GridUnitType::Star)); _root.RowDefinitions().Append(firstRowDef); _root.RowDefinitions().Append(secondRowDef); @@ -1002,6 +1008,9 @@ bool Pane::_CanSplit(SplitState splitType) const Size actualSize{ gsl::narrow_cast(_root.ActualWidth()), gsl::narrow_cast(_root.ActualHeight()) }; + const Size desiredSize = _root.DesiredSize(); + desiredSize; + const Size minSize = _GetMinSize(); auto actualSplitType = _convertAutomaticSplitState(splitType); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 2b734081795..ea0961fb968 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -146,20 +146,64 @@ namespace winrt::TerminalApp::implementation _tabContent.SizeChanged({ this, &TerminalPage::_OnContentSizeChanged }); - // Actually start the terminal. - if (_appArgs.GetStartupActions().empty()) - { - _OpenNewTab(nullptr); - } - else - { - _appArgs.ValidateStartupCommands(); - - // This will kick off a chain of events to perform each startup - // action. As each startup action is completed, the next will be - // fired. - _ProcessNextStartupAction(); - } + // //////////////////////////////////////////////////////////////////////// + // // Actually start the terminal. + // if (_appArgs.GetStartupActions().empty()) + // { + // _OpenNewTab(nullptr); + // } + // else + // { + // _appArgs.ValidateStartupCommands(); + + // // This will kick off a chain of events to perform each startup + // // action. As each startup action is completed, the next will be + // // fired. + // _ProcessNextStartupAction(); + // } + // //////////////////////////////////////////////////////////////////////// + + //////////////////////////////////////////////////////////////////////// + // Initialize the terminal only once the swapchainpanel is loaded - that + // way, we'll be able to query the real pixel size it got on layout + // _layoutUpdatedRevoker = this->LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { + _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { + Windows::Foundation::Size actualSize{ gsl::narrow_cast(_tabContent.ActualWidth()), + gsl::narrow_cast(_tabContent.ActualHeight()) }; + Windows::Foundation::Size desiredSize = _tabContent.DesiredSize(); + actualSize; + desiredSize; + + // Only let this succeed once. + this->_layoutUpdatedRevoker.revoke(); + + // This event fires every time the layout changes, but it is always the last one to fire + // in any layout change chain. That gives us great flexibility in finding the right point + // at which to initialize our renderer (and our terminal). + // Any earlier than the last layout update and we may not know the terminal's starting size. + if (_startupState == StartupState::NotInitialized) + { + _startupState = StartupState::InStartup; + if (_appArgs.GetStartupActions().size() == 0) + { + _OpenNewTab(nullptr); + _startupState = StartupState::Initialized; + } + else + { + Dispatcher().RunAsync(CoreDispatcherPriority::Low, [this]() { + for (const auto& action : _appArgs.GetStartupActions()) + { + // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { + _actionDispatch->DoAction(action); + // }); + } + _startupState = StartupState::Initialized; + }); + } + } + }); + //////////////////////////////////////////////////////////////////////// } // Method Description: @@ -600,7 +644,17 @@ namespace winrt::TerminalApp::implementation if (isFirstTab) { _tabContent.Children().Clear(); - _tabContent.Children().Append(newTabImpl->GetRootElement()); + const Windows::Foundation::Size actualSize{ gsl::narrow_cast(_tabContent.ActualWidth()), + gsl::narrow_cast(_tabContent.ActualHeight()) }; + auto tabRootElem = newTabImpl->GetRootElement(); + _tabContent.Children().Append(tabRootElem); + tabRootElem.Measure(actualSize); + Windows::Foundation::Size desiredSize = tabRootElem.DesiredSize(); + desiredSize; + ; + auto a = 0; + a += 1; + a; } else { @@ -1135,7 +1189,7 @@ namespace winrt::TerminalApp::implementation const auto canSplit = focusedTab->CanSplitPane(splitType); - if (!canSplit) + if (!canSplit && _startupState == StartupState::Initialized) { return; } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 684d55495f1..1ec03daa244 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -20,6 +20,13 @@ namespace TerminalAppLocalTests namespace winrt::TerminalApp::implementation { + enum StartupState : int + { + NotInitialized = 0, + InStartup = 1, + Initialized = 2 + }; + struct TerminalPage : TerminalPageT { public: @@ -75,6 +82,9 @@ namespace winrt::TerminalApp::implementation winrt::com_ptr _actionDispatch{ winrt::make_self() }; + winrt::Windows::UI::Xaml::Controls::Grid::LayoutUpdated_revoker _layoutUpdatedRevoker; + StartupState _startupState{ StartupState::NotInitialized }; + ::TerminalApp::AppCommandlineArgs _appArgs; int _ParseArgs(winrt::array_view& args); fire_and_forget _ProcessNextStartupAction(); diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 2959b22220e..326ddb4cc80 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -92,11 +92,16 @@ void AppHost::_HandleCommandlineArgs() int argc = 0; // Get the argv, and turn them into a hstring array to pass to the app. - wil::unique_any argv{ CommandLineToArgvW(commandline, &argc) }; - if (argv) + // std::vector argv{ L"wt.exe", L";", L"split-pane" }; + std::vector argv{ L"wt.exe", L";", L"split-pane", L";", L"split-pane", L"-p", L"cmd", L";", L"split-pane", L"-p", L"mode" }; + argc; + // wil::unique_any argv{ CommandLineToArgvW(commandline, &argc) }; + // if (argv) + if (argv.size() > 0) { std::vector args; - for (auto& elem : wil::make_range(argv.get(), argc)) + // for (auto& elem : wil::make_range(argv.get(), argc)) + for (auto& elem : argv) { args.emplace_back(elem); } From 499b003765b5bf17e483149b54bd57ca328d6ff5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 19 Mar 2020 12:20:36 -0500 Subject: [PATCH 14/32] immediately focus new panes --- src/cascadia/TerminalApp/Tab.cpp | 31 +++++++++++++++++++++---------- src/cascadia/TerminalApp/Tab.h | 1 + 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index ccc27636f1c..0ffb84f1d50 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -277,6 +277,12 @@ namespace winrt::TerminalApp::implementation // gains focus, we'll mark it as the new active pane. _AttachEventHandlersToPane(first); _AttachEventHandlersToPane(second); + + // Immediately update our tracker of the focused pane now. If we're + // splitting panes during startup (from a commandline), then it's + // possible that the focus events won't propogate immediately. Updating + // the focus here will give the same effect though. + _UpdateActivePane(second); } // Method Description: @@ -387,6 +393,20 @@ namespace winrt::TerminalApp::implementation }); } + void Tab::_UpdateActivePane(std::shared_ptr pane) + { + // Clear the active state of the entire tree, and mark only the pane as active. + _rootPane->ClearActive(); + _activePane = pane; + _activePane->SetActive(); + + // Update our own title text to match the newly-active pane. + SetTabText(GetActiveTitle()); + + // Raise our own ActivePaneChanged event. + _ActivePaneChangedHandlers(); + } + // Method Description: // - Add an event handler to this pane's GotFocus event. When that pane gains // focus, we'll mark it as the new active pane. We'll also query the title of @@ -406,16 +426,7 @@ namespace winrt::TerminalApp::implementation if (tab && sender != tab->_activePane) { - // Clear the active state of the entire tree, and mark only the sender as active. - tab->_rootPane->ClearActive(); - tab->_activePane = sender; - tab->_activePane->SetActive(); - - // Update our own title text to match the newly-active pane. - tab->SetTabText(tab->GetActiveTitle()); - - // Raise our own ActivePaneChanged event. - tab->_ActivePaneChangedHandlers(); + tab->_UpdateActivePane(sender); } }); } diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index ab0998a116a..f001d1a1fd4 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -72,6 +72,7 @@ namespace winrt::TerminalApp::implementation void _AttachEventHandlersToPane(std::shared_ptr pane); int _GetLeafPaneCount() const noexcept; + void _UpdateActivePane(std::shared_ptr pane); friend class ::TerminalAppLocalTests::TabTests; }; From 95bb854f05b9eab889da62f4eac10e14b3bf4145 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 19 Mar 2020 14:17:24 -0500 Subject: [PATCH 15/32] this works to alternate auto snapping, but maybe there's a better way --- src/cascadia/TerminalApp/Pane.cpp | 28 ++++++++++++++++++++++++++-- src/cascadia/TerminalApp/Pane.h | 4 ++++ src/cascadia/TerminalApp/Tab.cpp | 4 ++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 636e3bec1bf..858ad79d394 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -959,11 +959,21 @@ std::pair, std::shared_ptr> Pane::Split(SplitState s { if (_firstChild->_HasFocusedChild()) { - return _firstChild->Split(splitType, profile, control); + auto pretranslatedSplitType = splitType; + if (_firstChild->_IsLeaf() && _isSizeZero()) + { + pretranslatedSplitType = _preTranslateAutoSplitType(splitType); + } + return _firstChild->Split(pretranslatedSplitType, profile, control); } else if (_secondChild->_HasFocusedChild()) { - return _secondChild->Split(splitType, profile, control); + auto pretranslatedSplitType = splitType; + if (_secondChild->_IsLeaf() && _isSizeZero()) + { + pretranslatedSplitType = _preTranslateAutoSplitType(splitType); + } + return _secondChild->Split(pretranslatedSplitType, profile, control); } return { nullptr, nullptr }; @@ -997,6 +1007,20 @@ SplitState Pane::_convertAutomaticSplitState(const SplitState& splitType) const return splitType; } +SplitState Pane::_preTranslateAutoSplitType(const SplitState& splitType) const +{ + if (splitType == SplitState::Automatic && _isSizeZero()) + { + return _splitState == SplitState::Horizontal ? SplitState::Vertical : SplitState::Horizontal; + } + return splitType; +} + +bool Pane::_isSizeZero() const +{ + return _root.ActualWidth() == 0 && _root.ActualHeight() == 0; +} + // Method Description: // - Determines whether the pane can be split. // Arguments: diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 7a97c2116c2..940246367bb 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -135,6 +135,10 @@ class Pane : public std::enable_shared_from_this float _ClampSplitPosition(const bool widthOrHeight, const float requestedValue, const float totalSize) const; winrt::TerminalApp::SplitState _convertAutomaticSplitState(const winrt::TerminalApp::SplitState& splitType) const; + + winrt::TerminalApp::SplitState _preTranslateAutoSplitType(const winrt::TerminalApp::SplitState& splitType) const; + bool _isSizeZero() const; + // Function Description: // - Returns true if the given direction can be used with the given split // type. diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 0ffb84f1d50..d273b214dbd 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -269,8 +269,8 @@ namespace winrt::TerminalApp::implementation // - void Tab::SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, TermControl& control) { - auto [first, second] = _activePane->Split(splitType, profile, control); - _activePane = first; + // Use the _rootPane to determine who to split. When we're initializing multiple panes on startup, this is + auto [first, second] = _rootPane->Split(splitType, profile, control); _AttachEventHandlersToControl(control); // Add a event handlers to the new panes' GotFocus event. When the pane From a058181b42d0d7d86aefb00e2e55c2cb936197e0 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 19 Mar 2020 16:37:24 -0500 Subject: [PATCH 16/32] precompute auto sizing correctly --- src/cascadia/TerminalApp/Pane.cpp | 37 +++++++++++++++++++++-- src/cascadia/TerminalApp/Pane.h | 2 ++ src/cascadia/TerminalApp/Tab.cpp | 15 +++++++-- src/cascadia/TerminalApp/Tab.h | 2 ++ src/cascadia/TerminalApp/TerminalPage.cpp | 10 +++++- 5 files changed, 60 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 858ad79d394..611a2a5241d 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -793,7 +793,7 @@ void Pane::_SetupChildCloseHandlers() // - rootSize: The dimensions in pixels that this pane (and its children should consume.) // Return Value: // - -void Pane::_CreateRowColDefinitions(const Size& rootSize) +void Pane::_CreateRowColDefinitions(const Size& /*rootSize*/) { auto first = _desiredSplitPosition * 100.0f; auto second = 100.0f - first; @@ -802,7 +802,7 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize) _root.ColumnDefinitions().Clear(); // Create two columns in this grid: one for each pane - const auto paneSizes = _CalcChildrenSizes(rootSize.Width); + // const auto paneSizes = _CalcChildrenSizes(rootSize.Width); auto firstColDef = Controls::ColumnDefinition(); // firstColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); @@ -820,7 +820,7 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize) _root.RowDefinitions().Clear(); // Create two rows in this grid: one for each pane - const auto paneSizes = _CalcChildrenSizes(rootSize.Height); + // const auto paneSizes = _CalcChildrenSizes(rootSize.Height); auto firstRowDef = Controls::RowDefinition(); firstRowDef.Height(GridLengthHelper::FromValueAndType(first, GridUnitType::Star)); @@ -1560,4 +1560,35 @@ int Pane::GetLeafPaneCount() const noexcept return _IsLeaf() ? 1 : (_firstChild->GetLeafPaneCount() + _secondChild->GetLeafPaneCount()); } +std::optional Pane::PreCalculateAutoSplit(const std::shared_ptr target, + const winrt::Windows::Foundation::Size parentSize) const +{ + if (_IsLeaf()) + { + if (target.get() == this) + { + return parentSize.Width > parentSize.Height ? SplitState::Vertical : SplitState::Horizontal; + } + else + { + return std::nullopt; + } + } + else + { + // auto [firstWidth, secondWidth] = _CalcChildrenSizes(parentSize.Width); + // auto [firstHeight, secondHeight] = _CalcChildrenSizes(parentSize.Height); + float firstWidth = _splitState == SplitState::Vertical ? (parentSize.Width * _desiredSplitPosition) : parentSize.Width; + float secondWidth = _splitState == SplitState::Vertical ? (parentSize.Width - firstWidth) : parentSize.Width; + + float firstHeight = _splitState == SplitState::Horizontal ? (parentSize.Height * _desiredSplitPosition) : parentSize.Height; + float secondHeight = _splitState == SplitState::Horizontal ? (parentSize.Height - firstHeight) : parentSize.Height; + + auto firstResult = _firstChild->PreCalculateAutoSplit(target, { firstWidth, firstHeight }); + return firstResult.has_value() ? firstResult : _secondChild->PreCalculateAutoSplit(target, { secondWidth, secondHeight }); + } + + return std::nullopt; +} + DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate>); diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 940246367bb..bd6d0ccc20c 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -63,6 +63,7 @@ class Pane : public std::enable_shared_from_this const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; + std::optional PreCalculateAutoSplit(const std::shared_ptr target, const winrt::Windows::Foundation::Size parentSize) const; void Shutdown(); void Close(); @@ -138,6 +139,7 @@ class Pane : public std::enable_shared_from_this winrt::TerminalApp::SplitState _preTranslateAutoSplitType(const winrt::TerminalApp::SplitState& splitType) const; bool _isSizeZero() const; + std::optional _preCalculateAutoSplit(const std::shared_ptr target, const winrt::Windows::Foundation::Size parentSize) const; // Function Description: // - Returns true if the given direction can be used with the given split diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index d273b214dbd..4310762ab00 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -269,8 +269,8 @@ namespace winrt::TerminalApp::implementation // - void Tab::SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, TermControl& control) { - // Use the _rootPane to determine who to split. When we're initializing multiple panes on startup, this is - auto [first, second] = _rootPane->Split(splitType, profile, control); + auto [first, second] = _activePane->Split(splitType, profile, control); + _activePane = first; _AttachEventHandlersToControl(control); // Add a event handlers to the new panes' GotFocus event. When the pane @@ -436,5 +436,16 @@ namespace winrt::TerminalApp::implementation return _rootPane->GetLeafPaneCount(); } + // std::weak_ptr Tab::GetActivePane() const noexcept + // { + // return _activePane; + // } + + SplitState Tab::PreCalculateAutoSplit(winrt::Windows::Foundation::Size rootSize) const + { + auto result = _rootPane->PreCalculateAutoSplit(_activePane, rootSize); + return result.has_value() ? result.value() : SplitState::Vertical; + } + DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); } diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index f001d1a1fd4..a0d55c31e74 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -26,6 +26,7 @@ namespace winrt::TerminalApp::implementation winrt::Windows::UI::Xaml::UIElement GetRootElement(); winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const; std::optional GetFocusedProfile() const noexcept; + // std::weak_ptr GetActivePane() const noexcept; bool IsFocused() const noexcept; void SetFocused(const bool focused); @@ -38,6 +39,7 @@ namespace winrt::TerminalApp::implementation winrt::fire_and_forget UpdateIcon(const winrt::hstring iconPath); float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; + SplitState PreCalculateAutoSplit(winrt::Windows::Foundation::Size rootSize) const; void ResizeContent(const winrt::Windows::Foundation::Size& newSize); void ResizePane(const winrt::TerminalApp::Direction& direction); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index ea0961fb968..96d6e2b5c03 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1194,12 +1194,20 @@ namespace winrt::TerminalApp::implementation return; } + auto realSplitType = splitType; + if (realSplitType == SplitState::Automatic && _startupState < StartupState::Initialized) + { + float contentWidth = gsl::narrow_cast(_tabContent.ActualWidth()); + float contentHeight = gsl::narrow_cast(_tabContent.ActualHeight()); + realSplitType = focusedTab->PreCalculateAutoSplit({ contentWidth, contentHeight }); + } + TermControl newControl{ controlSettings, controlConnection }; // Hookup our event handlers to the new terminal _RegisterTerminalEvents(newControl, *focusedTab); - focusedTab->SplitPane(splitType, realGuid, newControl); + focusedTab->SplitPane(realSplitType, realGuid, newControl); } CATCH_LOG(); // TODO: Should we display a dialog when we do nothing because we From 998318aa3bab9c3bb9124cbf2f022bfebeba2e86 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 19 Mar 2020 17:16:22 -0500 Subject: [PATCH 17/32] Some code cleanup "before" 5pm --- src/cascadia/TerminalApp/Pane.cpp | 50 +++++++-- src/cascadia/TerminalApp/Tab.cpp | 19 ++-- src/cascadia/TerminalApp/TerminalPage.cpp | 123 ++++++++++++++++------ src/cascadia/TerminalApp/TerminalPage.h | 2 + 4 files changed, 145 insertions(+), 49 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 611a2a5241d..3e7b73c787f 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1560,31 +1560,63 @@ int Pane::GetLeafPaneCount() const noexcept return _IsLeaf() ? 1 : (_firstChild->GetLeafPaneCount() + _secondChild->GetLeafPaneCount()); } +// Method Description: +// - This is a helper to determine which direction an "Automatic" split should +// happen in for a given pane, but without using the ActualWidth() and +// ActualHeight() methods. This is used during the initialization of the +// Terminal, when we could be processing many "split-pane" commands _before_ +// we've ever laid out the Terminal for the first time. When this happens, the +// Pane's don't have an actual size yet. However, we'd still like to figure +// out how to do an "auto" split when these Panes are all laid out. +// - This method assumes that the Pane we're attempting to split is `target`, +// and this method should be called on the root of a tree of Panes. +// - We'll walk down the tree attempting to find `target`. As we traverse the +// tree, we'll reduce the size passed to each subsequent recursive call. The +// size passed to this method represents how much space this Pane _will_ have +// to use. +// * If this pane is a leaf, and it's the pane we're looking for, use the +// available space to calculate which direction to split in. +// * If this pane is _any other leaf_, then just return nullopt, to indicate +// that the `target` Pane is not down this branch. +// * If this pane is a parent, calculate how much space our children will be +// able to use, and recurse into them. +// Arguments: +// - target: The Pane we're attempting to split. +// - availableSpace: The theoretical space that's available for this pane to be able to split. +// Return Value: +// - nullopt if `target` is not this pane or a child of this pane, otherwise the +// SplitState that `target` would use for an `Automatic` split given +// `availableSpace` std::optional Pane::PreCalculateAutoSplit(const std::shared_ptr target, - const winrt::Windows::Foundation::Size parentSize) const + const winrt::Windows::Foundation::Size availableSpace) const { if (_IsLeaf()) { if (target.get() == this) { - return parentSize.Width > parentSize.Height ? SplitState::Vertical : SplitState::Horizontal; + //If this pane is a leaf, and it's the pane we're looking for, use + //the available space to calculate which direction to split in. + return availableSpace.Width > availableSpace.Height ? SplitState::Vertical : SplitState::Horizontal; } else { + // If this pane is _any other leaf_, then just return nullopt, to + // indicate that the `target` Pane is not down this branch. return std::nullopt; } } else { - // auto [firstWidth, secondWidth] = _CalcChildrenSizes(parentSize.Width); - // auto [firstHeight, secondHeight] = _CalcChildrenSizes(parentSize.Height); - float firstWidth = _splitState == SplitState::Vertical ? (parentSize.Width * _desiredSplitPosition) : parentSize.Width; - float secondWidth = _splitState == SplitState::Vertical ? (parentSize.Width - firstWidth) : parentSize.Width; + // If this pane is a parent, calculate how much space our children will + // be able to use, and recurse into them. - float firstHeight = _splitState == SplitState::Horizontal ? (parentSize.Height * _desiredSplitPosition) : parentSize.Height; - float secondHeight = _splitState == SplitState::Horizontal ? (parentSize.Height - firstHeight) : parentSize.Height; + const bool isVerticalSplit = _splitState == SplitState::Vertical; + const float firstWidth = isVerticalSplit ? (availableSpace.Width * _desiredSplitPosition) : availableSpace.Width; + const float secondWidth = isVerticalSplit ? (availableSpace.Width - firstWidth) : availableSpace.Width; + const float firstHeight = !isVerticalSplit ? (availableSpace.Height * _desiredSplitPosition) : availableSpace.Height; + const float secondHeight = !isVerticalSplit ? (availableSpace.Height - firstHeight) : availableSpace.Height; - auto firstResult = _firstChild->PreCalculateAutoSplit(target, { firstWidth, firstHeight }); + const auto firstResult = _firstChild->PreCalculateAutoSplit(target, { firstWidth, firstHeight }); return firstResult.has_value() ? firstResult : _secondChild->PreCalculateAutoSplit(target, { secondWidth, secondHeight }); } diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 4310762ab00..15260a49819 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -436,14 +436,19 @@ namespace winrt::TerminalApp::implementation return _rootPane->GetLeafPaneCount(); } - // std::weak_ptr Tab::GetActivePane() const noexcept - // { - // return _activePane; - // } - - SplitState Tab::PreCalculateAutoSplit(winrt::Windows::Foundation::Size rootSize) const + // Method Description: + // - This is a helper to determine which direction an "Automatic" split should + // happen in for the active pane of this tab, but without using the ActualWidth() and + // ActualHeight() methods. + // - See Pane::PreCalculateAutoSplit + // Arguments: + // - availableSpace: The theoretical space that's available for this Tab's content + // Return Value: + // - The SplitState that we should use for an `Automatic` split given + // `availableSpace` + SplitState Tab::PreCalculateAutoSplit(winrt::Windows::Foundation::Size availableSpace) const { - auto result = _rootPane->PreCalculateAutoSplit(_activePane, rootSize); + const auto result = _rootPane->PreCalculateAutoSplit(_activePane, availableSpace); return result.has_value() ? result.value() : SplitState::Vertical; } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 96d6e2b5c03..241611646cb 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -167,43 +167,100 @@ namespace winrt::TerminalApp::implementation // Initialize the terminal only once the swapchainpanel is loaded - that // way, we'll be able to query the real pixel size it got on layout // _layoutUpdatedRevoker = this->LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { - _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { - Windows::Foundation::Size actualSize{ gsl::narrow_cast(_tabContent.ActualWidth()), - gsl::narrow_cast(_tabContent.ActualHeight()) }; - Windows::Foundation::Size desiredSize = _tabContent.DesiredSize(); - actualSize; - desiredSize; + // _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { + // Windows::Foundation::Size actualSize{ gsl::narrow_cast(_tabContent.ActualWidth()), + // gsl::narrow_cast(_tabContent.ActualHeight()) }; + // Windows::Foundation::Size desiredSize = _tabContent.DesiredSize(); + // actualSize; + // desiredSize; + + // // Only let this succeed once. + // this->_layoutUpdatedRevoker.revoke(); + + // // This event fires every time the layout changes, but it is always the last one to fire + // // in any layout change chain. That gives us great flexibility in finding the right point + // // at which to initialize our renderer (and our terminal). + // // Any earlier than the last layout update and we may not know the terminal's starting size. + // if (_startupState == StartupState::NotInitialized) + // { + // _startupState = StartupState::InStartup; + // if (_appArgs.GetStartupActions().size() == 0) + // { + // _OpenNewTab(nullptr); + // _startupState = StartupState::Initialized; + // } + // else + // { + // Dispatcher().RunAsync(CoreDispatcherPriority::Low, [this]() { + // for (const auto& action : _appArgs.GetStartupActions()) + // { + // // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { + // _actionDispatch->DoAction(action); + // // }); + // } + // _startupState = StartupState::Initialized; + // }); + // } + // } + // }); + _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, { this, &TerminalPage::_OnFirstLayout }); + + //////////////////////////////////////////////////////////////////////// + } - // Only let this succeed once. - this->_layoutUpdatedRevoker.revoke(); + void TerminalPage::_OnFirstLayout(const IInspectable& /*sender*/, const IInspectable& /*eventArgs*/) + { + // Only let this succeed once. + _layoutUpdatedRevoker.revoke(); - // This event fires every time the layout changes, but it is always the last one to fire - // in any layout change chain. That gives us great flexibility in finding the right point - // at which to initialize our renderer (and our terminal). - // Any earlier than the last layout update and we may not know the terminal's starting size. - if (_startupState == StartupState::NotInitialized) + // This event fires every time the layout changes, but it is always the last one to fire + // in any layout change chain. That gives us great flexibility in finding the right point + // at which to initialize our renderer (and our terminal). + // Any earlier than the last layout update and we may not know the terminal's starting size. + if (_startupState == StartupState::NotInitialized) + { + _startupState = StartupState::InStartup; + if (_appArgs.GetStartupActions().size() == 0) { - _startupState = StartupState::InStartup; - if (_appArgs.GetStartupActions().size() == 0) - { - _OpenNewTab(nullptr); - _startupState = StartupState::Initialized; - } - else - { - Dispatcher().RunAsync(CoreDispatcherPriority::Low, [this]() { - for (const auto& action : _appArgs.GetStartupActions()) - { - // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { - _actionDispatch->DoAction(action); - // }); - } - _startupState = StartupState::Initialized; - }); - } + _OpenNewTab(nullptr); + _startupState = StartupState::Initialized; } - }); - //////////////////////////////////////////////////////////////////////// + else + { + _ProcessStartupActions(); + // Dispatcher().RunAsync(CoreDispatcherPriority::Low, [this]() { + // for (const auto& action : _appArgs.GetStartupActions()) + // { + // // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { + // _actionDispatch->DoAction(action); + // // }); + // } + // _startupState = StartupState::Initialized; + // }); + } + } + } + fire_and_forget TerminalPage::_ProcessStartupActions() + { + // If there are no actions left, do nothing. + if (_appArgs.GetStartupActions().empty()) + { + return; + } + auto weakThis{ get_weak() }; + + // Handle it on the UI thread. + co_await winrt::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low); + if (auto page{ weakThis.get() }) + { + for (const auto& action : page->_appArgs.GetStartupActions()) + { + // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { + page->_actionDispatch->DoAction(action); + // }); + } + _startupState = StartupState::Initialized; + } } // Method Description: diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 1ec03daa244..6302b994cd9 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -88,6 +88,7 @@ namespace winrt::TerminalApp::implementation ::TerminalApp::AppCommandlineArgs _appArgs; int _ParseArgs(winrt::array_view& args); fire_and_forget _ProcessNextStartupAction(); + fire_and_forget _ProcessStartupActions(); void _ShowAboutDialog(); void _ShowCloseWarningDialog(); @@ -151,6 +152,7 @@ namespace winrt::TerminalApp::implementation void _OnTabItemsChanged(const IInspectable& sender, const Windows::Foundation::Collections::IVectorChangedEventArgs& eventArgs); void _OnContentSizeChanged(const IInspectable& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e); void _OnTabCloseRequested(const IInspectable& sender, const Microsoft::UI::Xaml::Controls::TabViewTabCloseRequestedEventArgs& eventArgs); + void _OnFirstLayout(const IInspectable& sender, const IInspectable& eventArgs); void _Find(); From f3948d811592fa79f35a19a3edc4192ccb6c4de1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 11:07:17 -0500 Subject: [PATCH 18/32] I somehow totally blew up the test? --- .../LocalTests_TerminalApp/TabTests.cpp | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 9593c475c54..7ef3744feb0 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -55,6 +55,8 @@ namespace TerminalAppLocalTests TEST_METHOD(CreateSimpleTerminalXamlType); TEST_METHOD(CreateTerminalMuxXamlType); + TEST_METHOD(CreateTerminalPage); + TEST_METHOD(TryDuplicateBadTab); TEST_METHOD(TryDuplicateBadPane); @@ -169,6 +171,18 @@ namespace TerminalAppLocalTests VERIFY_SUCCEEDED(result); } + void TabTests::CreateTerminalPage() + { + winrt::com_ptr page{ nullptr }; + + auto result = RunOnUIThread([&page]() { + DebugBreak(); + page = winrt::make_self(); + VERIFY_IS_NOT_NULL(page); + }); + VERIFY_SUCCEEDED(result); + } + void TabTests::TryDuplicateBadTab() { // * Create a tab with a profile with GUID 1 @@ -238,6 +252,7 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Construct the TerminalPage")); auto result = RunOnUIThread([&projectedPage, &page, settings0]() { + // DebugBreak(); projectedPage = winrt::TerminalApp::TerminalPage(); page.copy_from(winrt::get_self(projectedPage)); page->_settings = settings0; @@ -252,7 +267,19 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); page->Create(); + Log::Comment(NoThrowString().Format(L"Create()'d")); + + auto app = ::winrt::Windows::UI::Xaml::Application::Current(); + auto f = app.as(); + //f.Navigate(page.) + // f.Content(page->Root()); + f.Content(*page); + Log::Comment(NoThrowString().Format(L"Content()'d")); + }); + VERIFY_SUCCEEDED(result); + result = RunOnUIThread([&page]() { + Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); // 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. @@ -260,9 +287,9 @@ 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"... Done")); }); VERIFY_SUCCEEDED(result); - result = RunOnUIThread([&page]() { VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); }); @@ -395,7 +422,7 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Duplicate the first pane")); result = RunOnUIThread([&page]() { // Oh no I didn't actually duplicate the pane, what did I do - DebugBreak(); + // DebugBreak(); // The problem here is that the pane doesn't actually have a real // size yet. It thinks it's 0x0, which it is. We either need to // - 1. trick the test into thinking the pane has a real size From 1dffef4e758a4de34d2a67b3b2158f60959d12c5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 11:54:49 -0500 Subject: [PATCH 19/32] okay this fixes this test --- src/cascadia/LocalTests_TerminalApp/TabTests.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 7ef3744feb0..b79588e4600 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -270,11 +270,20 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Create()'d")); auto app = ::winrt::Windows::UI::Xaml::Application::Current(); - auto f = app.as(); - //f.Navigate(page.) + Log::Comment(NoThrowString().Format(L"got app")); + + // auto f = app.as(); + // Log::Comment(NoThrowString().Format(L"got frame")); + // // f.Content(*page); + // //f.Navigate(page.) // f.Content(page->Root()); - f.Content(*page); + // Log::Comment(NoThrowString().Format(L"Content()'d")); + + winrt::TerminalApp::TerminalPage pp = *page; + winrt::Windows::UI::Xaml::Window::Current().Content(pp); Log::Comment(NoThrowString().Format(L"Content()'d")); + winrt::Windows::UI::Xaml::Window::Current().Activate(); + Log::Comment(NoThrowString().Format(L"Activate()'d")); }); VERIFY_SUCCEEDED(result); From e2e45a982a451b13f239992c69e23767c79d0f8a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 12:33:10 -0500 Subject: [PATCH 20/32] try manually cleaning up the page, but this doesn't work --- .../LocalTests_TerminalApp/TabTests.cpp | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index b79588e4600..8d4592a40b9 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -176,7 +176,6 @@ namespace TerminalAppLocalTests winrt::com_ptr page{ nullptr }; auto result = RunOnUIThread([&page]() { - DebugBreak(); page = winrt::make_self(); VERIFY_IS_NOT_NULL(page); }); @@ -252,7 +251,6 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Construct the TerminalPage")); auto result = RunOnUIThread([&projectedPage, &page, settings0]() { - // DebugBreak(); projectedPage = winrt::TerminalApp::TerminalPage(); page.copy_from(winrt::get_self(projectedPage)); page->_settings = settings0; @@ -262,8 +260,14 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); + winrt::Windows::UI::Xaml::UIElement originalContent{ nullptr }; + Log::Comment(NoThrowString().Format(L"Create() the TerminalPage")); - result = RunOnUIThread([&page]() { + result = RunOnUIThread([&page, &originalContent]() { + originalContent = winrt::Windows::UI::Xaml::Window::Current().Content(); + + // VERIFY_IS_NOT_NULL(originalContent); + VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); page->Create(); @@ -287,6 +291,20 @@ namespace TerminalAppLocalTests }); VERIFY_SUCCEEDED(result); + // VERIFY_IS_NOT_NULL(originalContent); + + auto cleanup = wil::scope_exit([originalContent] { + auto result = RunOnUIThread([originalContent]() { + Sleep(1000); + Log::Comment(NoThrowString().Format(L"Cleaning up application content...")); + winrt::Windows::UI::Xaml::Window::Current().Content(originalContent); + Log::Comment(NoThrowString().Format(L"Content()'d")); + winrt::Windows::UI::Xaml::Window::Current().Activate(); + Log::Comment(NoThrowString().Format(L"Activate()'d")); + }); + VERIFY_SUCCEEDED(result); + }); + result = RunOnUIThread([&page]() { Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); // I think in the tests, we don't always set the focused tab on @@ -410,7 +428,28 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); page->Create(); + Log::Comment(NoThrowString().Format(L"Create()'d")); + auto app = ::winrt::Windows::UI::Xaml::Application::Current(); + Log::Comment(NoThrowString().Format(L"got app")); + + // auto f = app.as(); + // Log::Comment(NoThrowString().Format(L"got frame")); + // // f.Content(*page); + // //f.Navigate(page.) + // f.Content(page->Root()); + // Log::Comment(NoThrowString().Format(L"Content()'d")); + + winrt::TerminalApp::TerminalPage pp = *page; + winrt::Windows::UI::Xaml::Window::Current().Content(pp); + Log::Comment(NoThrowString().Format(L"Content()'d")); + winrt::Windows::UI::Xaml::Window::Current().Activate(); + Log::Comment(NoThrowString().Format(L"Activate()'d")); + }); + VERIFY_SUCCEEDED(result); + + result = RunOnUIThread([&page]() { + Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); // 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. @@ -418,6 +457,11 @@ 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"... Done")); + }); + VERIFY_SUCCEEDED(result); + result = RunOnUIThread([&page]() { + VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); }); VERIFY_SUCCEEDED(result); From 808868f07302de9dfa0aa8acad68e6aa5e969fa6 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 12:40:48 -0500 Subject: [PATCH 21/32] cleanup the test --- .../LocalTests_TerminalApp/TabTests.cpp | 69 +++++++------------ 1 file changed, 25 insertions(+), 44 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 8d4592a40b9..4c6aa5f59d0 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -260,51 +260,21 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); - winrt::Windows::UI::Xaml::UIElement originalContent{ nullptr }; - Log::Comment(NoThrowString().Format(L"Create() the TerminalPage")); result = RunOnUIThread([&page, &originalContent]() { - originalContent = winrt::Windows::UI::Xaml::Window::Current().Content(); - - // VERIFY_IS_NOT_NULL(originalContent); - VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); page->Create(); - Log::Comment(NoThrowString().Format(L"Create()'d")); + Log::Comment(NoThrowString().Format(L"Create()'d the page successfully")); auto app = ::winrt::Windows::UI::Xaml::Application::Current(); - Log::Comment(NoThrowString().Format(L"got app")); - - // auto f = app.as(); - // Log::Comment(NoThrowString().Format(L"got frame")); - // // f.Content(*page); - // //f.Navigate(page.) - // f.Content(page->Root()); - // Log::Comment(NoThrowString().Format(L"Content()'d")); winrt::TerminalApp::TerminalPage pp = *page; winrt::Windows::UI::Xaml::Window::Current().Content(pp); - Log::Comment(NoThrowString().Format(L"Content()'d")); winrt::Windows::UI::Xaml::Window::Current().Activate(); - Log::Comment(NoThrowString().Format(L"Activate()'d")); }); VERIFY_SUCCEEDED(result); - // VERIFY_IS_NOT_NULL(originalContent); - - auto cleanup = wil::scope_exit([originalContent] { - auto result = RunOnUIThread([originalContent]() { - Sleep(1000); - Log::Comment(NoThrowString().Format(L"Cleaning up application content...")); - winrt::Windows::UI::Xaml::Window::Current().Content(originalContent); - Log::Comment(NoThrowString().Format(L"Content()'d")); - winrt::Windows::UI::Xaml::Window::Current().Activate(); - Log::Comment(NoThrowString().Format(L"Activate()'d")); - }); - VERIFY_SUCCEEDED(result); - }); - result = RunOnUIThread([&page]() { Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); // I think in the tests, we don't always set the focused tab on @@ -343,6 +313,18 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(2u, page->_tabs.Size(), L"We should gracefully do nothing here - the profile no longer exists."); }); VERIFY_SUCCEEDED(result); + + auto cleanup = wil::scope_exit([] { + auto result = RunOnUIThread([]() { + // There's something causing us to crash north of + // TSFInputControl::NotifyEnter, or LayoutRequested. It's very + // unclear what that issue is. Since these tests don't run in + // CI, simply log a message so that the dev running these tests + // knows it's expected. + Log::Comment(L"This test often crashes on cleanup, even when it succeeds. If it succeeded, then crashes, that's okay."); + }); + VERIFY_SUCCEEDED(result); + }); } void TabTests::TryDuplicateBadPane() @@ -433,13 +415,6 @@ namespace TerminalAppLocalTests auto app = ::winrt::Windows::UI::Xaml::Application::Current(); Log::Comment(NoThrowString().Format(L"got app")); - // auto f = app.as(); - // Log::Comment(NoThrowString().Format(L"got frame")); - // // f.Content(*page); - // //f.Navigate(page.) - // f.Content(page->Root()); - // Log::Comment(NoThrowString().Format(L"Content()'d")); - winrt::TerminalApp::TerminalPage pp = *page; winrt::Windows::UI::Xaml::Window::Current().Content(pp); Log::Comment(NoThrowString().Format(L"Content()'d")); @@ -474,12 +449,6 @@ namespace TerminalAppLocalTests Log::Comment(NoThrowString().Format(L"Duplicate the first pane")); result = RunOnUIThread([&page]() { - // Oh no I didn't actually duplicate the pane, what did I do - // DebugBreak(); - // The problem here is that the pane doesn't actually have a real - // size yet. It thinks it's 0x0, which it is. We either need to - // - 1. trick the test into thinking the pane has a real size - // - 2. allow panes to be split regardless of their minimum size page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, nullptr); VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); @@ -507,6 +476,18 @@ namespace TerminalAppLocalTests L"We should gracefully do nothing here - the profile no longer exists."); }); VERIFY_SUCCEEDED(result); + + auto cleanup = wil::scope_exit([] { + auto result = RunOnUIThread([]() { + // There's something causing us to crash north of + // TSFInputControl::NotifyEnter, or LayoutRequested. It's very + // unclear what that issue is. Since these tests don't run in + // CI, simply log a message so that the dev running these tests + // knows it's expected. + Log::Comment(L"This test often crashes on cleanup, even when it succeeds. If it succeeded, then crashes, that's okay."); + }); + VERIFY_SUCCEEDED(result); + }); } } From 6208f43acf7c90d56da9188dc053d4ccdbf1536f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 12:57:32 -0500 Subject: [PATCH 22/32] This is a ton of colde cleanup ; `wtd split-pane` is broken, doesn't create the first tab :O --- src/cascadia/TerminalApp/TerminalPage.cpp | 115 +++------------------- src/cascadia/TerminalApp/TerminalPage.h | 1 - src/cascadia/WindowsTerminal/AppHost.cpp | 11 +-- 3 files changed, 16 insertions(+), 111 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 241611646cb..a95b0e983c4 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -146,66 +146,12 @@ namespace winrt::TerminalApp::implementation _tabContent.SizeChanged({ this, &TerminalPage::_OnContentSizeChanged }); - // //////////////////////////////////////////////////////////////////////// - // // Actually start the terminal. - // if (_appArgs.GetStartupActions().empty()) - // { - // _OpenNewTab(nullptr); - // } - // else - // { - // _appArgs.ValidateStartupCommands(); - - // // This will kick off a chain of events to perform each startup - // // action. As each startup action is completed, the next will be - // // fired. - // _ProcessNextStartupAction(); - // } - // //////////////////////////////////////////////////////////////////////// - - //////////////////////////////////////////////////////////////////////// - // Initialize the terminal only once the swapchainpanel is loaded - that - // way, we'll be able to query the real pixel size it got on layout - // _layoutUpdatedRevoker = this->LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { - // _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { - // Windows::Foundation::Size actualSize{ gsl::narrow_cast(_tabContent.ActualWidth()), - // gsl::narrow_cast(_tabContent.ActualHeight()) }; - // Windows::Foundation::Size desiredSize = _tabContent.DesiredSize(); - // actualSize; - // desiredSize; - - // // Only let this succeed once. - // this->_layoutUpdatedRevoker.revoke(); - - // // This event fires every time the layout changes, but it is always the last one to fire - // // in any layout change chain. That gives us great flexibility in finding the right point - // // at which to initialize our renderer (and our terminal). - // // Any earlier than the last layout update and we may not know the terminal's starting size. - // if (_startupState == StartupState::NotInitialized) - // { - // _startupState = StartupState::InStartup; - // if (_appArgs.GetStartupActions().size() == 0) - // { - // _OpenNewTab(nullptr); - // _startupState = StartupState::Initialized; - // } - // else - // { - // Dispatcher().RunAsync(CoreDispatcherPriority::Low, [this]() { - // for (const auto& action : _appArgs.GetStartupActions()) - // { - // // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { - // _actionDispatch->DoAction(action); - // // }); - // } - // _startupState = StartupState::Initialized; - // }); - // } - // } - // }); + // Once the page is actually laid out on the screen, trigger all aout + // startup actions. Things like Panes need to know at least how big the + // window will be, so they can subdivide that space. + // + // _OnFirstLayout will remove this handler so it doesn't get called more than once. _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, { this, &TerminalPage::_OnFirstLayout }); - - //////////////////////////////////////////////////////////////////////// } void TerminalPage::_OnFirstLayout(const IInspectable& /*sender*/, const IInspectable& /*eventArgs*/) @@ -228,70 +174,35 @@ namespace winrt::TerminalApp::implementation else { _ProcessStartupActions(); - // Dispatcher().RunAsync(CoreDispatcherPriority::Low, [this]() { - // for (const auto& action : _appArgs.GetStartupActions()) - // { - // // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { - // _actionDispatch->DoAction(action); - // // }); - // } - // _startupState = StartupState::Initialized; - // }); - } - } - } - fire_and_forget TerminalPage::_ProcessStartupActions() - { - // If there are no actions left, do nothing. - if (_appArgs.GetStartupActions().empty()) - { - return; - } - auto weakThis{ get_weak() }; - - // Handle it on the UI thread. - co_await winrt::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low); - if (auto page{ weakThis.get() }) - { - for (const auto& action : page->_appArgs.GetStartupActions()) - { - // Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, action]() { - page->_actionDispatch->DoAction(action); - // }); } - _startupState = StartupState::Initialized; } } // Method Description: - // - Process the next startup action in our list of startup actions. When - // that action is complete, fire the next (if there are any more). + // - Process all the startup actions in our list of startup actions. We'll + // do this all at once here. // Arguments: // - // Return Value: // - - fire_and_forget TerminalPage::_ProcessNextStartupAction() + fire_and_forget TerminalPage::_ProcessStartupActions() { // If there are no actions left, do nothing. if (_appArgs.GetStartupActions().empty()) { return; } - - // Get the next action to be processed - auto nextAction = _appArgs.GetStartupActions().front(); - _appArgs.GetStartupActions().pop_front(); - auto weakThis{ get_weak() }; // Handle it on the UI thread. co_await winrt::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low); if (auto page{ weakThis.get() }) { - page->_actionDispatch->DoAction(nextAction); - - // Kick off the next action to be handled (if necessary) - page->_ProcessNextStartupAction(); + for (const auto& action : page->_appArgs.GetStartupActions()) + { + page->_actionDispatch->DoAction(action); + } + _startupState = StartupState::Initialized; } } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 6302b994cd9..9a924792fd6 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -87,7 +87,6 @@ namespace winrt::TerminalApp::implementation ::TerminalApp::AppCommandlineArgs _appArgs; int _ParseArgs(winrt::array_view& args); - fire_and_forget _ProcessNextStartupAction(); fire_and_forget _ProcessStartupActions(); void _ShowAboutDialog(); diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 326ddb4cc80..2959b22220e 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -92,16 +92,11 @@ void AppHost::_HandleCommandlineArgs() int argc = 0; // Get the argv, and turn them into a hstring array to pass to the app. - // std::vector argv{ L"wt.exe", L";", L"split-pane" }; - std::vector argv{ L"wt.exe", L";", L"split-pane", L";", L"split-pane", L"-p", L"cmd", L";", L"split-pane", L"-p", L"mode" }; - argc; - // wil::unique_any argv{ CommandLineToArgvW(commandline, &argc) }; - // if (argv) - if (argv.size() > 0) + wil::unique_any argv{ CommandLineToArgvW(commandline, &argc) }; + if (argv) { std::vector args; - // for (auto& elem : wil::make_range(argv.get(), argc)) - for (auto& elem : argv) + for (auto& elem : wil::make_range(argv.get(), argc)) { args.emplace_back(elem); } From 09ab62a3ebbd2b0504add198e5924de367852f2b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 15:40:20 -0500 Subject: [PATCH 23/32] this fixes just a bare `wt split-pane`; `wt; ; ; focus-tab -t 1; split-pane` doesn't seem to work anymore --- src/cascadia/TerminalApp/TerminalPage.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index a95b0e983c4..62c41ee0b06 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -146,7 +146,7 @@ namespace winrt::TerminalApp::implementation _tabContent.SizeChanged({ this, &TerminalPage::_OnContentSizeChanged }); - // Once the page is actually laid out on the screen, trigger all aout + // Once the page is actually laid out on the screen, trigger all our // startup actions. Things like Panes need to know at least how big the // window will be, so they can subdivide that space. // @@ -166,7 +166,8 @@ namespace winrt::TerminalApp::implementation if (_startupState == StartupState::NotInitialized) { _startupState = StartupState::InStartup; - if (_appArgs.GetStartupActions().size() == 0) + _appArgs.ValidateStartupCommands(); + if (_appArgs.GetStartupActions().empty()) { _OpenNewTab(nullptr); _startupState = StartupState::Initialized; From ce2b948a5aa162816b44acab3cfc6bad3ec2c150 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 16:57:15 -0500 Subject: [PATCH 24/32] this fixes `focus-tab` during startup, but it's kinda janky --- src/cascadia/TerminalApp/TerminalPage.cpp | 70 +++++++++++++++-------- src/cascadia/TerminalApp/TerminalPage.h | 1 + 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 62c41ee0b06..5d36c38e2e6 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -958,7 +958,9 @@ namespace winrt::TerminalApp::implementation // Wraparound math. By adding tabCount and then calculating modulo tabCount, // we clamp the values to the range [0, tabCount) while still supporting moving // leftward from 0 to tabCount - 1. - _SetFocusedTabIndex(((tabCount + *index + (bMoveRight ? 1 : -1)) % tabCount)); + const auto newTabIndex = ((tabCount + *index + (bMoveRight ? 1 : -1)) % tabCount); + // _SetFocusedTabIndex(newTabIndex); + _SelectTab(newTabIndex); } } @@ -971,7 +973,17 @@ namespace winrt::TerminalApp::implementation { if (tabIndex >= 0 && tabIndex < _tabs.Size()) { - _SetFocusedTabIndex(tabIndex); + if (_startupState == StartupState::InStartup) + { + auto tab{ _GetStrongTabImpl(tabIndex) }; + _tabView.SelectedItem(tab->GetTabViewItem()); + _UpdatedSelectedTab(tabIndex); + } + else + { + _SetFocusedTabIndex(tabIndex); + } + return true; } return false; @@ -1036,6 +1048,12 @@ namespace winrt::TerminalApp::implementation { auto tab{ _GetStrongTabImpl(tabIndex) }; _tabView.SelectedItem(tab->GetTabViewItem()); + + // If we're in startup, we're not going to come back through the dispatcher to handle + // if (page->_startupState == StartupState::InStartup) + // { + // page->_UpdatedSelectedTab(tabIndex); + // } } } @@ -1499,6 +1517,31 @@ namespace winrt::TerminalApp::implementation } } + void TerminalPage::_UpdatedSelectedTab(const int32_t index) + { + // Unfocus all the tabs. + for (auto tab : _tabs) + { + auto tabImpl{ _GetStrongTabImpl(tab) }; + tabImpl->SetFocused(false); + } + + if (index >= 0) + { + try + { + auto tab{ _GetStrongTabImpl(index) }; + + _tabContent.Children().Clear(); + _tabContent.Children().Append(tab->GetRootElement()); + + tab->SetFocused(true); + _titleChangeHandlers(*this, Title()); + } + CATCH_LOG(); + } + } + // Method Description: // - Responds to the TabView control's Selection Changed event (to move a // new terminal control into focus) when not in in the middle of a tab rearrangement. @@ -1511,28 +1554,7 @@ namespace winrt::TerminalApp::implementation { auto tabView = sender.as(); auto selectedIndex = tabView.SelectedIndex(); - - // Unfocus all the tabs. - for (auto tab : _tabs) - { - auto tabImpl{ _GetStrongTabImpl(tab) }; - tabImpl->SetFocused(false); - } - - if (selectedIndex >= 0) - { - try - { - auto tab{ _GetStrongTabImpl(selectedIndex) }; - - _tabContent.Children().Clear(); - _tabContent.Children().Append(tab->GetRootElement()); - - tab->SetFocused(true); - _titleChangeHandlers(*this, Title()); - } - CATCH_LOG(); - } + _UpdatedSelectedTab(selectedIndex); } } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 9a924792fd6..27c40522614 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -152,6 +152,7 @@ namespace winrt::TerminalApp::implementation void _OnContentSizeChanged(const IInspectable& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e); void _OnTabCloseRequested(const IInspectable& sender, const Microsoft::UI::Xaml::Controls::TabViewTabCloseRequestedEventArgs& eventArgs); void _OnFirstLayout(const IInspectable& sender, const IInspectable& eventArgs); + void _UpdatedSelectedTab(const int32_t index); void _Find(); From 4306389111a4828e3aef2090762307b0c9fabf6b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 23 Mar 2020 11:35:41 -0500 Subject: [PATCH 25/32] god bless, I fixed the tests --- .../LocalTests_TerminalApp/TabTests.cpp | 45 ++++++++++++++----- .../TerminalControl/TSFInputControl.cpp | 6 ++- src/cascadia/TerminalControl/TermControl.cpp | 6 +++ 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 4c6aa5f59d0..964cf0afa87 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -260,12 +260,14 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); - Log::Comment(NoThrowString().Format(L"Create() the TerminalPage")); - result = RunOnUIThread([&page, &originalContent]() { + // DebugBreak(); + + Log::Comment(L"Create() the TerminalPage"); + result = RunOnUIThread([&page]() { VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); page->Create(); - Log::Comment(NoThrowString().Format(L"Create()'d the page successfully")); + Log::Comment(L"Create()'d the page successfully"); auto app = ::winrt::Windows::UI::Xaml::Application::Current(); @@ -275,18 +277,37 @@ namespace TerminalAppLocalTests }); VERIFY_SUCCEEDED(result); + auto cleanupPage = wil::scope_exit([page] { + auto result = RunOnUIThread([page]() { + Log::Comment(NoThrowString().Format(L"Closing all tabs...")); + page->_CloseAllTabs(); + Log::Comment(L"_CloseAllTabs()'d successfully"); + }); + VERIFY_SUCCEEDED(result); + }); + + Log::Comment(L"Before Sleep"); + Sleep(1000); + Log::Comment(L"After Sleep"); + result = RunOnUIThread([&page]() { - Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); - // 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); - Log::Comment(NoThrowString().Format(L"... Done")); + // Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); + // // 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); + // Log::Comment(NoThrowString().Format(L"... Done")); + //////////////////////////////////////////////////////////////////// + + auto tab{ page->_GetStrongTabImpl(0) }; + page->_tabView.SelectedItem(tab->GetTabViewItem()); + page->_UpdatedSelectedTab(0); }); VERIFY_SUCCEEDED(result); + result = RunOnUIThread([&page]() { VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); }); diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index 27868776f3a..c69f3c7e360 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -172,8 +172,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation TextBlock().FontSize(fontSizePx); TextBlock().FontFamily(Media::FontFamily(fontArgs->FontFace())); - const auto widthToTerminalEnd = Canvas().ActualWidth() - ::base::ClampedNumeric(clientCursorPos.X); - TextBlock().MaxWidth(widthToTerminalEnd); + const auto canvasActualWidth = Canvas().ActualWidth(); + const auto widthToTerminalEnd = canvasActualWidth - ::base::ClampedNumeric(clientCursorPos.X); + const auto newMaxWidth = std::max(0.0, widthToTerminalEnd); + TextBlock().MaxWidth(newMaxWidth); // Set the text block bounds const auto yOffset = ::base::ClampedNumeric(TextBlock().ActualHeight()) - fontHeight; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 0d955716ca0..4dbc2dc1908 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -879,6 +879,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - point: the PointerPoint object representing a mouse event from our XAML input handler bool TermControl::_CanSendVTMouseInput() { + if (!_terminal) + { + return false; + } // If the user is holding down Shift, suppress mouse events // TODO GH#4875: disable/customize this functionality const auto modifiers = _GetPressedModifierKeys(); @@ -2207,6 +2211,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // If we haven't initialized yet, just quick return. if (!_terminal) { + // fake it + eventArgs.CurrentPosition({ 0, 0 }); return; } const COORD cursorPos = _terminal->GetCursorPosition(); From 2b3b517851e37f124304a021e4087c940b4e8c06 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 23 Mar 2020 12:01:53 -0500 Subject: [PATCH 26/32] add an Initialized event so the tests can know when we're finished initializing --- .../LocalTests_TerminalApp/TabTests.cpp | 52 ++++++++++--------- src/cascadia/TerminalApp/TerminalPage.cpp | 2 + src/cascadia/TerminalApp/TerminalPage.h | 1 + src/cascadia/TerminalApp/TerminalPage.idl | 1 + 4 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 964cf0afa87..84a82b9e94b 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -262,13 +262,28 @@ namespace TerminalAppLocalTests // DebugBreak(); + ::details::Event waitForLayoutEvent; + if (!waitForLayoutEvent.IsValid()) + { + VERIFY_SUCCEEDED(HRESULT_FROM_WIN32(::GetLastError())); + } + // winrt::Windows::UI::Xaml::Controls::Grid::LayoutUpdated_revoker layoutUpdatedRevoker; + Log::Comment(L"Create() the TerminalPage"); - result = RunOnUIThread([&page]() { + // result = RunOnUIThread([&page, &waitForLayoutEvent, &layoutUpdatedRevoker]() { + result = RunOnUIThread([&page, &waitForLayoutEvent]() { VERIFY_IS_NOT_NULL(page); VERIFY_IS_NOT_NULL(page->_settings); page->Create(); Log::Comment(L"Create()'d the page successfully"); + // layoutUpdatedRevoker = page->_tabContent.LayoutUpdated(winrt::auto_revoke, [&waitForLayoutEvent, &layoutUpdatedRevoker](auto&&, auto&&) { + page->Initialized([&waitForLayoutEvent](auto&&, auto&&) { + // layoutUpdatedRevoker.revoke(); + // Sleep(1000); + waitForLayoutEvent.Set(); + }); + auto app = ::winrt::Windows::UI::Xaml::Application::Current(); winrt::TerminalApp::TerminalPage pp = *page; @@ -277,18 +292,19 @@ namespace TerminalAppLocalTests }); VERIFY_SUCCEEDED(result); - auto cleanupPage = wil::scope_exit([page] { - auto result = RunOnUIThread([page]() { - Log::Comment(NoThrowString().Format(L"Closing all tabs...")); - page->_CloseAllTabs(); - Log::Comment(L"_CloseAllTabs()'d successfully"); - }); - VERIFY_SUCCEEDED(result); - }); + // auto cleanupPage = wil::scope_exit([page] { + // auto result = RunOnUIThread([page]() { + // Log::Comment(NoThrowString().Format(L"Closing all tabs...")); + // page->_CloseAllTabs(); + // Log::Comment(L"_CloseAllTabs()'d successfully"); + // }); + // VERIFY_SUCCEEDED(result); + // }); - Log::Comment(L"Before Sleep"); - Sleep(1000); - Log::Comment(L"After Sleep"); + // Log::Comment(L"Before Sleep"); + // Sleep(1000); + // Log::Comment(L"After Sleep"); + VERIFY_SUCCEEDED(waitForLayoutEvent.Wait()); result = RunOnUIThread([&page]() { // Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); @@ -334,18 +350,6 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(2u, page->_tabs.Size(), L"We should gracefully do nothing here - the profile no longer exists."); }); VERIFY_SUCCEEDED(result); - - auto cleanup = wil::scope_exit([] { - auto result = RunOnUIThread([]() { - // There's something causing us to crash north of - // TSFInputControl::NotifyEnter, or LayoutRequested. It's very - // unclear what that issue is. Since these tests don't run in - // CI, simply log a message so that the dev running these tests - // knows it's expected. - Log::Comment(L"This test often crashes on cleanup, even when it succeeds. If it succeeded, then crashes, that's okay."); - }); - VERIFY_SUCCEEDED(result); - }); } void TabTests::TryDuplicateBadPane() diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 5d36c38e2e6..967dbac077a 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -171,6 +171,7 @@ namespace winrt::TerminalApp::implementation { _OpenNewTab(nullptr); _startupState = StartupState::Initialized; + _InitializedHandlers(*this, nullptr); } else { @@ -204,6 +205,7 @@ namespace winrt::TerminalApp::implementation page->_actionDispatch->DoAction(action); } _startupState = StartupState::Initialized; + _InitializedHandlers(*this, nullptr); } } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 27c40522614..108451f61ce 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -55,6 +55,7 @@ namespace winrt::TerminalApp::implementation DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(SetTitleBarContent, _setTitleBarContentHandlers, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::UIElement); DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(ShowDialog, _showDialogHandlers, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::Controls::ContentDialog); DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(ToggleFullscreen, _toggleFullscreenHandlers, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::ToggleFullscreenEventArgs); + TYPED_EVENT(Initialized, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::RoutedEventArgs); private: // If you add controls here, but forget to null them either here or in diff --git a/src/cascadia/TerminalApp/TerminalPage.idl b/src/cascadia/TerminalApp/TerminalPage.idl index a1bfcd98db2..d91d4da861f 100644 --- a/src/cascadia/TerminalApp/TerminalPage.idl +++ b/src/cascadia/TerminalApp/TerminalPage.idl @@ -18,5 +18,6 @@ namespace TerminalApp event Windows.Foundation.TypedEventHandler SetTitleBarContent; event Windows.Foundation.TypedEventHandler ShowDialog; event Windows.Foundation.TypedEventHandler ToggleFullscreen; + event Windows.Foundation.TypedEventHandler Initialized; } } From e80ee3133a89a2690823dc56f5a3a3747ed154a3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 23 Mar 2020 12:42:39 -0500 Subject: [PATCH 27/32] some test code cleanup --- .../LocalTests_TerminalApp/TabTests.cpp | 218 ++++++++---------- 1 file changed, 100 insertions(+), 118 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 84a82b9e94b..01e1088bebb 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -65,6 +65,11 @@ namespace TerminalAppLocalTests InitializeJsonReader(); return true; } + + private: + void _initializeTerminalPage(winrt::TerminalApp::TerminalPage& projectedPage, + winrt::com_ptr& page, + std::shared_ptr initialSettings); }; void TabTests::EnsureTestsActivate() @@ -182,6 +187,95 @@ namespace TerminalAppLocalTests VERIFY_SUCCEEDED(result); } + // Method Description: + // - This is a helper to set up a TerminalPage for a unittest. This method + // does a couple things: + // * Create()'s a TerminalPage with the given settings. Constructing a + // TerminalPage so that we can get at its implementation is wacky, so + // this helper will do it correctly for you, even if this doesn't make a + // ton of sense on the surface. This is also why you need to pass both a + // projection and a com_ptr to this method. + // * It will use the provided settings object to initialize the TerminalPage + // * It will add the TerminalPage to the test Application, so that we can + // get actual layout events. Much of the Terminal assumes there's a + // non-zero ActualSize to the Terminal window, and adding the Page to + // the Application will make it behave as expected. + // * It will wait for the TerminalPage to finish initialization before + // returning control to the caler. It does this by creating an event and + // only setting the event when the TerminalPage raises its Initialized + // event, to signal that startup is complete. At this point, there will + // be one tab with the default profile in the page. + // * It will also ensure that the first tab is focused, since that happens + // asynchronously in the application typically. + // Arguments: + // - projectedPage: a TerminalPage projected type that will recieve the new TerminalPage instance + // - page: a TerminalPage implementation ptr that will recieve the new TerminalPage instance + // - initialSettings: a CascadiaSettings to initialize the TerminalPage with. + // Return Value: + // - + void TabTests::_initializeTerminalPage(winrt::TerminalApp::TerminalPage& projectedPage, + winrt::com_ptr& page, + std::shared_ptr initialSettings) + { + // 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. + Log::Comment(NoThrowString().Format(L"Construct the TerminalPage")); + auto result = RunOnUIThread([&projectedPage, &page, initialSettings]() { + projectedPage = winrt::TerminalApp::TerminalPage(); + page.copy_from(winrt::get_self(projectedPage)); + page->_settings = initialSettings; + }); + VERIFY_SUCCEEDED(result); + + VERIFY_IS_NOT_NULL(page); + VERIFY_IS_NOT_NULL(page->_settings); + + ::details::Event waitForInitEvent; + if (!waitForInitEvent.IsValid()) + { + VERIFY_SUCCEEDED(HRESULT_FROM_WIN32(::GetLastError())); + } + page->Initialized([&waitForInitEvent](auto&&, auto&&) { + waitForInitEvent.Set(); + }); + + Log::Comment(L"Create() the TerminalPage"); + + result = RunOnUIThread([&page]() { + VERIFY_IS_NOT_NULL(page); + VERIFY_IS_NOT_NULL(page->_settings); + page->Create(); + Log::Comment(L"Create()'d the page successfully"); + + auto app = ::winrt::Windows::UI::Xaml::Application::Current(); + + winrt::TerminalApp::TerminalPage pp = *page; + winrt::Windows::UI::Xaml::Window::Current().Content(pp); + winrt::Windows::UI::Xaml::Window::Current().Activate(); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Wait for the page to finish initializing..."); + VERIFY_SUCCEEDED(waitForInitEvent.Wait()); + Log::Comment(L"...Done"); + + result = RunOnUIThread([&page]() { + // In the real app, this isn't a problem, but doesn't happen + // reliably in the unit tests. + Log::Comment(L"Ensure we set the first tab as the selected one."); + auto tab{ page->_GetStrongTabImpl(0) }; + page->_tabView.SelectedItem(tab->GetTabViewItem()); + page->_UpdatedSelectedTab(0); + }); + VERIFY_SUCCEEDED(result); + } + void TabTests::TryDuplicateBadTab() { // * Create a tab with a profile with GUID 1 @@ -249,87 +343,14 @@ namespace TerminalAppLocalTests winrt::TerminalApp::TerminalPage projectedPage{ nullptr }; winrt::com_ptr page{ nullptr }; - 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); - - VERIFY_IS_NOT_NULL(page); - VERIFY_IS_NOT_NULL(page->_settings); - - // DebugBreak(); + _initializeTerminalPage(projectedPage, page, settings0); - ::details::Event waitForLayoutEvent; - if (!waitForLayoutEvent.IsValid()) - { - VERIFY_SUCCEEDED(HRESULT_FROM_WIN32(::GetLastError())); - } - // winrt::Windows::UI::Xaml::Controls::Grid::LayoutUpdated_revoker layoutUpdatedRevoker; - - Log::Comment(L"Create() the TerminalPage"); - // result = RunOnUIThread([&page, &waitForLayoutEvent, &layoutUpdatedRevoker]() { - result = RunOnUIThread([&page, &waitForLayoutEvent]() { - VERIFY_IS_NOT_NULL(page); - VERIFY_IS_NOT_NULL(page->_settings); - page->Create(); - Log::Comment(L"Create()'d the page successfully"); - - // layoutUpdatedRevoker = page->_tabContent.LayoutUpdated(winrt::auto_revoke, [&waitForLayoutEvent, &layoutUpdatedRevoker](auto&&, auto&&) { - page->Initialized([&waitForLayoutEvent](auto&&, auto&&) { - // layoutUpdatedRevoker.revoke(); - // Sleep(1000); - waitForLayoutEvent.Set(); - }); - - auto app = ::winrt::Windows::UI::Xaml::Application::Current(); - - winrt::TerminalApp::TerminalPage pp = *page; - winrt::Windows::UI::Xaml::Window::Current().Content(pp); - winrt::Windows::UI::Xaml::Window::Current().Activate(); - }); - VERIFY_SUCCEEDED(result); - - // auto cleanupPage = wil::scope_exit([page] { - // auto result = RunOnUIThread([page]() { - // Log::Comment(NoThrowString().Format(L"Closing all tabs...")); - // page->_CloseAllTabs(); - // Log::Comment(L"_CloseAllTabs()'d successfully"); - // }); - // VERIFY_SUCCEEDED(result); - // }); - - // Log::Comment(L"Before Sleep"); - // Sleep(1000); - // Log::Comment(L"After Sleep"); - VERIFY_SUCCEEDED(waitForLayoutEvent.Wait()); - - result = RunOnUIThread([&page]() { - // Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); - // // 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); - // Log::Comment(NoThrowString().Format(L"... Done")); - //////////////////////////////////////////////////////////////////// - - auto tab{ page->_GetStrongTabImpl(0) }; - page->_tabView.SelectedItem(tab->GetTabViewItem()); - page->_UpdatedSelectedTab(0); - }); - VERIFY_SUCCEEDED(result); - - result = RunOnUIThread([&page]() { + auto result = RunOnUIThread([&page]() { VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); }); VERIFY_SUCCEEDED(result); - Log::Comment(NoThrowString().Format(L"Duplicate the first tab")); + Log::Comment(L"Duplicate the first tab"); result = RunOnUIThread([&page]() { page->_DuplicateTabViewItem(); VERIFY_ARE_EQUAL(2u, page->_tabs.Size()); @@ -344,7 +365,7 @@ namespace TerminalAppLocalTests }); VERIFY_SUCCEEDED(result); - Log::Comment(NoThrowString().Format(L"Duplicate the tab, and don't crash")); + Log::Comment(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."); @@ -419,48 +440,9 @@ namespace TerminalAppLocalTests winrt::TerminalApp::TerminalPage projectedPage{ nullptr }; winrt::com_ptr page{ nullptr }; - 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); + _initializeTerminalPage(projectedPage, page, settings0); - VERIFY_IS_NOT_NULL(page); - VERIFY_IS_NOT_NULL(page->_settings); - - Log::Comment(NoThrowString().Format(L"Create() the TerminalPage")); - result = RunOnUIThread([&page]() { - VERIFY_IS_NOT_NULL(page); - VERIFY_IS_NOT_NULL(page->_settings); - page->Create(); - Log::Comment(NoThrowString().Format(L"Create()'d")); - - auto app = ::winrt::Windows::UI::Xaml::Application::Current(); - Log::Comment(NoThrowString().Format(L"got app")); - - winrt::TerminalApp::TerminalPage pp = *page; - winrt::Windows::UI::Xaml::Window::Current().Content(pp); - Log::Comment(NoThrowString().Format(L"Content()'d")); - winrt::Windows::UI::Xaml::Window::Current().Activate(); - Log::Comment(NoThrowString().Format(L"Activate()'d")); - }); - VERIFY_SUCCEEDED(result); - - result = RunOnUIThread([&page]() { - Log::Comment(NoThrowString().Format(L"_SetFocusedTabIndex()...")); - // 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); - Log::Comment(NoThrowString().Format(L"... Done")); - }); - VERIFY_SUCCEEDED(result); - result = RunOnUIThread([&page]() { + auto result = RunOnUIThread([&page]() { VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); }); VERIFY_SUCCEEDED(result); From 9cd77e8cb71ed5cb93590bdf1c71575ff698fba0 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 23 Mar 2020 15:41:29 -0500 Subject: [PATCH 28/32] code cleanup for review --- src/cascadia/TerminalApp/TerminalPage.cpp | 52 ++++++++++++++--------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 967dbac077a..477f18858ce 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -154,15 +154,28 @@ namespace winrt::TerminalApp::implementation _layoutUpdatedRevoker = _tabContent.LayoutUpdated(winrt::auto_revoke, { this, &TerminalPage::_OnFirstLayout }); } + // Method Description: + // - This method is caled once on startup, on the first LayoutUpdated event. + // We'll use this event to know that we have an ActualWidth and + // ActualHeight, so we can now attempt to process our list of startup + // actions. + // - We'll remove this event handler when the event is first handled. + // - If there are no startup actions, we'll open a single tab with the + // default profile. + // Arguments: + // - + // Return Value: + // - void TerminalPage::_OnFirstLayout(const IInspectable& /*sender*/, const IInspectable& /*eventArgs*/) { // Only let this succeed once. _layoutUpdatedRevoker.revoke(); - // This event fires every time the layout changes, but it is always the last one to fire - // in any layout change chain. That gives us great flexibility in finding the right point - // at which to initialize our renderer (and our terminal). - // Any earlier than the last layout update and we may not know the terminal's starting size. + // This event fires every time the layout changes, but it is always the + // last one to fire in any layout change chain. That gives us great + // flexibility in finding the right point at which to initialize our + // renderer (and our terminal). Any earlier than the last layout update + // and we may not know the terminal's starting size. if (_startupState == StartupState::NotInitialized) { _startupState = StartupState::InStartup; @@ -615,17 +628,8 @@ namespace winrt::TerminalApp::implementation if (isFirstTab) { _tabContent.Children().Clear(); - const Windows::Foundation::Size actualSize{ gsl::narrow_cast(_tabContent.ActualWidth()), - gsl::narrow_cast(_tabContent.ActualHeight()) }; auto tabRootElem = newTabImpl->GetRootElement(); _tabContent.Children().Append(tabRootElem); - tabRootElem.Measure(actualSize); - Windows::Foundation::Size desiredSize = tabRootElem.DesiredSize(); - desiredSize; - ; - auto a = 0; - a += 1; - a; } else { @@ -961,7 +965,6 @@ namespace winrt::TerminalApp::implementation // we clamp the values to the range [0, tabCount) while still supporting moving // leftward from 0 to tabCount - 1. const auto newTabIndex = ((tabCount + *index + (bMoveRight ? 1 : -1)) % tabCount); - // _SetFocusedTabIndex(newTabIndex); _SelectTab(newTabIndex); } } @@ -969,6 +972,11 @@ namespace winrt::TerminalApp::implementation // Method Description: // - Sets focus to the desired tab. Returns false if the provided tabIndex // is greater than the number of tabs we have. + // - During startup, we'll immediately set the selected tab as focused. + // - After startup, we'll dispatch an async method to set the the selected + // item of the TabView, which will then also trigger a + // TabView::SelectionChanged, handled in + // TerminalPage::_OnTabSelectionChanged // Return Value: // true iff we were able to select that tab index, false otherwise bool TerminalPage::_SelectTab(const uint32_t tabIndex) @@ -1038,6 +1046,16 @@ namespace winrt::TerminalApp::implementation return std::nullopt; } + // Method Description: + // - An async method for changing the focused tab on the UI thread. This + // method will _only_ set the selected item of the TabView, which will + // then also trigger a TabView::SelectionChanged event, which we'll handle + // in TerminalPage::_OnTabSelectionChanged, where we'll mark the new tab + // as focused. + // Arguments: + // - tabIndex: the index in the list of tabs to focus. + // Return Value: + // - winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(const uint32_t tabIndex) { // GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex) @@ -1050,12 +1068,6 @@ namespace winrt::TerminalApp::implementation { auto tab{ _GetStrongTabImpl(tabIndex) }; _tabView.SelectedItem(tab->GetTabViewItem()); - - // If we're in startup, we're not going to come back through the dispatcher to handle - // if (page->_startupState == StartupState::InStartup) - // { - // page->_UpdatedSelectedTab(tabIndex); - // } } } From 7507f9f7880286e8e0c6871ba5ceb00ecdc0569a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 23 Mar 2020 16:01:04 -0500 Subject: [PATCH 29/32] finalize code cleanup --- src/cascadia/TerminalApp/Pane.cpp | 49 +++---------------- src/cascadia/TerminalApp/Pane.h | 4 +- src/cascadia/TerminalApp/Tab.cpp | 15 ++++++ src/cascadia/TerminalApp/Tab.h | 1 - src/cascadia/TerminalApp/TerminalPage.cpp | 40 +++++++++------ .../TerminalControl/TSFInputControl.cpp | 3 ++ 6 files changed, 52 insertions(+), 60 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 3e7b73c787f..47bb02f1275 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -70,7 +70,7 @@ void Pane::ResizeContent(const Size& newSize) const auto width = newSize.Width; const auto height = newSize.Height; - _CreateRowColDefinitions(newSize); + _CreateRowColDefinitions(); if (_splitState == SplitState::Vertical) { @@ -790,26 +790,23 @@ void Pane::_SetupChildCloseHandlers() // which is stored in _desiredSplitPosition // - Does nothing if our split state is currently set to SplitState::None // Arguments: -// - rootSize: The dimensions in pixels that this pane (and its children should consume.) +// - // Return Value: // - -void Pane::_CreateRowColDefinitions(const Size& /*rootSize*/) +void Pane::_CreateRowColDefinitions() { - auto first = _desiredSplitPosition * 100.0f; - auto second = 100.0f - first; + const auto first = _desiredSplitPosition * 100.0f; + const auto second = 100.0f - first; if (_splitState == SplitState::Vertical) { _root.ColumnDefinitions().Clear(); // Create two columns in this grid: one for each pane - // const auto paneSizes = _CalcChildrenSizes(rootSize.Width); auto firstColDef = Controls::ColumnDefinition(); - // firstColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); firstColDef.Width(GridLengthHelper::FromValueAndType(first, GridUnitType::Star)); auto secondColDef = Controls::ColumnDefinition(); - // secondColDef.Width(GridLengthHelper::FromValueAndType(paneSizes.second, GridUnitType::Star)); secondColDef.Width(GridLengthHelper::FromValueAndType(second, GridUnitType::Star)); _root.ColumnDefinitions().Append(firstColDef); @@ -820,15 +817,12 @@ void Pane::_CreateRowColDefinitions(const Size& /*rootSize*/) _root.RowDefinitions().Clear(); // Create two rows in this grid: one for each pane - // const auto paneSizes = _CalcChildrenSizes(rootSize.Height); auto firstRowDef = Controls::RowDefinition(); firstRowDef.Height(GridLengthHelper::FromValueAndType(first, GridUnitType::Star)); - // firstRowDef.Height(GridLengthHelper::FromValueAndType(paneSizes.first, GridUnitType::Star)); auto secondRowDef = Controls::RowDefinition(); secondRowDef.Height(GridLengthHelper::FromValueAndType(second, GridUnitType::Star)); - // secondRowDef.Height(GridLengthHelper::FromValueAndType(paneSizes.second, GridUnitType::Star)); _root.RowDefinitions().Append(firstRowDef); _root.RowDefinitions().Append(secondRowDef); @@ -845,10 +839,7 @@ void Pane::_CreateRowColDefinitions(const Size& /*rootSize*/) // - void Pane::_CreateSplitContent() { - Size actualSize{ gsl::narrow_cast(_root.ActualWidth()), - gsl::narrow_cast(_root.ActualHeight()) }; - - _CreateRowColDefinitions(actualSize); + _CreateRowColDefinitions(); } // Method Description: @@ -959,21 +950,11 @@ std::pair, std::shared_ptr> Pane::Split(SplitState s { if (_firstChild->_HasFocusedChild()) { - auto pretranslatedSplitType = splitType; - if (_firstChild->_IsLeaf() && _isSizeZero()) - { - pretranslatedSplitType = _preTranslateAutoSplitType(splitType); - } - return _firstChild->Split(pretranslatedSplitType, profile, control); + return _firstChild->Split(splitType, profile, control); } else if (_secondChild->_HasFocusedChild()) { - auto pretranslatedSplitType = splitType; - if (_secondChild->_IsLeaf() && _isSizeZero()) - { - pretranslatedSplitType = _preTranslateAutoSplitType(splitType); - } - return _secondChild->Split(pretranslatedSplitType, profile, control); + return _secondChild->Split(splitType, profile, control); } return { nullptr, nullptr }; @@ -1007,20 +988,6 @@ SplitState Pane::_convertAutomaticSplitState(const SplitState& splitType) const return splitType; } -SplitState Pane::_preTranslateAutoSplitType(const SplitState& splitType) const -{ - if (splitType == SplitState::Automatic && _isSizeZero()) - { - return _splitState == SplitState::Horizontal ? SplitState::Vertical : SplitState::Horizontal; - } - return splitType; -} - -bool Pane::_isSizeZero() const -{ - return _root.ActualWidth() == 0 && _root.ActualHeight() == 0; -} - // Method Description: // - Determines whether the pane can be split. // Arguments: diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index bd6d0ccc20c..707098876b4 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -110,7 +110,7 @@ class Pane : public std::enable_shared_from_this const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - void _CreateRowColDefinitions(const winrt::Windows::Foundation::Size& rootSize); + void _CreateRowColDefinitions(); void _CreateSplitContent(); void _ApplySplitDefinitions(); void _UpdateBorders(); @@ -137,8 +137,6 @@ class Pane : public std::enable_shared_from_this winrt::TerminalApp::SplitState _convertAutomaticSplitState(const winrt::TerminalApp::SplitState& splitType) const; - winrt::TerminalApp::SplitState _preTranslateAutoSplitType(const winrt::TerminalApp::SplitState& splitType) const; - bool _isSizeZero() const; std::optional _preCalculateAutoSplit(const std::shared_ptr target, const winrt::Windows::Foundation::Size parentSize) const; // Function Description: diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 15260a49819..fa930ad3a43 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -393,6 +393,14 @@ namespace winrt::TerminalApp::implementation }); } + // Method Description: + // - Mark the given pane as the active pane in this tab. All other panes + // will be marked as inactive. We'll also update our own UI state to + // reflect this newly active pane. + // Arguments: + // - pane: a Pane to mark as active. + // Return Value: + // - void Tab::_UpdateActivePane(std::shared_ptr pane) { // Clear the active state of the entire tree, and mark only the pane as active. @@ -431,6 +439,13 @@ namespace winrt::TerminalApp::implementation }); } + // Method Description: + // - Get the total number of leaf panes in this tab. This will be the number + // of actual controls hosted by this tab. + // Arguments: + // - + // Return Value: + // - The total number of leaf panes hosted by this tab. int Tab::_GetLeafPaneCount() const noexcept { return _rootPane->GetLeafPaneCount(); diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index a0d55c31e74..93817b8f57b 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -26,7 +26,6 @@ namespace winrt::TerminalApp::implementation winrt::Windows::UI::Xaml::UIElement GetRootElement(); winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const; std::optional GetFocusedProfile() const noexcept; - // std::weak_ptr GetActivePane() const noexcept; bool IsFocused() const noexcept; void SetFocused(const bool focused); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 477f18858ce..bc5d5ce4bf3 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -547,11 +547,6 @@ namespace winrt::TerminalApp::implementation 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) @@ -867,6 +862,19 @@ namespace winrt::TerminalApp::implementation try { auto focusedTab = _GetStrongTabImpl(*index); + // TODO: GH#5047 - In the future, we should get the Profile of + // the focused pane, and use that to build a new instance of the + // settings so we can duplicate this tab/pane. + // + // Currently, if the profile doesn't exist anymore in our + // settings, we'll silently do nothing. + // + // In the future, it will be preferable to just duplicate the + // current control's settings, but we can't do that currently, + // because we won't be able to create a new instance of the + // connection without keeing an instance of the original Profile + // object around. + const auto& profileGuid = focusedTab->GetFocusedProfile(); if (profileGuid.has_value()) { @@ -875,11 +883,6 @@ namespace winrt::TerminalApp::implementation } } 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. } } @@ -1180,6 +1183,18 @@ namespace winrt::TerminalApp::implementation controlSettings = _settings->BuildSettings(current_guid.value()); realGuid = current_guid.value(); } + // TODO: GH#5047 - In the future, we should get the Profile of + // the focused pane, and use that to build a new instance of the + // settings so we can duplicate this tab/pane. + // + // Currently, if the profile doesn't exist anymore in our + // settings, we'll silently do nothing. + // + // In the future, it will be preferable to just duplicate the + // current control's settings, but we can't do that currently, + // because we won't be able to create a new instance of the + // connection without keeing an instance of the original Profile + // object around. } if (!profileFound) { @@ -1211,11 +1226,6 @@ namespace winrt::TerminalApp::implementation focusedTab->SplitPane(realSplitType, 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: diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index c69f3c7e360..2b909983f49 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -174,6 +174,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation const auto canvasActualWidth = Canvas().ActualWidth(); const auto widthToTerminalEnd = canvasActualWidth - ::base::ClampedNumeric(clientCursorPos.X); + // Make sure that we're setting the MaxWidth to a positive number - a + // negative number here will crash us in mysterious ways with a useless + // stack trace const auto newMaxWidth = std::max(0.0, widthToTerminalEnd); TextBlock().MaxWidth(newMaxWidth); From 393492bec81364ee79eaacfcf9fb08694e1ad4db Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 24 Mar 2020 09:47:12 -0500 Subject: [PATCH 30/32] Tons of non-test code cleanup --- src/cascadia/TerminalApp/Pane.cpp | 22 +----- src/cascadia/TerminalApp/Pane.h | 1 - src/cascadia/TerminalApp/Tab.cpp | 3 +- src/cascadia/TerminalApp/TerminalPage.cpp | 89 ++++++++++------------- src/cascadia/TerminalApp/TerminalPage.h | 2 +- 5 files changed, 43 insertions(+), 74 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 47bb02f1275..90fd9e63894 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -829,19 +829,6 @@ void Pane::_CreateRowColDefinitions() } } -// Method Description: -// - Initializes our UI for a new split in this pane. Sets up row/column -// definitions, and initializes the separator grid. Does nothing if our split -// state is currently set to SplitState::None -// Arguments: -// - -// Return Value: -// - -void Pane::_CreateSplitContent() -{ - _CreateRowColDefinitions(); -} - // Method Description: // - Sets the thickness of each side of our borders to match our _borders state. // Arguments: @@ -999,9 +986,6 @@ bool Pane::_CanSplit(SplitState splitType) const Size actualSize{ gsl::narrow_cast(_root.ActualWidth()), gsl::narrow_cast(_root.ActualHeight()) }; - const Size desiredSize = _root.DesiredSize(); - desiredSize; - const Size minSize = _GetMinSize(); auto actualSplitType = _convertAutomaticSplitState(splitType); @@ -1077,7 +1061,7 @@ std::pair, std::shared_ptr> Pane::_Split(SplitState _control = { nullptr }; _secondChild = std::make_shared(profile, control); - _CreateSplitContent(); + _CreateRowColDefinitions(); _root.Children().Append(_firstChild->GetRootElement()); _root.Children().Append(_secondChild->GetRootElement()); @@ -1587,7 +1571,9 @@ std::optional Pane::PreCalculateAutoSplit(const return firstResult.has_value() ? firstResult : _secondChild->PreCalculateAutoSplit(target, { secondWidth, secondHeight }); } - return std::nullopt; + // We should not possibly be getting here - both the above branches should + // return a value. + FAIL_FAST(); } DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate>); diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 707098876b4..7223e35b540 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -111,7 +111,6 @@ class Pane : public std::enable_shared_from_this const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); void _CreateRowColDefinitions(); - void _CreateSplitContent(); void _ApplySplitDefinitions(); void _UpdateBorders(); diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index fa930ad3a43..1ba4142b9c2 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -463,8 +463,7 @@ namespace winrt::TerminalApp::implementation // `availableSpace` SplitState Tab::PreCalculateAutoSplit(winrt::Windows::Foundation::Size availableSpace) const { - const auto result = _rootPane->PreCalculateAutoSplit(_activePane, availableSpace); - return result.has_value() ? result.value() : SplitState::Vertical; + return _rootPane->PreCalculateAutoSplit(_activePane, availableSpace).value_or(SplitState::Vertical); } DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 7c0aed4b1f4..cb47ad2511f 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -155,7 +155,7 @@ namespace winrt::TerminalApp::implementation } // Method Description: - // - This method is caled once on startup, on the first LayoutUpdated event. + // - This method is called once on startup, on the first LayoutUpdated event. // We'll use this event to know that we have an ActualWidth and // ActualHeight, so we can now attempt to process our list of startup // actions. @@ -200,7 +200,7 @@ namespace winrt::TerminalApp::implementation // - // Return Value: // - - fire_and_forget TerminalPage::_ProcessStartupActions() + winrt::fire_and_forget TerminalPage::_ProcessStartupActions() { // If there are no actions left, do nothing. if (_appArgs.GetStartupActions().empty()) @@ -209,13 +209,13 @@ namespace winrt::TerminalApp::implementation } auto weakThis{ get_weak() }; - // Handle it on the UI thread. - co_await winrt::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low); + // Handle it on a subsequent pass of the UI thread. + co_await winrt::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal); if (auto page{ weakThis.get() }) { - for (const auto& action : page->_appArgs.GetStartupActions()) + for (const auto& action : _appArgs.GetStartupActions()) { - page->_actionDispatch->DoAction(action); + _actionDispatch->DoAction(action); } _startupState = StartupState::Initialized; _InitializedHandlers(*this, nullptr); @@ -521,33 +521,31 @@ namespace winrt::TerminalApp::implementation // control which profile is created and with possible other // configurations. See CascadiaSettings::BuildSettings for more details. void TerminalPage::_OpenNewTab(const winrt::TerminalApp::NewTerminalArgs& newTerminalArgs) + try { - try - { - const auto [profileGuid, settings] = _settings->BuildSettings(newTerminalArgs); - - _CreateNewTabFromSettings(profileGuid, settings); - - const uint32_t tabCount = _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"), - TraceLoggingUInt32(1u, "EventVer", "Version of this event"), - TraceLoggingUInt32(tabCount, "TabCount", "Count of tabs currently 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"), - TraceLoggingBool(settings.UseAcrylic(), "UseAcrylic", "The acrylic preference from the settings"), - TraceLoggingFloat64(settings.TintOpacity(), "TintOpacity", "Opacity preference from the settings"), - TraceLoggingWideString(settings.FontFace().c_str(), "FontFace", "Font face chosen in the settings"), - TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), - TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance)); - } - CATCH_LOG(); + const auto [profileGuid, settings] = _settings->BuildSettings(newTerminalArgs); + + _CreateNewTabFromSettings(profileGuid, settings); + + const uint32_t tabCount = _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"), + TraceLoggingUInt32(1u, "EventVer", "Version of this event"), + TraceLoggingUInt32(tabCount, "TabCount", "Count of tabs currently 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"), + TraceLoggingBool(settings.UseAcrylic(), "UseAcrylic", "The acrylic preference from the settings"), + TraceLoggingFloat64(settings.TintOpacity(), "TintOpacity", "Opacity preference from the settings"), + TraceLoggingWideString(settings.FontFace().c_str(), "FontFace", "Font face chosen in the settings"), + TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), + TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance)); } + CATCH_LOG(); winrt::fire_and_forget TerminalPage::_RemoveOnCloseRoutine(Microsoft::UI::Xaml::Controls::TabViewItem tabViewItem, winrt::com_ptr page) { @@ -563,7 +561,6 @@ namespace winrt::TerminalApp::implementation // - settings: the TerminalSettings object to use to create the TerminalControl with. void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, TerminalSettings settings) { - const bool isFirstTab = _tabs.Size() == 0; // Initialize the new tab // Create a connection based on the values in our settings object. @@ -617,21 +614,9 @@ namespace winrt::TerminalApp::implementation } }); - // If this is the first tab, we don't need to kick off the event to get - // the tab's content added to the root of the page. just do it - // immediately. - if (isFirstTab) - { - _tabContent.Children().Clear(); - auto tabRootElem = newTabImpl->GetRootElement(); - _tabContent.Children().Append(tabRootElem); - } - else - { - // This kicks off TabView::SelectionChanged, in response to which - // we'll attach the terminal's Xaml control to the Xaml root. - _tabView.SelectedItem(tabViewItem); - } + // This kicks off TabView::SelectionChanged, in response to which + // we'll attach the terminal's Xaml control to the Xaml root. + _tabView.SelectedItem(tabViewItem); } // Method Description: @@ -1660,13 +1645,13 @@ namespace winrt::TerminalApp::implementation } } 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. } + // GH#2455: If there are any panes with controls that had been + // initialized with a Profile that no longer exists in our list of + // profiles, we'll leave it unmodified. The profile doesn't exist + // anymore, so we can't possibly update its settings. + // Update the icon of the tab for the currently focused profile in that tab. for (auto tab : _tabs) { diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 108451f61ce..09c4fad95ed 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -88,7 +88,7 @@ namespace winrt::TerminalApp::implementation ::TerminalApp::AppCommandlineArgs _appArgs; int _ParseArgs(winrt::array_view& args); - fire_and_forget _ProcessStartupActions(); + winrt::fire_and_forget _ProcessStartupActions(); void _ShowAboutDialog(); void _ShowCloseWarningDialog(); From a1c7f7e69eb9640c155057c9589553c879e97081 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 24 Mar 2020 09:47:28 -0500 Subject: [PATCH 31/32] wacky--; --- .../LocalTests_TerminalApp/TabTests.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 01e1088bebb..9c744e595e8 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -67,8 +67,7 @@ namespace TerminalAppLocalTests } private: - void _initializeTerminalPage(winrt::TerminalApp::TerminalPage& projectedPage, - winrt::com_ptr& page, + void _initializeTerminalPage(winrt::com_ptr& page, std::shared_ptr initialSettings); }; @@ -208,13 +207,11 @@ namespace TerminalAppLocalTests // * It will also ensure that the first tab is focused, since that happens // asynchronously in the application typically. // Arguments: - // - projectedPage: a TerminalPage projected type that will recieve the new TerminalPage instance // - page: a TerminalPage implementation ptr that will recieve the new TerminalPage instance // - initialSettings: a CascadiaSettings to initialize the TerminalPage with. // Return Value: // - - void TabTests::_initializeTerminalPage(winrt::TerminalApp::TerminalPage& projectedPage, - winrt::com_ptr& page, + void TabTests::_initializeTerminalPage(winrt::com_ptr& page, std::shared_ptr initialSettings) { // This is super wacky, but we can't just initialize the @@ -225,6 +222,8 @@ namespace TerminalAppLocalTests // 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 }; + Log::Comment(NoThrowString().Format(L"Construct the TerminalPage")); auto result = RunOnUIThread([&projectedPage, &page, initialSettings]() { projectedPage = winrt::TerminalApp::TerminalPage(); @@ -340,10 +339,8 @@ namespace TerminalAppLocalTests // 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 }; - - _initializeTerminalPage(projectedPage, page, settings0); + _initializeTerminalPage(page, settings0); auto result = RunOnUIThread([&page]() { VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); @@ -437,10 +434,8 @@ namespace TerminalAppLocalTests // 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 }; - - _initializeTerminalPage(projectedPage, page, settings0); + _initializeTerminalPage(page, settings0); auto result = RunOnUIThread([&page]() { VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); From c3facb5fd00ed0aae393ea9be79c46670c357e0e Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 25 Mar 2020 16:58:29 -0700 Subject: [PATCH 32/32] spelling --- src/cascadia/LocalTests_TerminalApp/TabTests.cpp | 4 ++-- src/cascadia/TerminalApp/Tab.cpp | 2 +- src/cascadia/TerminalApp/TerminalPage.cpp | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index bf36f1d0e3e..e7886178ec5 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -200,14 +200,14 @@ namespace TerminalAppLocalTests // non-zero ActualSize to the Terminal window, and adding the Page to // the Application will make it behave as expected. // * It will wait for the TerminalPage to finish initialization before - // returning control to the caler. It does this by creating an event and + // returning control to the caller. It does this by creating an event and // only setting the event when the TerminalPage raises its Initialized // event, to signal that startup is complete. At this point, there will // be one tab with the default profile in the page. // * It will also ensure that the first tab is focused, since that happens // asynchronously in the application typically. // Arguments: - // - page: a TerminalPage implementation ptr that will recieve the new TerminalPage instance + // - page: a TerminalPage implementation ptr that will receive the new TerminalPage instance // - initialSettings: a CascadiaSettings to initialize the TerminalPage with. // Return Value: // - diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 1ba4142b9c2..1fb49c719a7 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -280,7 +280,7 @@ namespace winrt::TerminalApp::implementation // Immediately update our tracker of the focused pane now. If we're // splitting panes during startup (from a commandline), then it's - // possible that the focus events won't propogate immediately. Updating + // possible that the focus events won't propagate immediately. Updating // the focus here will give the same effect though. _UpdateActivePane(second); } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index a235a4d918f..0f2773402dd 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -858,7 +858,7 @@ namespace winrt::TerminalApp::implementation // In the future, it will be preferable to just duplicate the // current control's settings, but we can't do that currently, // because we won't be able to create a new instance of the - // connection without keeing an instance of the original Profile + // connection without keeping an instance of the original Profile // object around. const auto& profileGuid = focusedTab->GetFocusedProfile(); @@ -1179,7 +1179,7 @@ namespace winrt::TerminalApp::implementation // In the future, it will be preferable to just duplicate the // current control's settings, but we can't do that currently, // because we won't be able to create a new instance of the - // connection without keeing an instance of the original Profile + // connection without keeping an instance of the original Profile // object around. } if (!profileFound)