From 0a4fc243f3dacbee62e0d7d58cb249703760cebe Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Wed, 2 Oct 2019 16:25:19 -0500
Subject: [PATCH 1/4] fixes #2547

---
 .../LocalTests_TerminalApp/SettingsTests.cpp  | 61 +++++++++++++++++++
 src/cascadia/TerminalApp/App.cpp              |  5 +-
 src/cascadia/TerminalApp/CascadiaSettings.cpp | 30 ++++++++-
 src/cascadia/TerminalApp/CascadiaSettings.h   |  1 +
 src/cascadia/TerminalApp/Profile.cpp          | 28 +++++++++
 src/cascadia/TerminalApp/Profile.h            |  2 +
 .../Resources/en-US/Resources.resw            | 60 +++++++++---------
 src/cascadia/TerminalApp/TerminalWarnings.h   |  3 +-
 src/types/inc/utils.hpp                       | 32 +++++++++-
 src/types/utils.cpp                           | 31 ----------
 10 files changed, 189 insertions(+), 64 deletions(-)

diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
index e3915d3dbc8..1fe7ea2463f 100644
--- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
+++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
@@ -48,6 +48,7 @@ namespace TerminalAppLocalTests
         TEST_METHOD(TestLayeringNameOnlyProfiles);
         TEST_METHOD(TestExplodingNameOnlyProfiles);
         TEST_METHOD(TestHideAllProfiles);
+        TEST_METHOD(TestInvalidColorSchemeName);
 
         TEST_CLASS_SETUP(ClassSetup)
         {
@@ -1191,4 +1192,64 @@ namespace TerminalAppLocalTests
             VERIFY_IS_TRUE(caughtExpectedException);
         }
     }
+
+    void SettingsTests::TestInvalidColorSchemeName()
+    {
+        Log::Comment(NoThrowString().Format(
+            L"Ensure that setting a profile's scheme to a non-existent scheme causes a warning."));
+
+        const std::string settings0String{ R"(
+        {
+            "profiles": [
+                {
+                    "name" : "profile0",
+                    "colorScheme": "schemeOne"
+                },
+                {
+                    "name" : "profile1",
+                    "colorScheme": "InvalidSchemeName"
+                },
+                {
+                    "name" : "profile2"
+                    // Will use the Profile default value, "Campbell"
+                }
+            ],
+            "schemes": [
+                {
+                    "name": "schemeOne",
+                    "foreground": "#111111"
+                },
+                {
+                    "name": "schemeTwo",
+                    "foreground": "#222222"
+                }
+            ]
+        })" };
+
+        VerifyParseSucceeded(settings0String);
+
+        CascadiaSettings settings;
+        settings._ParseJsonString(settings0String, false);
+        settings.LayerJson(settings._userSettings);
+
+        VERIFY_ARE_EQUAL(3u, settings._profiles.size());
+        VERIFY_ARE_EQUAL(2u, settings._globals._colorSchemes.size());
+
+        settings._ValidateAllSchemesExist();
+
+        VERIFY_ARE_EQUAL(1u, settings._warnings.size());
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme, settings._warnings.at(0));
+
+        VERIFY_ARE_EQUAL(3u, settings._profiles.size());
+        VERIFY_ARE_EQUAL(2u, settings._globals._colorSchemes.size());
+
+        VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), settings._profiles.at(0)._colorTable[0]);
+        VERIFY_ARE_EQUAL(ARGB(0, 12, 12, 12), settings._profiles.at(1)._colorTable[0]);
+        // In this set of schemes, Campbell doesn't exist, so the table will get written
+        VERIFY_ARE_EQUAL(ARGB(0, 12, 12, 12), settings._profiles.at(2)._colorTable[0]);
+
+        VERIFY_ARE_EQUAL(L"schemeOne", settings._profiles.at(0)._schemeName.value());
+        VERIFY_ARE_EQUAL(L"InvalidSchemeName", settings._profiles.at(1)._schemeName.value());
+        VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(2)._schemeName.value());
+    }
 }
diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp
index 4a61d60fbe6..5b9070fe48f 100644
--- a/src/cascadia/TerminalApp/App.cpp
+++ b/src/cascadia/TerminalApp/App.cpp
@@ -26,9 +26,10 @@ namespace winrt
 // !!! IMPORTANT !!!
 // Make sure that these keys are in the same order as the
 // SettingsLoadWarnings/Errors enum is!
-static const std::array<std::wstring_view, 2> settingsLoadWarningsLabels {
+static const std::array<std::wstring_view, 3> settingsLoadWarningsLabels {
    L"MissingDefaultProfileText",
-   L"DuplicateProfileText"
+   L"DuplicateProfileText",
+   L"UnknownColorSchemeText"
 };
 static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
     L"NoProfilesText",
diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp
index 5b5fe32518e..dd23aa0ba75 100644
--- a/src/cascadia/TerminalApp/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp
@@ -174,9 +174,10 @@ void CascadiaSettings::_ValidateSettings()
     _ValidateNoDuplicateProfiles();
     _ValidateDefaultProfileExists();
 
-    // TODO:GH#2547 ensure that all the profile's color scheme names are
+    // Ensure that all the profile's color scheme names are
     // actually the names of schemes we've parsed. If the scheme doesn't exist,
     // just use the hardcoded defaults
+    _ValidateAllSchemesExist();
 
     // TODO:GH#2548 ensure there's at least one key bound. Display a warning if
     // there's _NO_ keys bound to any actions. That's highly irregular, and
@@ -366,3 +367,30 @@ void CascadiaSettings::_RemoveHiddenProfiles()
         throw ::TerminalApp::SettingsException(::TerminalApp::SettingsLoadErrors::AllProfilesHidden);
     }
 }
+
+// Method Description:
+// - Ensures that every profile has a valid "color scheme" set. If any profile
+//   has a colorScheme set to a value which is _not_ the name of an actual color
+//   scheme, we'll set the color table of the profile to something reasonable.
+// Arguments:
+// - <none>
+// Return Value:
+// - <none>
+// - Appends a SettingsLoadWarnings::UnknownColorScheme to our list of warnings if
+//   we find any such duplicate.
+void CascadiaSettings::_ValidateAllSchemesExist()
+{
+    bool foundInvalidScheme = false;
+    for (auto& profile : _profiles)
+    {
+        if (!profile.ValidateColorScheme(_globals.GetColorSchemes()))
+        {
+            foundInvalidScheme = true;
+        }
+    }
+
+    if (foundInvalidScheme)
+    {
+        _warnings.push_back(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme);
+    }
+}
diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h
index aca875f3982..b41c2dbd689 100644
--- a/src/cascadia/TerminalApp/CascadiaSettings.h
+++ b/src/cascadia/TerminalApp/CascadiaSettings.h
@@ -104,6 +104,7 @@ class TerminalApp::CascadiaSettings final
     void _ValidateNoDuplicateProfiles();
     void _ReorderProfilesToMatchUserSettingsOrder();
     void _RemoveHiddenProfiles();
+    void _ValidateAllSchemesExist();
 
     friend class TerminalAppLocalTests::SettingsTests;
     friend class TerminalAppLocalTests::ProfileTests;
diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp
index d293abf5d46..c80a6dfe210 100644
--- a/src/cascadia/TerminalApp/Profile.cpp
+++ b/src/cascadia/TerminalApp/Profile.cpp
@@ -1188,3 +1188,31 @@ GUID Profile::GetGuidOrGenerateForJson(const Json::Value& json) noexcept
 
     return Profile::_GenerateGuidForProfile(name, source);
 }
+
+// Method Description:
+// - Validates that the `colorScheme` set for this Profile exists. If it does
+//   exist, returns true. Otherwise, we'll initialize our color table to the
+//   default color scheme, the Campbell scheme, and returns false.
+// - This is done
+//   for microsoft/terminal#2547. In the case where the scheme was invalid, we'd
+//   leave the table un-initialized, resulting in all the colors being RGB(0, 0,
+//   0,) _black_.
+// Arguments:
+// - schemes: a list of color schemes to check and ensure that our colorScheme exitsts in.
+// Return Value:
+// - true if the color scheme existed, false otherwise.
+bool Profile::ValidateColorScheme(const std::vector<::TerminalApp::ColorScheme>& schemes) noexcept
+{
+    const ColorScheme* const matchingScheme = _FindScheme(schemes, _schemeName.value());
+    // If we couldn't find a match, initialize the table to the campbell colors
+    if (matchingScheme == nullptr)
+    {
+        gsl::span<uint32_t> tableView{ &_colorTable[0], COLOR_TABLE_SIZE };
+        // Then use fill the first 16 values with the Campbell scheme
+        Utils::InitializeCampbellColorTable(tableView);
+
+        return false;
+    }
+
+    return true;
+}
diff --git a/src/cascadia/TerminalApp/Profile.h b/src/cascadia/TerminalApp/Profile.h
index eabba4f3aeb..63e413bf769 100644
--- a/src/cascadia/TerminalApp/Profile.h
+++ b/src/cascadia/TerminalApp/Profile.h
@@ -63,6 +63,8 @@ class TerminalApp::Profile final
     bool HasConnectionType() const noexcept;
     GUID GetConnectionType() const noexcept;
 
+    bool ValidateColorScheme(const std::vector<::TerminalApp::ColorScheme>& schemes) noexcept;
+
     void SetFontFace(std::wstring fontFace) noexcept;
     void SetColorScheme(std::optional<std::wstring> schemeName) noexcept;
     void SetTabTitle(std::wstring tabTitle) noexcept;
diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw
index cf078dcec5e..1162be2ba17 100644
--- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw
+++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw
@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!-- 
-    Microsoft ResX Schema 
-    
+  <!--
+    Microsoft ResX Schema
+
     Version 2.0
-    
-    The primary goals of this format is to allow a simple XML format 
-    that is mostly human readable. The generation and parsing of the 
-    various data types are done through the TypeConverter classes 
+
+    The primary goals of this format is to allow a simple XML format
+    that is mostly human readable. The generation and parsing of the
+    various data types are done through the TypeConverter classes
     associated with the data types.
-    
+
     Example:
-    
+
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-                
-    There are any number of "resheader" rows that contain simple 
+
+    There are any number of "resheader" rows that contain simple
     name/value pairs.
-    
-    Each data row contains a name, and value. The row also contains a 
-    type or mimetype. Type corresponds to a .NET class that support 
-    text/value conversion through the TypeConverter architecture. 
-    Classes that don't support this are serialized and stored with the 
+
+    Each data row contains a name, and value. The row also contains a
+    type or mimetype. Type corresponds to a .NET class that support
+    text/value conversion through the TypeConverter architecture.
+    Classes that don't support this are serialized and stored with the
     mimetype set.
-    
-    The mimetype is used for serialized objects, and tells the 
-    ResXResourceReader how to depersist the object. This is currently not 
+
+    The mimetype is used for serialized objects, and tells the
+    ResXResourceReader how to depersist the object. This is currently not
     extensible. For a given mimetype the value must be set accordingly:
-    
-    Note - application/x-microsoft.net.object.binary.base64 is the format 
-    that the ResXResourceWriter will generate, however the reader can 
+
+    Note - application/x-microsoft.net.object.binary.base64 is the format
+    that the ResXResourceWriter will generate, however the reader can
     read any of the formats listed below.
-    
+
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-    
+
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array 
+    value   : The object must be serialized into a byte array
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -127,6 +127,10 @@
   </data>
   <data name="DuplicateProfileText" xml:space="preserve">
     <value>Found multiple profiles with the same GUID in your settings file - ignoring duplicates. Make sure each profile's GUID is unique.
+</value>
+  </data>
+  <data name="UnknownColorSchemeText" xml:space="preserve">
+    <value>Found a profile with an invalid "colorScheme". Defaulting that profile to the default colors. Make sure that when setting a "colorScheme", the value matches the "name" of a color scheme in the "schemes" list.
 </value>
   </data>
   <data name="NoProfilesText" xml:space="preserve">
@@ -206,4 +210,4 @@ Temporarily using the Windows Terminal default settings.
   <data name="CloseWindowWarningTitle" xml:space="preserve">
     <value>Do you want to close all tabs?</value>
   </data>
-</root>
\ No newline at end of file
+</root>
diff --git a/src/cascadia/TerminalApp/TerminalWarnings.h b/src/cascadia/TerminalApp/TerminalWarnings.h
index b298b22827c..19d6fdcd665 100644
--- a/src/cascadia/TerminalApp/TerminalWarnings.h
+++ b/src/cascadia/TerminalApp/TerminalWarnings.h
@@ -22,7 +22,8 @@ namespace TerminalApp
     enum class SettingsLoadWarnings : uint32_t
     {
         MissingDefaultProfile = 0,
-        DuplicateProfile = 1
+        DuplicateProfile = 1,
+        UnknownColorScheme = 2
     };
 
     // SettingsLoadWarnings are scenarios where the settings had invalid state
diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp
index ff1c8dd1781..0102f8c822c 100644
--- a/src/types/inc/utils.hpp
+++ b/src/types/inc/utils.hpp
@@ -36,7 +36,37 @@ namespace Microsoft::Console::Utils
     std::string ColorToHexString(const COLORREF color);
     COLORREF ColorFromHexString(const std::string wstr);
 
-    void InitializeCampbellColorTable(const gsl::span<COLORREF> table);
+    // Function Description:
+    // - Fill the first 16 entries of a given color table with the Campbell color
+    //   scheme, in the ANSI/VT RGB order.
+    // Arguments:
+    // - table: a color table with at least 16 entries
+    // Return Value:
+    // - <none>, throws if the table has less that 16 entries
+    template<typename T>
+    void InitializeCampbellColorTable(const gsl::span<T> table)
+    {
+        THROW_HR_IF(E_INVALIDARG, table.size() < 16);
+
+        // clang-format off
+        table[0]   = RGB( 12,   12,   12);
+        table[1]   = RGB( 197,  15,   31);
+        table[2]   = RGB( 19,   161,  14);
+        table[3]   = RGB( 193,  156,  0);
+        table[4]   = RGB( 0,    55,   218);
+        table[5]   = RGB( 136,  23,   152);
+        table[6]   = RGB( 58,   150,  221);
+        table[7]   = RGB( 204,  204,  204);
+        table[8]   = RGB( 118,  118,  118);
+        table[9]   = RGB( 231,  72,   86);
+        table[10]  = RGB( 22,   198,  12);
+        table[11]  = RGB( 249,  241,  165);
+        table[12]  = RGB( 59,   120,  255);
+        table[13]  = RGB( 180,  0,    158);
+        table[14]  = RGB( 97,   214,  214);
+        table[15]  = RGB( 242,  242,  242);
+        // clang-format on
+    };
     void InitializeCampbellColorTableForConhost(const gsl::span<COLORREF> table);
     void SwapANSIColorOrderForConhost(const gsl::span<COLORREF> table);
     void Initialize256ColorTable(const gsl::span<COLORREF> table);
diff --git a/src/types/utils.cpp b/src/types/utils.cpp
index 5916d27ef9d..173914b4167 100644
--- a/src/types/utils.cpp
+++ b/src/types/utils.cpp
@@ -117,37 +117,6 @@ bool Utils::IsValidHandle(const HANDLE handle) noexcept
     return handle != nullptr && handle != INVALID_HANDLE_VALUE;
 }
 
-// Function Description:
-// - Fill the first 16 entries of a given color table with the Campbell color
-//   scheme, in the ANSI/VT RGB order.
-// Arguments:
-// - table: a color table with at least 16 entries
-// Return Value:
-// - <none>, throws if the table has less that 16 entries
-void Utils::InitializeCampbellColorTable(const gsl::span<COLORREF> table)
-{
-    THROW_HR_IF(E_INVALIDARG, table.size() < 16);
-
-    // clang-format off
-    table[0]   = RGB( 12,   12,   12);
-    table[1]   = RGB( 197,  15,   31);
-    table[2]   = RGB( 19,   161,  14);
-    table[3]   = RGB( 193,  156,  0);
-    table[4]   = RGB( 0,    55,   218);
-    table[5]   = RGB( 136,  23,   152);
-    table[6]   = RGB( 58,   150,  221);
-    table[7]   = RGB( 204,  204,  204);
-    table[8]   = RGB( 118,  118,  118);
-    table[9]   = RGB( 231,  72,   86);
-    table[10]  = RGB( 22,   198,  12);
-    table[11]  = RGB( 249,  241,  165);
-    table[12]  = RGB( 59,   120,  255);
-    table[13]  = RGB( 180,  0,    158);
-    table[14]  = RGB( 97,   214,  214);
-    table[15]  = RGB( 242,  242,  242);
-    // clang-format on
-}
-
 // Function Description:
 // - Fill the first 16 entries of a given color table with the Campbell color
 //   scheme, in the Windows BGR order.

From 3206644cca5610637be068c4c6862be45b630cb2 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Thu, 3 Oct 2019 17:07:08 -0500
Subject: [PATCH 2/4] Well this was an enormous pain

---
 .../ColorSchemeTests.cpp                      | 112 ++++++++++++------
 src/cascadia/TerminalApp/CascadiaSettings.cpp |  10 +-
 src/cascadia/TerminalApp/CascadiaSettings.h   |   2 -
 .../CascadiaSettingsSerialization.cpp         |  57 +--------
 .../TerminalApp/GlobalAppSettings.cpp         |  10 +-
 src/cascadia/TerminalApp/GlobalAppSettings.h  |  10 +-
 src/cascadia/TerminalApp/Profile.cpp          |  62 ++--------
 src/cascadia/TerminalApp/Profile.h            |   5 +-
 src/cascadia/ut_app/JsonTests.cpp             |   1 +
 9 files changed, 113 insertions(+), 156 deletions(-)

diff --git a/src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp b/src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp
index 08e27bcfc3a..eaf1b740e61 100644
--- a/src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp
+++ b/src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp
@@ -169,7 +169,7 @@ namespace TerminalAppLocalTests
             "background": "#050505"
         })" };
         const std::string scheme3String{ R"({
-            // "name": "scheme3",
+            // by not providing a name, the scheme will have the name ""
             "foreground": "#060606",
             "background": "#070707"
         })" };
@@ -188,47 +188,85 @@ namespace TerminalAppLocalTests
         VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
 
         settings._LayerOrCreateColorScheme(scheme0Json);
-        VERIFY_ARE_EQUAL(1u, settings._globals.GetColorSchemes().size());
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
-        VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme1Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
-        VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
-        VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), settings._globals.GetColorSchemes().at(0)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), settings._globals.GetColorSchemes().at(0)._defaultBackground);
+        {
+            for (auto& kv : settings._globals._colorSchemes)
+            {
+                Log::Comment(NoThrowString().Format(
+                    L"kv:%s->%s", kv.first.data(), kv.second.GetName().data()));
+            }
+            VERIFY_ARE_EQUAL(1u, settings._globals.GetColorSchemes().size());
+
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
+            auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
+
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
+            VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme1Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
+            VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
+            VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), scheme0._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), scheme0._defaultBackground);
+        }
 
         settings._LayerOrCreateColorScheme(scheme1Json);
-        VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
-        VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
-        VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), settings._globals.GetColorSchemes().at(0)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), settings._globals.GetColorSchemes().at(0)._defaultBackground);
-        VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), settings._globals.GetColorSchemes().at(1)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), settings._globals.GetColorSchemes().at(1)._defaultBackground);
 
+        {
+            VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());
+
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
+            auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme1") != settings._globals._colorSchemes.end());
+            auto scheme1 = settings._globals._colorSchemes.find(L"scheme1")->second;
+
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
+            VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
+            VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), scheme0._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), scheme0._defaultBackground);
+            VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), scheme1._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), scheme1._defaultBackground);
+        }
         settings._LayerOrCreateColorScheme(scheme2Json);
-        VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
-        VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
-        VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), settings._globals.GetColorSchemes().at(0)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), settings._globals.GetColorSchemes().at(0)._defaultBackground);
-        VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), settings._globals.GetColorSchemes().at(1)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), settings._globals.GetColorSchemes().at(1)._defaultBackground);
 
+        {
+            VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());
+
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
+            auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme1") != settings._globals._colorSchemes.end());
+            auto scheme1 = settings._globals._colorSchemes.find(L"scheme1")->second;
+
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
+            VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
+            VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), scheme0._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), scheme0._defaultBackground);
+            VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), scheme1._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), scheme1._defaultBackground);
+        }
         settings._LayerOrCreateColorScheme(scheme3Json);
-        VERIFY_ARE_EQUAL(3u, settings._globals.GetColorSchemes().size());
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
-        VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
-        VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
-        VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), settings._globals.GetColorSchemes().at(0)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), settings._globals.GetColorSchemes().at(0)._defaultBackground);
-        VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), settings._globals.GetColorSchemes().at(1)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), settings._globals.GetColorSchemes().at(1)._defaultBackground);
-        VERIFY_ARE_EQUAL(ARGB(0, 6, 6, 6), settings._globals.GetColorSchemes().at(2)._defaultForeground);
-        VERIFY_ARE_EQUAL(ARGB(0, 7, 7, 7), settings._globals.GetColorSchemes().at(2)._defaultBackground);
+
+        {
+            VERIFY_ARE_EQUAL(3u, settings._globals.GetColorSchemes().size());
+
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
+            auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme1") != settings._globals._colorSchemes.end());
+            auto scheme1 = settings._globals._colorSchemes.find(L"scheme1")->second;
+            VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"") != settings._globals._colorSchemes.end());
+            auto scheme2 = settings._globals._colorSchemes.find(L"")->second;
+
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
+            VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
+            VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
+            VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), scheme0._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), scheme0._defaultBackground);
+            VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), scheme1._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), scheme1._defaultBackground);
+            VERIFY_ARE_EQUAL(ARGB(0, 6, 6, 6), scheme2._defaultForeground);
+            VERIFY_ARE_EQUAL(ARGB(0, 7, 7, 7), scheme2._defaultBackground);
+        }
     }
 }
diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp
index dd23aa0ba75..6beb8c5fa10 100644
--- a/src/cascadia/TerminalApp/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp
@@ -383,9 +383,15 @@ void CascadiaSettings::_ValidateAllSchemesExist()
     bool foundInvalidScheme = false;
     for (auto& profile : _profiles)
     {
-        if (!profile.ValidateColorScheme(_globals.GetColorSchemes()))
+        auto schemeName = profile.GetSchemeName();
+        if (schemeName.has_value())
         {
-            foundInvalidScheme = true;
+            const auto found = _globals.GetColorSchemes().find(schemeName.value());
+            if (found == _globals.GetColorSchemes().end())
+            {
+                profile.SetColorScheme({ L"Campbell" });
+                foundInvalidScheme = true;
+            }
         }
     }
 
diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h
index b41c2dbd689..fde8177507c 100644
--- a/src/cascadia/TerminalApp/CascadiaSettings.h
+++ b/src/cascadia/TerminalApp/CascadiaSettings.h
@@ -48,7 +48,6 @@ class TerminalApp::CascadiaSettings final
 
     static std::unique_ptr<CascadiaSettings> LoadDefaults();
     static std::unique_ptr<CascadiaSettings> LoadAll();
-    void SaveAll() const;
 
     winrt::Microsoft::Terminal::Settings::TerminalSettings MakeSettings(std::optional<GUID> profileGuid) const;
 
@@ -58,7 +57,6 @@ class TerminalApp::CascadiaSettings final
 
     winrt::TerminalApp::AppKeyBindings GetKeybindings() const noexcept;
 
-    Json::Value ToJson() const;
     static std::unique_ptr<CascadiaSettings> FromJson(const Json::Value& json);
     void LayerJson(const Json::Value& json);
 
diff --git a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
index 858b491f94d..c56949b612b 100644
--- a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
@@ -240,26 +240,6 @@ void CascadiaSettings::_ParseJsonString(std::string_view fileData, const bool is
     }
 }
 
-// Method Description:
-// - Serialize this settings structure, and save it to a file. The location of
-//      the file changes depending whether we're running as a packaged
-//      application or not.
-// Arguments:
-// - <none>
-// Return Value:
-// - <none>
-void CascadiaSettings::SaveAll() const
-{
-    const auto json = ToJson();
-    Json::StreamWriterBuilder wbuilder;
-    // Use 4 spaces to indent instead of \t
-    wbuilder.settings_["indentation"] = "    ";
-    wbuilder.settings_["enableYAMLCompatibility"] = true; // suppress spaces around colons
-    const auto serializedString = Json::writeString(wbuilder, json);
-
-    _WriteSettings(serializedString);
-}
-
 // Method Description:
 // - Determines whether the user's settings file is missing a schema directive
 //   and, if so, inserts one.
@@ -402,36 +382,6 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings()
     return changedFile;
 }
 
-// Method Description:
-// - Serialize this object to a JsonObject.
-// Arguments:
-// - <none>
-// Return Value:
-// - a JsonObject which is an equivalent serialization of this object.
-Json::Value CascadiaSettings::ToJson() const
-{
-    Json::Value root;
-
-    Json::Value profilesArray;
-    for (const auto& profile : _profiles)
-    {
-        profilesArray.append(profile.ToJson());
-    }
-
-    Json::Value schemesArray;
-    const auto& colorSchemes = _globals.GetColorSchemes();
-    for (auto& scheme : colorSchemes)
-    {
-        schemesArray.append(scheme.ToJson());
-    }
-
-    root[GlobalsKey.data()] = _globals.ToJson();
-    root[ProfilesKey.data()] = profilesArray;
-    root[SchemesKey.data()] = schemesArray;
-
-    return root;
-}
-
 // Method Description:
 // - Create a new instance of this class from a serialized JsonObject.
 // Arguments:
@@ -570,8 +520,7 @@ void CascadiaSettings::_LayerOrCreateColorScheme(const Json::Value& schemeJson)
     }
     else
     {
-        auto scheme = ColorScheme::FromJson(schemeJson);
-        _globals.GetColorSchemes().emplace_back(scheme);
+        _globals.AddScheme(ColorScheme::FromJson(schemeJson));
     }
 }
 
@@ -590,13 +539,13 @@ ColorScheme* CascadiaSettings::_FindMatchingColorScheme(const Json::Value& schem
 {
     for (auto& scheme : _globals.GetColorSchemes())
     {
-        if (scheme.ShouldBeLayered(schemeJson))
+        if (scheme.second.ShouldBeLayered(schemeJson))
         {
             // HERE BE DRAGONS: Returning a pointer to a type in the vector is
             // maybe not the _safest_ thing, but we have a mind to make Profile
             // and ColorScheme winrt types in the future, so this will be safer
             // then.
-            return &scheme;
+            return &scheme.second;
         }
     }
     return nullptr;
diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.cpp b/src/cascadia/TerminalApp/GlobalAppSettings.cpp
index 2fe54b0540c..1b539b286e4 100644
--- a/src/cascadia/TerminalApp/GlobalAppSettings.cpp
+++ b/src/cascadia/TerminalApp/GlobalAppSettings.cpp
@@ -49,12 +49,12 @@ GlobalAppSettings::~GlobalAppSettings()
 {
 }
 
-const std::vector<ColorScheme>& GlobalAppSettings::GetColorSchemes() const noexcept
+std::unordered_map<std::wstring, ColorScheme>& GlobalAppSettings::GetColorSchemes() noexcept
 {
     return _colorSchemes;
 }
 
-std::vector<ColorScheme>& GlobalAppSettings::GetColorSchemes() noexcept
+const std::unordered_map<std::wstring, ColorScheme>& GlobalAppSettings::GetColorSchemes() const noexcept
 {
     return _colorSchemes;
 }
@@ -280,3 +280,9 @@ std::wstring_view GlobalAppSettings::_SerializeTheme(const ElementTheme theme) n
         return SystemThemeValue;
     }
 }
+
+void GlobalAppSettings::AddScheme(ColorScheme scheme)
+{
+    std::wstring name{ scheme.GetName() };
+    _colorSchemes[name] = std::move(scheme);
+}
diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.h b/src/cascadia/TerminalApp/GlobalAppSettings.h
index b70640019f3..bb874167250 100644
--- a/src/cascadia/TerminalApp/GlobalAppSettings.h
+++ b/src/cascadia/TerminalApp/GlobalAppSettings.h
@@ -21,6 +21,7 @@ Author(s):
 namespace TerminalAppLocalTests
 {
     class SettingsTests;
+    class ColorSchemeTests;
 };
 
 namespace TerminalApp
@@ -34,8 +35,10 @@ class TerminalApp::GlobalAppSettings final
     GlobalAppSettings();
     ~GlobalAppSettings();
 
-    const std::vector<ColorScheme>& GetColorSchemes() const noexcept;
-    std::vector<ColorScheme>& GetColorSchemes() noexcept;
+    std::unordered_map<std::wstring, ColorScheme>& GetColorSchemes() noexcept;
+    const std::unordered_map<std::wstring, ColorScheme>& GetColorSchemes() const noexcept;
+    void AddScheme(ColorScheme scheme);
+
     void SetDefaultProfile(const GUID defaultProfile) noexcept;
     GUID GetDefaultProfile() const noexcept;
 
@@ -70,7 +73,7 @@ class TerminalApp::GlobalAppSettings final
     GUID _defaultProfile;
     winrt::com_ptr<winrt::TerminalApp::implementation::AppKeyBindings> _keybindings;
 
-    std::vector<ColorScheme> _colorSchemes;
+    std::unordered_map<std::wstring, ColorScheme> _colorSchemes;
 
     int32_t _initialRows;
     int32_t _initialCols;
@@ -88,4 +91,5 @@ class TerminalApp::GlobalAppSettings final
     static std::wstring_view _SerializeTheme(const winrt::Windows::UI::Xaml::ElementTheme theme) noexcept;
 
     friend class TerminalAppLocalTests::SettingsTests;
+    friend class TerminalAppLocalTests::ColorSchemeTests;
 };
diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp
index c80a6dfe210..ca60cc572d4 100644
--- a/src/cascadia/TerminalApp/Profile.cpp
+++ b/src/cascadia/TerminalApp/Profile.cpp
@@ -141,27 +141,6 @@ void Profile::SetSource(std::wstring_view sourceNamespace) noexcept
     _source = sourceNamespace;
 }
 
-// Function Description:
-// - Searches a list of color schemes to find one matching the given name. Will
-//return the first match in the list, if the list has multiple schemes with the same name.
-// Arguments:
-// - schemes: a list of schemes to search
-// - schemeName: the name of the sceme to look for
-// Return Value:
-// - a non-ownership pointer to the matching scheme if we found one, else nullptr
-const ColorScheme* _FindScheme(const std::vector<ColorScheme>& schemes,
-                               const std::wstring& schemeName)
-{
-    for (auto& scheme : schemes)
-    {
-        if (scheme.GetName() == schemeName)
-        {
-            return &scheme;
-        }
-    }
-    return nullptr;
-}
-
 // Method Description:
 // - Create a TerminalSettings from this object. Apply our settings, as well as
 //      any colors from our color scheme, if we have one.
@@ -169,7 +148,7 @@ const ColorScheme* _FindScheme(const std::vector<ColorScheme>& schemes,
 // - schemes: a list of schemes to look for our color scheme in, if we have one.
 // Return Value:
 // - a new TerminalSettings object with our settings in it.
-TerminalSettings Profile::CreateTerminalSettings(const std::vector<ColorScheme>& schemes) const
+TerminalSettings Profile::CreateTerminalSettings(const std::unordered_map<std::wstring, ColorScheme>& schemes) const
 {
     TerminalSettings terminalSettings{};
 
@@ -208,10 +187,10 @@ TerminalSettings Profile::CreateTerminalSettings(const std::vector<ColorScheme>&
 
     if (_schemeName)
     {
-        const ColorScheme* const matchingScheme = _FindScheme(schemes, _schemeName.value());
-        if (matchingScheme)
+        const auto found = schemes.find(_schemeName.value());
+        if (found != schemes.end())
         {
-            matchingScheme->ApplyScheme(terminalSettings);
+            found->second.ApplyScheme(terminalSettings);
         }
     }
     if (_defaultForeground)
@@ -714,6 +693,11 @@ void Profile::SetColorScheme(std::optional<std::wstring> schemeName) noexcept
     _schemeName = std::move(schemeName);
 }
 
+std::optional<std::wstring>& Profile::GetSchemeName() noexcept
+{
+    return _schemeName;
+}
+
 void Profile::SetAcrylicOpacity(double opacity) noexcept
 {
     _acrylicTransparency = opacity;
@@ -1188,31 +1172,3 @@ GUID Profile::GetGuidOrGenerateForJson(const Json::Value& json) noexcept
 
     return Profile::_GenerateGuidForProfile(name, source);
 }
-
-// Method Description:
-// - Validates that the `colorScheme` set for this Profile exists. If it does
-//   exist, returns true. Otherwise, we'll initialize our color table to the
-//   default color scheme, the Campbell scheme, and returns false.
-// - This is done
-//   for microsoft/terminal#2547. In the case where the scheme was invalid, we'd
-//   leave the table un-initialized, resulting in all the colors being RGB(0, 0,
-//   0,) _black_.
-// Arguments:
-// - schemes: a list of color schemes to check and ensure that our colorScheme exitsts in.
-// Return Value:
-// - true if the color scheme existed, false otherwise.
-bool Profile::ValidateColorScheme(const std::vector<::TerminalApp::ColorScheme>& schemes) noexcept
-{
-    const ColorScheme* const matchingScheme = _FindScheme(schemes, _schemeName.value());
-    // If we couldn't find a match, initialize the table to the campbell colors
-    if (matchingScheme == nullptr)
-    {
-        gsl::span<uint32_t> tableView{ &_colorTable[0], COLOR_TABLE_SIZE };
-        // Then use fill the first 16 values with the Campbell scheme
-        Utils::InitializeCampbellColorTable(tableView);
-
-        return false;
-    }
-
-    return true;
-}
diff --git a/src/cascadia/TerminalApp/Profile.h b/src/cascadia/TerminalApp/Profile.h
index 63e413bf769..a0d113f2f49 100644
--- a/src/cascadia/TerminalApp/Profile.h
+++ b/src/cascadia/TerminalApp/Profile.h
@@ -45,7 +45,7 @@ class TerminalApp::Profile final
 
     ~Profile();
 
-    winrt::Microsoft::Terminal::Settings::TerminalSettings CreateTerminalSettings(const std::vector<::TerminalApp::ColorScheme>& schemes) const;
+    winrt::Microsoft::Terminal::Settings::TerminalSettings CreateTerminalSettings(const std::unordered_map<std::wstring, ColorScheme>& schemes) const;
 
     Json::Value ToJson() const;
     Json::Value DiffToJson(const Profile& other) const;
@@ -63,10 +63,9 @@ class TerminalApp::Profile final
     bool HasConnectionType() const noexcept;
     GUID GetConnectionType() const noexcept;
 
-    bool ValidateColorScheme(const std::vector<::TerminalApp::ColorScheme>& schemes) noexcept;
-
     void SetFontFace(std::wstring fontFace) noexcept;
     void SetColorScheme(std::optional<std::wstring> schemeName) noexcept;
+    std::optional<std::wstring>& GetSchemeName() noexcept;
     void SetTabTitle(std::wstring tabTitle) noexcept;
     void SetAcrylicOpacity(double opacity) noexcept;
     void SetCommandline(std::wstring cmdline) noexcept;
diff --git a/src/cascadia/ut_app/JsonTests.cpp b/src/cascadia/ut_app/JsonTests.cpp
index 36c4f3504cc..1e2884cc663 100644
--- a/src/cascadia/ut_app/JsonTests.cpp
+++ b/src/cascadia/ut_app/JsonTests.cpp
@@ -98,6 +98,7 @@ namespace TerminalAppUnitTests
         std::array<COLORREF, COLOR_TABLE_SIZE> expectedCampbellTable;
         auto campbellSpan = gsl::span<COLORREF>(&expectedCampbellTable[0], gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE));
         Utils::InitializeCampbellColorTable(campbellSpan);
+        Utils::SetColorTableAlpha(campbellSpan, 0);
 
         for (size_t i = 0; i < expectedCampbellTable.size(); i++)
         {

From bfc6c618a8cc6b83285b417185b5fc0ee6d723b2 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Fri, 4 Oct 2019 08:49:12 -0500
Subject: [PATCH 3/4] Final bits of polish on the vector->map change

---
 .../LocalTests_TerminalApp/SettingsTests.cpp    | 17 ++++++++++-------
 src/cascadia/TerminalApp/GlobalAppSettings.cpp  |  6 ++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
index 1fe7ea2463f..4e1b6536252 100644
--- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
+++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
@@ -394,9 +394,10 @@ namespace TerminalAppLocalTests
 
         settings->_ValidateSettings();
 
-        VERIFY_ARE_EQUAL(2u, settings->_warnings.size());
+        VERIFY_ARE_EQUAL(3u, settings->_warnings.size());
         VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::DuplicateProfile, settings->_warnings.at(0));
         VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile, settings->_warnings.at(1));
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme, settings->_warnings.at(2));
 
         VERIFY_ARE_EQUAL(3u, settings->_profiles.size());
         VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid());
@@ -794,6 +795,9 @@ namespace TerminalAppLocalTests
                 {
                     "name" : "profile1"
                 }
+            ],
+            "schemes": [
+                { "name": "Campbell" }
             ]
         })" };
 
@@ -1235,6 +1239,10 @@ namespace TerminalAppLocalTests
         VERIFY_ARE_EQUAL(3u, settings._profiles.size());
         VERIFY_ARE_EQUAL(2u, settings._globals._colorSchemes.size());
 
+        VERIFY_ARE_EQUAL(L"schemeOne", settings._profiles.at(0)._schemeName.value());
+        VERIFY_ARE_EQUAL(L"InvalidSchemeName", settings._profiles.at(1)._schemeName.value());
+        VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(2)._schemeName.value());
+
         settings._ValidateAllSchemesExist();
 
         VERIFY_ARE_EQUAL(1u, settings._warnings.size());
@@ -1243,13 +1251,8 @@ namespace TerminalAppLocalTests
         VERIFY_ARE_EQUAL(3u, settings._profiles.size());
         VERIFY_ARE_EQUAL(2u, settings._globals._colorSchemes.size());
 
-        VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), settings._profiles.at(0)._colorTable[0]);
-        VERIFY_ARE_EQUAL(ARGB(0, 12, 12, 12), settings._profiles.at(1)._colorTable[0]);
-        // In this set of schemes, Campbell doesn't exist, so the table will get written
-        VERIFY_ARE_EQUAL(ARGB(0, 12, 12, 12), settings._profiles.at(2)._colorTable[0]);
-
         VERIFY_ARE_EQUAL(L"schemeOne", settings._profiles.at(0)._schemeName.value());
-        VERIFY_ARE_EQUAL(L"InvalidSchemeName", settings._profiles.at(1)._schemeName.value());
+        VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(1)._schemeName.value());
         VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(2)._schemeName.value());
     }
 }
diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.cpp b/src/cascadia/TerminalApp/GlobalAppSettings.cpp
index 1b539b286e4..eb4f7448467 100644
--- a/src/cascadia/TerminalApp/GlobalAppSettings.cpp
+++ b/src/cascadia/TerminalApp/GlobalAppSettings.cpp
@@ -281,6 +281,12 @@ std::wstring_view GlobalAppSettings::_SerializeTheme(const ElementTheme theme) n
     }
 }
 
+// Method Description:
+// - Adds the given colorscheme to our map of schemes, using its name as the key.
+// Arguments:
+// - scheme: the color scheme to add
+// Return Value:
+// - <none>
 void GlobalAppSettings::AddScheme(ColorScheme scheme)
 {
     std::wstring name{ scheme.GetName() };

From 598837339d5bd153ab2605846bcf6c13a185d92c Mon Sep 17 00:00:00 2001
From: "Dustin L. Howett" <duhowett@microsoft.com>
Date: Mon, 14 Oct 2019 21:08:45 -0700
Subject: [PATCH 4/4] CR feedback

---
 .../CascadiaSettingsSerialization.cpp         |  2 +-
 .../TerminalApp/GlobalAppSettings.cpp         |  2 +-
 src/cascadia/TerminalApp/GlobalAppSettings.h  |  2 +-
 src/types/inc/utils.hpp                       | 32 +------------------
 src/types/utils.cpp                           | 31 ++++++++++++++++++
 5 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
index c56949b612b..6eaa4d79ed9 100644
--- a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
@@ -520,7 +520,7 @@ void CascadiaSettings::_LayerOrCreateColorScheme(const Json::Value& schemeJson)
     }
     else
     {
-        _globals.AddScheme(ColorScheme::FromJson(schemeJson));
+        _globals.AddColorScheme(ColorScheme::FromJson(schemeJson));
     }
 }
 
diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.cpp b/src/cascadia/TerminalApp/GlobalAppSettings.cpp
index eb4f7448467..207927aec84 100644
--- a/src/cascadia/TerminalApp/GlobalAppSettings.cpp
+++ b/src/cascadia/TerminalApp/GlobalAppSettings.cpp
@@ -287,7 +287,7 @@ std::wstring_view GlobalAppSettings::_SerializeTheme(const ElementTheme theme) n
 // - scheme: the color scheme to add
 // Return Value:
 // - <none>
-void GlobalAppSettings::AddScheme(ColorScheme scheme)
+void GlobalAppSettings::AddColorScheme(ColorScheme scheme)
 {
     std::wstring name{ scheme.GetName() };
     _colorSchemes[name] = std::move(scheme);
diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.h b/src/cascadia/TerminalApp/GlobalAppSettings.h
index bb874167250..da9ad71d05d 100644
--- a/src/cascadia/TerminalApp/GlobalAppSettings.h
+++ b/src/cascadia/TerminalApp/GlobalAppSettings.h
@@ -37,7 +37,7 @@ class TerminalApp::GlobalAppSettings final
 
     std::unordered_map<std::wstring, ColorScheme>& GetColorSchemes() noexcept;
     const std::unordered_map<std::wstring, ColorScheme>& GetColorSchemes() const noexcept;
-    void AddScheme(ColorScheme scheme);
+    void AddColorScheme(ColorScheme scheme);
 
     void SetDefaultProfile(const GUID defaultProfile) noexcept;
     GUID GetDefaultProfile() const noexcept;
diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp
index 0102f8c822c..ff1c8dd1781 100644
--- a/src/types/inc/utils.hpp
+++ b/src/types/inc/utils.hpp
@@ -36,37 +36,7 @@ namespace Microsoft::Console::Utils
     std::string ColorToHexString(const COLORREF color);
     COLORREF ColorFromHexString(const std::string wstr);
 
-    // Function Description:
-    // - Fill the first 16 entries of a given color table with the Campbell color
-    //   scheme, in the ANSI/VT RGB order.
-    // Arguments:
-    // - table: a color table with at least 16 entries
-    // Return Value:
-    // - <none>, throws if the table has less that 16 entries
-    template<typename T>
-    void InitializeCampbellColorTable(const gsl::span<T> table)
-    {
-        THROW_HR_IF(E_INVALIDARG, table.size() < 16);
-
-        // clang-format off
-        table[0]   = RGB( 12,   12,   12);
-        table[1]   = RGB( 197,  15,   31);
-        table[2]   = RGB( 19,   161,  14);
-        table[3]   = RGB( 193,  156,  0);
-        table[4]   = RGB( 0,    55,   218);
-        table[5]   = RGB( 136,  23,   152);
-        table[6]   = RGB( 58,   150,  221);
-        table[7]   = RGB( 204,  204,  204);
-        table[8]   = RGB( 118,  118,  118);
-        table[9]   = RGB( 231,  72,   86);
-        table[10]  = RGB( 22,   198,  12);
-        table[11]  = RGB( 249,  241,  165);
-        table[12]  = RGB( 59,   120,  255);
-        table[13]  = RGB( 180,  0,    158);
-        table[14]  = RGB( 97,   214,  214);
-        table[15]  = RGB( 242,  242,  242);
-        // clang-format on
-    };
+    void InitializeCampbellColorTable(const gsl::span<COLORREF> table);
     void InitializeCampbellColorTableForConhost(const gsl::span<COLORREF> table);
     void SwapANSIColorOrderForConhost(const gsl::span<COLORREF> table);
     void Initialize256ColorTable(const gsl::span<COLORREF> table);
diff --git a/src/types/utils.cpp b/src/types/utils.cpp
index 173914b4167..5916d27ef9d 100644
--- a/src/types/utils.cpp
+++ b/src/types/utils.cpp
@@ -117,6 +117,37 @@ bool Utils::IsValidHandle(const HANDLE handle) noexcept
     return handle != nullptr && handle != INVALID_HANDLE_VALUE;
 }
 
+// Function Description:
+// - Fill the first 16 entries of a given color table with the Campbell color
+//   scheme, in the ANSI/VT RGB order.
+// Arguments:
+// - table: a color table with at least 16 entries
+// Return Value:
+// - <none>, throws if the table has less that 16 entries
+void Utils::InitializeCampbellColorTable(const gsl::span<COLORREF> table)
+{
+    THROW_HR_IF(E_INVALIDARG, table.size() < 16);
+
+    // clang-format off
+    table[0]   = RGB( 12,   12,   12);
+    table[1]   = RGB( 197,  15,   31);
+    table[2]   = RGB( 19,   161,  14);
+    table[3]   = RGB( 193,  156,  0);
+    table[4]   = RGB( 0,    55,   218);
+    table[5]   = RGB( 136,  23,   152);
+    table[6]   = RGB( 58,   150,  221);
+    table[7]   = RGB( 204,  204,  204);
+    table[8]   = RGB( 118,  118,  118);
+    table[9]   = RGB( 231,  72,   86);
+    table[10]  = RGB( 22,   198,  12);
+    table[11]  = RGB( 249,  241,  165);
+    table[12]  = RGB( 59,   120,  255);
+    table[13]  = RGB( 180,  0,    158);
+    table[14]  = RGB( 97,   214,  214);
+    table[15]  = RGB( 242,  242,  242);
+    // clang-format on
+}
+
 // Function Description:
 // - Fill the first 16 entries of a given color table with the Campbell color
 //   scheme, in the Windows BGR order.