From 9700598ecba9b30e886a595c5696212b13048ea4 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 22 Jan 2021 10:21:18 -0800 Subject: [PATCH] Bugfix: sync color scheme rename with profile reference (#8793) ## Summary of the Pull Request This fixes a bug where renaming/deleting a color scheme would not update profiles that referenced it. This also adds detection for renaming a color scheme to a name that is already in use, and adds appropriate UI for that. ## References #6800 - Settings UI Epic ## PR Checklist * [X] Closes #8756 ## Detailed Description of the Pull Request / Additional comments `Model::CascadiaSettings` was updated to have a `UpdateColorSchemeReferences()` function that updates all profiles referencing the newly renamed color scheme. `Editor::ColorSchemesPageNavigationState` now takes and exposes a `Model::CascadiaSettings`. When a color scheme is renamed or deleted, we use `CascadiaSettings` to update our list of color schemes appropriately, then call `UpdateColorSchemeReferences()` to update the profiles. The tricky part is that `Profile` does not store a direct reference to `ColorScheme`, but rather the name of the color scheme. See [this tread](https://github.com/microsoft/terminal/issues/8756#issuecomment-760375027) for a discussion on this topic. ## Validation Steps Performed Repro steps from #8756 when renaming/deleting a referenced color scheme. ## Demo ![Scheme Name Already In Use Demo](https://user-images.githubusercontent.com/11050425/105431427-6e023980-5c0a-11eb-894a-42152fc77f05.gif) --- .../TerminalSettingsEditor/ColorSchemes.cpp | 35 ++++++++++++++++--- .../TerminalSettingsEditor/ColorSchemes.h | 6 ++-- .../TerminalSettingsEditor/ColorSchemes.idl | 2 +- .../TerminalSettingsEditor/ColorSchemes.xaml | 10 ++++++ .../TerminalSettingsEditor/MainPage.cpp | 4 +-- .../Resources/en-US/Resources.resw | 6 ++++ .../CascadiaSettings.cpp | 19 ++++++++++ .../TerminalSettingsModel/CascadiaSettings.h | 1 + .../CascadiaSettings.idl | 1 + 9 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp b/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp index fdbe93005..37ab687d5 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp @@ -129,7 +129,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { // Surprisingly, though this is called every time we navigate to the page, // the list does not keep growing on each navigation. - const auto& colorSchemeMap{ _State.Globals().ColorSchemes() }; + const auto& colorSchemeMap{ _State.Settings().GlobalSettings().ColorSchemes() }; for (const auto& pair : colorSchemeMap) { _ColorSchemeList.Append(pair.Value()); @@ -174,7 +174,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void ColorSchemes::DeleteConfirmation_Click(IInspectable const& /*sender*/, RoutedEventArgs const& /*e*/) { const auto schemeName{ CurrentColorScheme().Name() }; - _State.Globals().RemoveColorScheme(schemeName); + _State.Settings().GlobalSettings().RemoveColorScheme(schemeName); + + // This ensures that the JSON is updated with "Campbell", because the color scheme was deleted + _State.Settings().UpdateColorSchemeReferences(schemeName, L"Campbell"); const auto removedSchemeIndex{ ColorSchemeComboBox().SelectedIndex() }; if (static_cast(removedSchemeIndex) < _ColorSchemeList.Size() - 1) @@ -194,11 +197,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void ColorSchemes::AddNew_Click(IInspectable const& /*sender*/, RoutedEventArgs const& /*e*/) { // Give the new scheme a distinct name - const hstring schemeName{ fmt::format(L"Color Scheme {}", _State.Globals().ColorSchemes().Size() + 1) }; + const hstring schemeName{ fmt::format(L"Color Scheme {}", _State.Settings().GlobalSettings().ColorSchemes().Size() + 1) }; Model::ColorScheme scheme{ schemeName }; // Add the new color scheme - _State.Globals().AddColorScheme(scheme); + _State.Settings().GlobalSettings().AddColorScheme(scheme); // Update current page _ColorSchemeList.Append(scheme); @@ -227,6 +230,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void ColorSchemes::RenameCancel_Click(IInspectable const& /*sender*/, RoutedEventArgs const& /*e*/) { IsRenaming(false); + RenameErrorTip().IsOpen(false); } void ColorSchemes::NameBox_PreviewKeyDown(IInspectable const& /*sender*/, winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e) @@ -239,12 +243,35 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation else if (e.OriginalKey() == winrt::Windows::System::VirtualKey::Escape) { IsRenaming(false); + RenameErrorTip().IsOpen(false); e.Handled(true); } } void ColorSchemes::_RenameCurrentScheme(hstring newName) { + // check if different name is already in use + const auto oldName{ CurrentColorScheme().Name() }; + if (newName != oldName && _State.Settings().GlobalSettings().ColorSchemes().HasKey(newName)) + { + // open the error tip + RenameErrorTip().Target(NameBox()); + RenameErrorTip().IsOpen(true); + + // focus the name box + NameBox().Focus(FocusState::Programmatic); + NameBox().SelectAll(); + return; + } + + // update the settings model + CurrentColorScheme().Name(newName); + _State.Settings().GlobalSettings().RemoveColorScheme(oldName); + _State.Settings().GlobalSettings().AddColorScheme(CurrentColorScheme()); + _State.Settings().UpdateColorSchemeReferences(oldName, newName); + + // update the UI + RenameErrorTip().IsOpen(false); CurrentColorScheme().Name(newName); IsRenaming(false); diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.h b/src/cascadia/TerminalSettingsEditor/ColorSchemes.h index dbe819208..6f053e995 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.h +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.h @@ -13,10 +13,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation struct ColorSchemesPageNavigationState : ColorSchemesPageNavigationStateT { public: - ColorSchemesPageNavigationState(const Model::GlobalAppSettings& settings) : - _Globals{ settings } {} + ColorSchemesPageNavigationState(const Model::CascadiaSettings& settings) : + _Settings{ settings } {} - GETSET_PROPERTY(Model::GlobalAppSettings, Globals, nullptr); + GETSET_PROPERTY(Model::CascadiaSettings, Settings, nullptr); GETSET_PROPERTY(winrt::hstring, LastSelectedScheme, L""); }; diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl b/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl index 6a4029911..21c80c2b4 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl @@ -6,7 +6,7 @@ namespace Microsoft.Terminal.Settings.Editor runtimeclass ColorSchemesPageNavigationState { - Microsoft.Terminal.Settings.Model.GlobalAppSettings Globals; + Microsoft.Terminal.Settings.Model.CascadiaSettings Settings; String LastSelectedScheme; }; diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml b/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml index fe91879b9..0dddeb106 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml @@ -6,6 +6,7 @@ the MIT License. See LICENSE in the project root for license information. --> xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:local="using:Microsoft.Terminal.Settings.Editor" xmlns:model="using:Microsoft.Terminal.Settings.Model" + xmlns:muxc="using:Microsoft.UI.Xaml.Controls" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" mc:Ignorable="d"> @@ -97,9 +98,17 @@ the MIT License. See LICENSE in the project root for license information. --> + + + + + + + +