Skip to content

Commit

Permalink
Bugfix: sync color scheme rename with profile reference (microsoft#8793)
Browse files Browse the repository at this point in the history
## 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
microsoft#6800 - Settings UI Epic

## PR Checklist
* [X] Closes microsoft#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](microsoft#8756 (comment)) for a discussion on this topic.

## Validation Steps Performed
Repro steps from microsoft#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)
  • Loading branch information
carlos-zamora authored and mpela81 committed Jan 28, 2021
1 parent 81c6b0c commit 9a53096
Show file tree
Hide file tree
Showing 9 changed files with 74 additions and 10 deletions.
35 changes: 31 additions & 4 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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)
Expand All @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -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);

Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"");
};

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/ColorSchemes.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down Expand Up @@ -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">
Expand All @@ -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">
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
19 changes: 19 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit 9a53096

Please sign in to comment.