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)
This commit is contained in:
Carlos Zamora 2021-01-22 10:21:18 -08:00 committed by GitHub
parent 124cbd9e47
commit 9700598ecb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 74 additions and 10 deletions

View file

@ -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<uint32_t>(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);

View file

@ -13,10 +13,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
struct ColorSchemesPageNavigationState : ColorSchemesPageNavigationStateT<ColorSchemesPageNavigationState>
{
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"");
};

View file

@ -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;
};

View file

@ -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. -->
<StackPanel Orientation="Horizontal"
Visibility="{x:Bind IsRenaming, Mode=OneWay}">
<!--Shown when color scheme name is already in use-->
<muxc:TeachingTip x:Name="RenameErrorTip"
x:Uid="ColorScheme_RenameErrorTip"/>
<!--Name text box-->
<TextBox x:Name="NameBox"
Style="{StaticResource TextBoxSettingStyle}"
PreviewKeyDown="NameBox_PreviewKeyDown"/>
<!--Accept rename button-->
<Button x:Uid="RenameAccept"
Style="{StaticResource AccentSmallButtonStyle}"
Click="RenameAccept_Click">
@ -108,6 +117,7 @@ the MIT License. See LICENSE in the project root for license information. -->
FontSize="{StaticResource StandardIconSize}"/>
</StackPanel>
</Button>
<!--Cancel rename button-->
<Button x:Uid="RenameCancel"
Style="{StaticResource SmallButtonStyle}"
Click="RenameCancel_Click">

View file

@ -50,7 +50,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_InitializeProfilesList();
_colorSchemesNavState = winrt::make<ColorSchemesPageNavigationState>(_settingsClone.GlobalSettings());
_colorSchemesNavState = winrt::make<ColorSchemesPageNavigationState>(_settingsClone);
}
// Method Description:
@ -110,7 +110,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// Repopulate profile-related menu items
_InitializeProfilesList();
// Update the Nav State with the new version of the settings
_colorSchemesNavState.Globals(_settingsClone.GlobalSettings());
_colorSchemesNavState.Settings(_settingsClone);
// now that the menuItems are repopulated,
// refresh the current page using the SelectedItem data we collected before the refresh

View file

@ -832,4 +832,10 @@
<data name="Globals_CopyFormatAll.Content" xml:space="preserve">
<value>Both HTML and RTF</value>
</data>
<data name="ColorScheme_RenameErrorTip.Subtitle" xml:space="preserve">
<value>Please choose a different name.</value>
</data>
<data name="ColorScheme_RenameErrorTip.Title" xml:space="preserve">
<value>This color scheme name is already in use.</value>
</data>
</root>

View file

@ -865,6 +865,25 @@ winrt::Microsoft::Terminal::Settings::Model::ColorScheme CascadiaSettings::GetCo
return _globals->ColorSchemes().TryLookup(schemeName);
}
// Method Description:
// - updates all references to that color scheme with the new name
// Arguments:
// - oldName: the original name for the color scheme
// - newName: the new name for the color scheme
// Return Value:
// - <none>
void CascadiaSettings::UpdateColorSchemeReferences(const hstring oldName, const hstring newName)
{
// update all profiles referencing this color scheme
for (const auto& profile : _allProfiles)
{
if (profile.ColorSchemeName() == oldName)
{
profile.ColorSchemeName(newName);
}
}
}
winrt::hstring CascadiaSettings::ApplicationDisplayName()
{
try

View file

@ -88,6 +88,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Model::Profile CreateNewProfile();
Model::Profile FindProfile(guid profileGuid) const noexcept;
Model::ColorScheme GetColorSchemeForProfile(const guid profileGuid) const;
void UpdateColorSchemeReferences(const hstring oldName, const hstring newName);
Windows::Foundation::Collections::IVectorView<SettingsLoadWarnings> Warnings();
void ClearWarnings();

View file

@ -39,6 +39,7 @@ namespace Microsoft.Terminal.Settings.Model
Profile CreateNewProfile();
Profile FindProfile(Guid profileGuid);
ColorScheme GetColorSchemeForProfile(Guid profileGuid);
void UpdateColorSchemeReferences(String oldName, String newName);
Guid GetProfileForArgs(NewTerminalArgs newTerminalArgs);
}