Add an error dialog when we fail to parse settings

src/cascadia/CascadiaPackage/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.
     -->
@@ -123,6 +123,21 @@
   <data name="AppName" xml:space="preserve">
     <value>Windows Terminal (Preview)</value>
   </data>
+  <data name="InitialJsonParseErrorText" xml:space="preserve">
+    <value>Settings could not be loaded from file - temporarily using the default settings. Check for syntax errors, including trailing commas.</value>
+  </data>
+  <data name="InitialJsonParseErrorTitle" xml:space="preserve">
+    <value>Failed to load settings</value>
+  </data>
+  <data name="Ok" xml:space="preserve">
+    <value>Ok</value>
+  </data>
+  <data name="ReloadJsonParseErrorText" xml:space="preserve">
+    <value>Settings could not be reloaded from file. Check for syntax errors, including trailing commas.</value>
+  </data>
+  <data name="ReloadJsonParseErrorTitle" xml:space="preserve">
+    <value>Failed to reload settings</value>
+  </data>
   <data name="AppDescriptionDev" xml:space="preserve">
     <value>The Windows Terminal, but Unofficial</value>
   </data>

src/cascadia/TerminalApp/App.cpp

@@ -6,6 +6,7 @@
 #include <shellapi.h>
 #include <filesystem>
 #include <winrt/Microsoft.UI.Xaml.XamlTypeInfo.h>
+#include <winrt/Windows.ApplicationModel.Resources.h>
 
 using namespace winrt::Windows::ApplicationModel::DataTransfer;
 using namespace winrt::Windows::UI::Xaml;
@@ -42,7 +43,9 @@ namespace winrt::TerminalApp::implementation
         base_type(parentProvider),
         _settings{  },
         _tabs{  },
-        _loadedInitialSettings{ false }
+        _loadedInitialSettings{ false },
+        _settingsLoadedResult{ S_OK },
+        _dialogLock{}
     {
         // For your own sanity, it's better to do setup outside the ctor.
         // If you do any setup in the ctor that ends up throwing an exception,
@@ -159,13 +162,76 @@ namespace winrt::TerminalApp::implementation
         {
             IInspectable g = res.Lookup(key);
             winrt::Windows::UI::Xaml::Style style = g.try_as<winrt::Windows::UI::Xaml::Style>();
+            _root.Style(style);
             _tabRow.Style(style);
         }
 
         // Apply the UI theme from our settings to our UI elements
         _ApplyTheme(_settings->GlobalSettings().GetRequestedTheme());
 
         _OpenNewTab(std::nullopt);
+
+        _root.Loaded({ this, &App::_OnLoaded });
+    }
+
+    // Method Description:
+    // - Show a ContentDialog with a single "Ok" button to dismiss. Looks up the
+    //   the title and text from our Resources using the provided keys.
+    // - Only one dialog can be visible at a time. If another dialog is visible
+    //   when this is called, nothing happens.
+    // Arguments:
+    // - titleKey: The key to use to lookup the title text from our resources.
+    // - contentKey: The key to use to lookup the content text from our resources.
+    fire_and_forget App::_ShowOkDialog(const winrt::hstring& titleKey,
+                                       const winrt::hstring& contentKey)
+    {
+        // DON'T release this lock in a wil::scope_exit. The scope_exit will get
+        // called when we await, which is not what we want.
+        std::unique_lock lock{ _dialogLock, std::try_to_lock };
+        if (!lock)
+        {
+            // Another dialog is visible.
+            return;
+        }
+
+        auto resourceLoader = Windows::ApplicationModel::Resources::ResourceLoader::GetForCurrentView();
+        auto title = resourceLoader.GetString(titleKey);
+        auto message = resourceLoader.GetString(contentKey);
+        auto buttonText = resourceLoader.GetString(L"Ok");
+
+        Controls::ContentDialog dialog;
+        dialog.Title(winrt::box_value(title));
+        dialog.Content(winrt::box_value(message));
+        dialog.CloseButtonText(buttonText);
+
+        // IMPORTANT: Add the dialog to the _root UIElement before you show it,
+        // so it knows how to attach to the XAML content.
+        _root.Children().Append(dialog);
+
+        // Display the dialog.
+        Controls::ContentDialogResult result = await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup);
+
+        // After the dialog is dismissed, the dialog lock (held by `lock`) will
+        // be released so another can be shown.
+    }
+
+    // Method Description:
+    // - Triggered when the application is fiished loading. If we failed to load
+    //   the settings, then this will display the error dialog. This is done
+    //   here instead of when loading the settings, because we need our UI to be
+    //   visible to display the dialog, and when we're loading the settings,
+    //   the UI might not be visible yet.
+    // Arguments:
+    // - <unused>
+    void App::_OnLoaded(const IInspectable& /*sender*/,
+                        const RoutedEventArgs& /*eventArgs*/)
+    {
+        if (FAILED(_settingsLoadedResult))
+        {
+            const winrt::hstring titleKey = L"InitialJsonParseErrorTitle";
+            const winrt::hstring textKey = L"InitialJsonParseErrorText";
+            _ShowOkDialog(titleKey, textKey);
+        }
     }
 
     // Method Description:
@@ -333,6 +399,38 @@ namespace winrt::TerminalApp::implementation
         bindings.OpenSettings([this]() { _OpenSettings(); });
     }
 
+    // Method Description:
+    // - Attempt to load the settings. If we fail for any reason, returns an error.
+    // Arguments:
+    // - saveOnLoad: If true, after loading the settings, we should re-write
+    //   them to the file, to make sure the schema is updated. See
+    //   `CascadiaSettings::LoadAll` for details.
+    // Return Value:
+    // - S_OK if we successfully parsed the settings, otherwise an appropriate HRESULT.
+    [[nodiscard]]
+    HRESULT App::_TryLoadSettings(const bool saveOnLoad) noexcept
+    {
+        HRESULT hr = E_FAIL;
+
+        try
+        {
+            auto newSettings = CascadiaSettings::LoadAll(saveOnLoad);
+            _settings = std::move(newSettings);
+            hr = S_OK;
+        }
+        catch (const winrt::hresult_error& e)
+        {
+            hr = e.code();
+            LOG_HR(hr);
+        }
+        catch (...)
+        {
+            hr = wil::ResultFromCaughtException();
+            LOG_HR(hr);
+        }
+        return hr;
+    }
+
     // Method Description:
     // - Initialized our settings. See CascadiaSettings for more details.
     //      Additionally hooks up our callbacks for keybinding events to the
@@ -342,7 +440,21 @@ namespace winrt::TerminalApp::implementation
     //      happening during startup, it'll need to happen on a background thread.
     void App::LoadSettings()
     {
-        _settings = CascadiaSettings::LoadAll();
+        // Attempt to load the settings.
+        // If it fails,
+        //  - use Default settings,
+        //  - don't persist them (LoadAll won't save them in this case).
+        //  - _settingsLoadedResult will be set to an error, indicating that
+        //    we should display the loading error.
+        //    * We can't display the error now, because we might not have a
+        //      UI yet. We'll display the error in _OnLoaded.
+        _settingsLoadedResult = _TryLoadSettings(true);
+
+        if (FAILED(_settingsLoadedResult))
+        {
+            _settings = std::make_unique<CascadiaSettings>();
+            _settings->CreateDefaults();
+        }
 
         _HookupKeyBindings(_settings->GetKeybindings());
 
@@ -399,19 +511,36 @@ namespace winrt::TerminalApp::implementation
 
     // Method Description:
     // - Reloads the settings from the profile.json.
-    // Arguments:
-    // - <none>
-    // Return Value:
-    // - <none>
     void App::_ReloadSettings()
     {
-        _settings = CascadiaSettings::LoadAll(false);
+        // Attempt to load our settings.
+        // If it fails,
+        //  - don't change the settings (and don't actually apply the new settings)
+        //  - don't persist them.
+        //  - display a loading error
+        _settingsLoadedResult = _TryLoadSettings(false);
+
+        if (FAILED(_settingsLoadedResult))
+        {
+            _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
+                const winrt::hstring titleKey = L"ReloadJsonParseErrorTitle";
+                const winrt::hstring textKey = L"ReloadJsonParseErrorText";
+                _ShowOkDialog(titleKey, textKey);
+            });
+
+            return;
+        }
+
+        // Here, we successfully reloaded the settings, and created a new
+        // TerminalSettings object.
+
         // Re-wire the keybindings to their handlers, as we'll have created a
         // new AppKeyBindings object.
         _HookupKeyBindings(_settings->GetKeybindings());
 
-        auto profiles = _settings->GetProfiles();
+        // Refresh UI elements
 
+        auto profiles = _settings->GetProfiles();
         for (auto &profile : profiles)
         {
             const GUID profileGuid = profile.GetGuid();
@@ -690,7 +819,7 @@ namespace winrt::TerminalApp::implementation
     //      Negative values of `delta` will move the view up by one page, and positive values
     //      will move the viewport down by one page.
     // Arguments:
-    // - delta: The direction to move the view relative to the current viewport(it 
+    // - delta: The direction to move the view relative to the current viewport(it
     //      is clamped between -1 and 1)
     void App::_ScrollPage(int delta)
     {

src/cascadia/TerminalApp/App.h

@@ -65,13 +65,20 @@ namespace winrt::TerminalApp::implementation
 
         std::unique_ptr<::TerminalApp::CascadiaSettings> _settings;
 
+        HRESULT _settingsLoadedResult;
+
         bool _loadedInitialSettings;
+        std::shared_mutex _dialogLock;
 
         wil::unique_folder_change_reader_nothrow _reader;
 
         void _Create();
         void _CreateNewTabFlyout();
 
+        fire_and_forget _ShowOkDialog(const winrt::hstring& titleKey, const winrt::hstring& contentKey);
+
+        [[nodiscard]]
+        HRESULT _TryLoadSettings(const bool saveOnLoad) noexcept;
         void _LoadSettings();
         void _OpenSettings();
 
@@ -101,6 +108,7 @@ namespace winrt::TerminalApp::implementation
         // MSFT:20641986: Add keybindings for New Window
         void _ScrollPage(int delta);
 
+        void _OnLoaded(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
         void _OnTabSelectionChanged(const IInspectable& sender, const Windows::UI::Xaml::Controls::SelectionChangedEventArgs& eventArgs);
         void _OnTabClosing(const IInspectable& sender, const Microsoft::UI::Xaml::Controls::TabViewTabClosingEventArgs& eventArgs);
         void _OnTabItemsChanged(const IInspectable& sender, const Windows::Foundation::Collections::IVectorChangedEventArgs& eventArgs);

src/cascadia/TerminalApp/App.xaml

@@ -1,3 +1,5 @@
+<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under
+the MIT License. See LICENSE in the project root for license information. -->
 <MSMarkup:XamlApplication
     x:Class="TerminalApp.App"
     xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
@@ -16,7 +18,6 @@
             <ResourceDictionary.MergedDictionaries>
                 <!-- Include the MUX Controls resources -->
                 <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>
-
                 <ResourceDictionary>
 
                     <!-- We're going to apply this style to the root Grid acting
@@ -29,6 +30,20 @@
                         <Setter Property="Background" Value="{ThemeResource ApplicationPageBackgroundThemeBrush}" />
                     </Style>
 
+                    <!-- Manually theme the CloseButton of a ContentDialog. We
+                    need to do this, because for whatever reason, if we show a
+                    ContentDialog when the app theme is opposite the system
+                    theme, the buttons will appear transparent. This only
+                    applies to the Close button of the dialog, since we're only
+                    using the Close button of the dialog. If we ever add other
+                    dialogs with more buttons, we'll probably want to make sure
+                    the buttons are styled differently. -->
+                    <Style TargetType="ContentDialog">
+                        <!-- the value `AccentButtonStyle` is taken straight
+                        from the ContentDialog source -->
+                        <Setter Target="CloseButtonStyle" Value="{StaticResource AccentButtonStyle}" />
+                    </Style>
+
                     <ResourceDictionary.ThemeDictionaries>
                         <ResourceDictionary x:Key="Dark">
                             <!-- Define resources for Dark mode here -->
@@ -39,8 +54,11 @@
                         </ResourceDictionary>
 
                     </ResourceDictionary.ThemeDictionaries>
+
                 </ResourceDictionary>
             </ResourceDictionary.MergedDictionaries>
         </ResourceDictionary>
+
+
     </MSMarkup:XamlApplication.Resources>
 </MSMarkup:XamlApplication>

src/cascadia/TerminalApp/CascadiaSettings.cpp

@@ -326,7 +326,7 @@ void CascadiaSettings::_CreateDefaultKeybindings()
 // - <none>
 // Return Value:
 // - <none>
-void CascadiaSettings::_CreateDefaults()
+void CascadiaSettings::CreateDefaults()
 {
     _CreateDefaultProfiles();
     _CreateDefaultSchemes();