From b5370a1da1a5d695a2ffa5628fefe6cd7b31c934 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Sat, 26 Dec 2020 17:11:52 -0600 Subject: [PATCH 1/8] Bugfix: Dropdown shows settings file keybinding --- src/cascadia/TerminalApp/TerminalPage.cpp | 4 +++- src/cascadia/TerminalSettingsModel/ActionArgs.h | 3 +++ src/cascadia/TerminalSettingsModel/ActionArgs.idl | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 3f9b5ee83ff..40fe081a86e 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -602,7 +602,9 @@ namespace winrt::TerminalApp::implementation settingsItem.Click({ this, &TerminalPage::_SettingsButtonOnClick }); newTabFlyout.Items().Append(settingsItem); - auto settingsKeyChord = keyBindings.GetKeyBindingForAction(ShortcutAction::OpenSettings); + Microsoft::Terminal::Settings::Model::OpenSettingsArgs args{ SettingsTarget::SettingsFile }; + Microsoft::Terminal::Settings::Model::ActionAndArgs settingsAction{ ShortcutAction::OpenSettings, args }; + const auto settingsKeyChord{ keyBindings.GetKeyBindingForActionWithArgs(settingsAction) }; if (settingsKeyChord) { _SetAcceleratorForMenuItem(settingsItem, settingsKeyChord); diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h index 8551288e617..7a249330cf0 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h @@ -441,6 +441,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation struct OpenSettingsArgs : public OpenSettingsArgsT { OpenSettingsArgs() = default; + OpenSettingsArgs(const SettingsTarget& target) : + _Target{ target } {} GETSET_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsFile); static constexpr std::string_view TargetKey{ "target" }; @@ -850,4 +852,5 @@ namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation BASIC_FACTORY(CloseOtherTabsArgs); BASIC_FACTORY(CloseTabsAfterArgs); BASIC_FACTORY(MoveTabArgs); + BASIC_FACTORY(OpenSettingsArgs); } diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.idl b/src/cascadia/TerminalSettingsModel/ActionArgs.idl index 2c539babe3d..e4413fbd452 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.idl +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.idl @@ -147,6 +147,7 @@ namespace Microsoft.Terminal.Settings.Model [default_interface] runtimeclass OpenSettingsArgs : IActionArgs { + OpenSettingsArgs(SettingsTarget target); SettingsTarget Target { get; }; }; From 91eb49e20a9ceffa3293b053aba1bd89baf1a758 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Sat, 26 Dec 2020 17:18:25 -0600 Subject: [PATCH 2/8] Bugfix: auto-generate name for settings ui action --- src/cascadia/TerminalSettingsModel/ActionArgs.cpp | 2 ++ .../TerminalSettingsModel/Resources/en-US/Resources.resw | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionArgs.cpp index 04dde967fcb..22744cacfad 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.cpp @@ -292,6 +292,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return RS_(L"OpenDefaultSettingsCommandKey"); case SettingsTarget::AllFiles: return RS_(L"OpenBothSettingsFilesCommandKey"); + case SettingsTarget::SettingsUI: + return RS_(L"OpenSettingsUICommandKey"); default: return RS_(L"OpenSettingsCommandKey"); } diff --git a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw index da183b249dc..a2d6f5853c3 100644 --- a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw @@ -363,4 +363,7 @@ Break into the debugger - + + Open Settings UI + + \ No newline at end of file From 1cadbf4493a43dcd8892144ebf23a299c422726f Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Sat, 26 Dec 2020 17:38:19 -0600 Subject: [PATCH 3/8] Bugfix: avoid crash on discard changes (bad cast) --- src/cascadia/TerminalSettingsEditor/MainPage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index a417779e8c1..f2be2710c96 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -126,7 +126,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // could not find the page we were on, fallback to first menu item const auto firstItem{ navigationMenu.MenuItems().GetAt(0) }; navigationMenu.SelectedItem(firstItem); - if (const auto tag{ navigationMenu.SelectedItem().as().Tag() }) + if (const auto tag{ navigationMenu.SelectedItem().as().Tag() }) { _Navigate(unbox_value(tag)); } From dd2f3e5b5997422779ee7d693c03e03b862d364e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Sat, 26 Dec 2020 17:56:56 -0600 Subject: [PATCH 4/8] Bugfix: allow refreshed profile page to be deleted --- src/cascadia/TerminalSettingsEditor/MainPage.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index f2be2710c96..05fd1cd3039 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -104,13 +104,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ selectedItem.as().Tag() }) { - if (const auto profile{ tag.try_as() }) + if (const auto oldProfile{ tag.try_as() }) { // check if the profile still exists - if (_settingsClone.FindProfile(profile.Guid())) + if (const auto profile{ _settingsClone.FindProfile(oldProfile.Guid()) }) { // Navigate to the page with the given profile - contentFrame().Navigate(xaml_typename(), winrt::make(_viewModelForProfile(profile), _settingsClone.GlobalSettings().ColorSchemes(), *this)); + _Navigate(_viewModelForProfile(profile)); return; } } From 1174aabe8844c822b19ca607c257fc993762d32c Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 29 Dec 2020 12:38:49 -0600 Subject: [PATCH 5/8] Remove NavViewItem visibility hack --- .../TerminalSettingsEditor/MainPage.cpp | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index 05fd1cd3039..fec4af35a4a 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -64,11 +64,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation co_await winrt::resume_foreground(Dispatcher()); - // "remove" all profile-related NavViewItems - // LOAD-BEARING: use Visibility here, instead of menuItems.Remove(). - // Remove() works fine on NavViewItems with an hstring tag, - // but causes an out-of-bounds error with Profile tagged items. - // The cause of this error is unknown. + // remove all profile-related NavViewItems auto menuItems{ SettingsNav().MenuItems() }; for (auto i = menuItems.Size() - 1; i > 0; --i) { @@ -76,17 +72,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ navViewItem.Tag() }) { - if (tag.try_as()) + if (tag.try_as()) { - // hide NavViewItem pointing to a Profile - navViewItem.Visibility(Visibility::Collapsed); + // remove NavViewItem pointing to a Profile + menuItems.RemoveAt(i); } else if (const auto stringTag{ tag.try_as() }) { if (stringTag == addProfileTag) { - // hide NavViewItem pointing to "Add Profile" - navViewItem.Visibility(Visibility::Collapsed); + // remove NavViewItem pointing to "Add Profile" + menuItems.RemoveAt(i); } } } @@ -104,7 +100,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ selectedItem.as().Tag() }) { - if (const auto oldProfile{ tag.try_as() }) + if (const auto oldProfile{ tag.try_as() }) { // check if the profile still exists if (const auto profile{ _settingsClone.FindProfile(oldProfile.Guid()) }) From ff84ee90e4812a61acbc4e9394f617b22953f093 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 29 Dec 2020 12:53:04 -0600 Subject: [PATCH 6/8] Revert "Remove NavViewItem visibility hack" This reverts commit 1174aabe8844c822b19ca607c257fc993762d32c. --- .../TerminalSettingsEditor/MainPage.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index fec4af35a4a..05fd1cd3039 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -64,7 +64,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation co_await winrt::resume_foreground(Dispatcher()); - // remove all profile-related NavViewItems + // "remove" all profile-related NavViewItems + // LOAD-BEARING: use Visibility here, instead of menuItems.Remove(). + // Remove() works fine on NavViewItems with an hstring tag, + // but causes an out-of-bounds error with Profile tagged items. + // The cause of this error is unknown. auto menuItems{ SettingsNav().MenuItems() }; for (auto i = menuItems.Size() - 1; i > 0; --i) { @@ -72,17 +76,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ navViewItem.Tag() }) { - if (tag.try_as()) + if (tag.try_as()) { - // remove NavViewItem pointing to a Profile - menuItems.RemoveAt(i); + // hide NavViewItem pointing to a Profile + navViewItem.Visibility(Visibility::Collapsed); } else if (const auto stringTag{ tag.try_as() }) { if (stringTag == addProfileTag) { - // remove NavViewItem pointing to "Add Profile" - menuItems.RemoveAt(i); + // hide NavViewItem pointing to "Add Profile" + navViewItem.Visibility(Visibility::Collapsed); } } } @@ -100,7 +104,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ selectedItem.as().Tag() }) { - if (const auto oldProfile{ tag.try_as() }) + if (const auto oldProfile{ tag.try_as() }) { // check if the profile still exists if (const auto profile{ _settingsClone.FindProfile(oldProfile.Guid()) }) From 9fea6deeb56970cb77c8f9bd8b87885dceb76bba Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 29 Dec 2020 12:58:45 -0600 Subject: [PATCH 7/8] Properly cast NavViewItem Profile tags --- src/cascadia/TerminalSettingsEditor/MainPage.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index 05fd1cd3039..fb6b12a449f 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -76,7 +76,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ navViewItem.Tag() }) { - if (tag.try_as()) + if (tag.try_as()) { // hide NavViewItem pointing to a Profile navViewItem.Visibility(Visibility::Collapsed); @@ -104,7 +104,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { if (const auto tag{ selectedItem.as().Tag() }) { - if (const auto oldProfile{ tag.try_as() }) + if (const auto oldProfile{ tag.try_as() }) { // check if the profile still exists if (const auto profile{ _settingsClone.FindProfile(oldProfile.Guid()) }) From 1df700823cafd3252bf42d27098a3958bc3505e3 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 4 Jan 2021 12:52:48 -0800 Subject: [PATCH 8/8] Apply suggestions from code review --- .../TerminalSettingsModel/Resources/en-US/Resources.resw | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw index a2d6f5853c3..88ea5ac30ed 100644 --- a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw @@ -364,6 +364,6 @@ Break into the debugger - Open Settings UI + Open Settings... - \ No newline at end of file +