Deduplicate identical inbox color schemes to heal user settings (#12800)

Up until now, we have treated inbox, fragment and user color schemes the same: we load them all into one big map and when we save the settings file we write them *all* out. It's been a big annoyance pretty much forever. In addition to cluttering the user's settings file, it prevents us from making changes to the stock color schemes (like to change the cursor color, or to adjust the colors in Tango Dark, or what have you) because they're already copied in full in the user settings. It also means that we need some special UI affordances for color schemes that you are allowed to view but not to delete or rename. We also have a funny hardcoded list of color scheme names and we use that to determine whether they're "inbox" for UI purposes. Because of all that, we are hesitant to add *more* color schemes to the default set. This pull request resolves all of those issues at once. It: - Adds an "origin" to color schemes indicating where they're from (Inbox, Fragment, User, ...) - Replaces the Edit UI with a much simpler version that pretty much only has a "duplicate this color scheme to start editing it" button - Deletes color schemes that we consider to be equivalent to inbox ones; this allows us to finally disentangle the user's preferences from the terminal's. - Migrates all user settings that referred to schemes they may have modified (even implicitly!) to their modified versions. The equivalence check intentionally leaves out the cursor and selection colors, so that we have the freedom to change them in the future. The Origin is part of a new interface, `ISettingsModelObject`, which we can use in the future for things like Themes and Actions.
microsoft · Feb 27, 2024 · de0f702 · de0f702
1 parent abf5d94
commit de0f702
Show file tree

Hide file tree

Showing 23 changed files with 1,235 additions and 97 deletions.
diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt
@@ -90,6 +90,8 @@ ptys
 pwshw
 qof
 qps
+Remappings
+Retargets
 rclt
 reimplementation
 reserialization

diff --git a/src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp b/src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp
diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
@@ -45,6 +45,10 @@ namespace SettingsModelLocalTests
  TEST_METHOD(RoundtripReloadEnvVars);
  TEST_METHOD(DontRoundtripNoReloadEnvVars);
 
+ TEST_METHOD(RoundtripUserModifiedColorSchemeCollision);
+ TEST_METHOD(RoundtripUserModifiedColorSchemeCollisionUnusedByProfiles);
+ TEST_METHOD(RoundtripUserDeletedColorSchemeCollision);
+
  private:
  // Method Description:
  // - deserializes and reserializes a json string representing a settings object model of type T
@@ -630,4 +634,306 @@ namespace SettingsModelLocalTests
  VERIFY_IS_FALSE(newSettings->ProfileDefaults().HasReloadEnvironmentVariables(),
  L"Ensure that the new settings object didn't find a reloadEnvironmentVariables");
  }
+
+ void SerializationTests::RoundtripUserModifiedColorSchemeCollision()
+ {
+ static constexpr std::string_view oldSettingsJson{ R"(
+ {
+ "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+ "profiles": [
+ {
+ "name": "profile0",
+ "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}"
+ },
+ {
+ "name": "profile1",
+ "colorScheme": "Tango Dark",
+ "guid": "{d0a65a9d-8665-4128-97a4-a581aa747aa7}"
+ }
+ ],
+ "schemes": [
+ {
+ "background": "#121314",
+ "black": "#121314",
+ "blue": "#121314",
+ "brightBlack": "#121314",
+ "brightBlue": "#121314",
+ "brightCyan": "#121314",
+ "brightGreen": "#121314",
+ "brightPurple": "#121314",
+ "brightRed": "#121314",
+ "brightWhite": "#121314",
+ "brightYellow": "#121314",
+ "cursorColor": "#121314",
+ "cyan": "#121314",
+ "foreground": "#121314",
+ "green": "#121314",
+ "name": "Campbell",
+ "purple": "#121314",
+ "red": "#121314",
+ "selectionBackground": "#121314",
+ "white": "#121314",
+ "yellow": "#121314"
+ },
+ {
+ "background": "#000000",
+ "black": "#000000",
+ "blue": "#3465A4",
+ "brightBlack": "#555753",
+ "brightBlue": "#729FCF",
+ "brightCyan": "#34E2E2",
+ "brightGreen": "#8AE234",
+ "brightPurple": "#AD7FA8",
+ "brightRed": "#EF2929",
+ "brightWhite": "#EEEEEC",
+ "brightYellow": "#FCE94F",
+ "cursorColor": "#FFFFFF",
+ "cyan": "#06989A",
+ "foreground": "#D3D7CF",
+ "green": "#4E9A06",
+ "name": "Tango Dark",
+ "purple": "#75507B",
+ "red": "#CC0000",
+ "selectionBackground": "#FFFFFF",
+ "white": "#D3D7CF",
+ "yellow": "#C4A000"
+ },
+ ]
+ })" };
+
+ // Key differences: one fewer color scheme (Tango Dark has been deleted) and defaults.colorScheme is set.
+ static constexpr std::string_view newSettingsJson{ R"-(
+ {
+ "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+ "profiles":
+ {
+ "defaults": {
+ "colorScheme": "Campbell (modified)"
+ },
+ "list":
+ [
+ {
+ "name": "profile0",
+ "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}"
+ },
+ {
+ "name": "profile1",
+ "colorScheme": "Tango Dark",
+ "guid": "{d0a65a9d-8665-4128-97a4-a581aa747aa7}"
+ }
+ ]
+ },
+ "actions": [ ],
+ "schemes": [
+ {
+ "background": "#121314",
+ "black": "#121314",
+ "blue": "#121314",
+ "brightBlack": "#121314",
+ "brightBlue": "#121314",
+ "brightCyan": "#121314",
+ "brightGreen": "#121314",
+ "brightPurple": "#121314",
+ "brightRed": "#121314",
+ "brightWhite": "#121314",
+ "brightYellow": "#121314",
+ "cursorColor": "#121314",
+ "cyan": "#121314",
+ "foreground": "#121314",
+ "green": "#121314",
+ "name": "Campbell",
+ "purple": "#121314",
+ "red": "#121314",
+ "selectionBackground": "#121314",
+ "white": "#121314",
+ "yellow": "#121314"
+ }
+ ]
+ })-" };
+
+ implementation::SettingsLoader oldLoader{ oldSettingsJson, DefaultJson };
+ oldLoader.MergeInboxIntoUserSettings();
+ oldLoader.FinalizeLayering();
+ VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+ const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
+ const auto oldResult{ oldSettings->ToJson() };
+
+ implementation::SettingsLoader newLoader{ newSettingsJson, DefaultJson };
+ newLoader.MergeInboxIntoUserSettings();
+ newLoader.FinalizeLayering();
+ newLoader.FixupUserSettings();
+ const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
+ const auto newResult{ newSettings->ToJson() };
+
+ VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult));
+ }
+
+ void SerializationTests::RoundtripUserModifiedColorSchemeCollisionUnusedByProfiles()
+ {
+ static constexpr std::string_view oldSettingsJson{ R"(
+ {
+ "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+ "profiles": [
+ {
+ "name": "profile0",
+ "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}"
+ }
+ ],
+ "schemes": [
+ {
+ "background": "#111111",
+ "black": "#111111",
+ "blue": "#111111",
+ "brightBlack": "#111111",
+ "brightBlue": "#111111",
+ "brightCyan": "#111111",
+ "brightGreen": "#111111",
+ "brightPurple": "#111111",
+ "brightRed": "#111111",
+ "brightWhite": "#111111",
+ "brightYellow": "#111111",
+ "cursorColor": "#111111",
+ "cyan": "#111111",
+ "foreground": "#111111",
+ "green": "#111111",
+ "name": "Tango Dark",
+ "purple": "#111111",
+ "red": "#111111",
+ "selectionBackground": "#111111",
+ "white": "#111111",
+ "yellow": "#111111"
+ },
+ ]
+ })" };
+
+ // Key differences: Tango Dark has been renamed; nothing else has changed
+ static constexpr std::string_view newSettingsJson{ R"-(
+ {
+ "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+ "profiles":
+ {
+ "list":
+ [
+ {
+ "name": "profile0",
+ "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}"
+ }
+ ]
+ },
+ "actions": [ ],
+ "schemes": [
+ {
+ "background": "#111111",
+ "black": "#111111",
+ "blue": "#111111",
+ "brightBlack": "#111111",
+ "brightBlue": "#111111",
+ "brightCyan": "#111111",
+ "brightGreen": "#111111",
+ "brightPurple": "#111111",
+ "brightRed": "#111111",
+ "brightWhite": "#111111",
+ "brightYellow": "#111111",
+ "cursorColor": "#111111",
+ "cyan": "#111111",
+ "foreground": "#111111",
+ "green": "#111111",
+ "name": "Tango Dark (modified)",
+ "purple": "#111111",
+ "red": "#111111",
+ "selectionBackground": "#111111",
+ "white": "#111111",
+ "yellow": "#111111"
+ },
+ ]
+ })-" };
+
+ implementation::SettingsLoader oldLoader{ oldSettingsJson, DefaultJson };
+ oldLoader.MergeInboxIntoUserSettings();
+ oldLoader.FinalizeLayering();
+ VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+ const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
+ const auto oldResult{ oldSettings->ToJson() };
+
+ implementation::SettingsLoader newLoader{ newSettingsJson, DefaultJson };
+ newLoader.MergeInboxIntoUserSettings();
+ newLoader.FinalizeLayering();
+ newLoader.FixupUserSettings();
+ const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
+ const auto newResult{ newSettings->ToJson() };
+
+ VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult));
+ }
+
+ void SerializationTests::RoundtripUserDeletedColorSchemeCollision()
+ {
+ static constexpr std::string_view oldSettingsJson{ R"(
+ {
+ "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+ "profiles": [
+ {
+ "name": "profile0",
+ "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}"
+ }
+ ],
+ "schemes": [
+ {
+ "name": "Tango Dark",
+ "foreground": "#D3D7CF",
+ "background": "#000000",
+ "cursorColor": "#FFFFFF",
+ "black": "#000000",
+ "red": "#CC0000",
+ "green": "#4E9A06",
+ "yellow": "#C4A000",
+ "blue": "#3465A4",
+ "purple": "#75507B",
+ "cyan": "#06989A",
+ "white": "#D3D7CF",
+ "brightBlack": "#555753",
+ "brightRed": "#EF2929",
+ "brightGreen": "#8AE234",
+ "brightYellow": "#FCE94F",
+ "brightBlue": "#729FCF",
+ "brightPurple": "#AD7FA8",
+ "brightCyan": "#34E2E2",
+ "brightWhite": "#EEEEEC"
+ }
+ ]
+ })" };
+
+ // Key differences: Tango Dark has been deleted, as it was identical to the inbox one.
+ static constexpr std::string_view newSettingsJson{ R"-(
+ {
+ "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+ "profiles":
+ {
+ "list":
+ [
+ {
+ "name": "profile0",
+ "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}"
+ }
+ ]
+ },
+ "actions": [ ],
+ "schemes": [ ]
+ })-" };
+
+ implementation::SettingsLoader oldLoader{ oldSettingsJson, DefaultJson };
+ oldLoader.MergeInboxIntoUserSettings();
+ oldLoader.FinalizeLayering();
+ VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+ const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
+ const auto oldResult{ oldSettings->ToJson() };
+
+ implementation::SettingsLoader newLoader{ newSettingsJson, DefaultJson };
+ newLoader.MergeInboxIntoUserSettings();
+ newLoader.FinalizeLayering();
+ newLoader.FixupUserSettings();
+ const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
+ const auto newResult{ newSettings->ToJson() };
+
+ VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult));
+ }
 }
diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.cpp b/src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.cpp
@@ -74,6 +74,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
  _NotifyChanges(L"IsDefaultScheme");
  }
 
+ bool ColorSchemeViewModel::IsEditable() const
+ {
+ return _scheme.Origin() == Model::OriginTag::User;
+ }
+
  bool ColorSchemeViewModel::RequestRename(winrt::hstring newName)
  {
  if (const auto parentPageVM{ _parentPageVM.get() })
@@ -126,6 +131,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
  }
  }
 
+ void ColorSchemeViewModel::Duplicate_Click(const IInspectable& /*sender*/, const Windows::UI::Xaml::RoutedEventArgs& /*e*/)
+ {
+ if (const auto parentPageVM{ _parentPageVM.get() })
+ {
+ return parentPageVM.RequestDuplicateCurrentScheme();
+ }
+ }
+
  void ColorSchemeViewModel::DeleteConfirmation_Click(const IInspectable& /*sender*/, const Windows::UI::Xaml::RoutedEventArgs& /*e*/)
  {
  if (const auto parentPageVM{ _parentPageVM.get() })

diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.h b/src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.h
@@ -32,14 +32,15 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
  Editor::ColorTableEntry ColorEntryAt(uint32_t index);
  bool IsDefaultScheme();
  void RefreshIsDefault();
+ bool IsEditable() const;
 
  void DeleteConfirmation_Click(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::RoutedEventArgs& e);
  void SetAsDefault_Click(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::RoutedEventArgs& e);
+ void Duplicate_Click(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::RoutedEventArgs& e);
 
  // DON'T YOU DARE ADD A `WINRT_CALLBACK(PropertyChanged` TO A CLASS DERIVED FROM ViewModelHelper. Do this instead:
  using ViewModelHelper<ColorSchemeViewModel>::PropertyChanged;
 
- WINRT_PROPERTY(bool, IsInBoxScheme);
  WINRT_PROPERTY(Windows::Foundation::Collections::IVector<Editor::ColorTableEntry>, NonBrightColorTable, nullptr);
  WINRT_PROPERTY(Windows::Foundation::Collections::IVector<Editor::ColorTableEntry>, BrightColorTable, nullptr);
 

diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.idl b/src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.idl
@@ -8,7 +8,7 @@ namespace Microsoft.Terminal.Settings.Editor
  runtimeclass ColorSchemeViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged, Windows.Foundation.IStringable
  {
  String Name;
- Boolean IsInBoxScheme;
+ Boolean IsEditable { get; };
  Boolean IsDefaultScheme { get; };
 
  Boolean RequestRename(String newName);
@@ -27,6 +27,7 @@ namespace Microsoft.Terminal.Settings.Editor
 
  void DeleteConfirmation_Click(IInspectable sender, Windows.UI.Xaml.RoutedEventArgs args);
  void SetAsDefault_Click(IInspectable sender, Windows.UI.Xaml.RoutedEventArgs args);
+ void Duplicate_Click(IInspectable sender, Windows.UI.Xaml.RoutedEventArgs args);
  }
 
  [default_interface] runtimeclass ColorTableEntry : Windows.UI.Xaml.Data.INotifyPropertyChanged