[fxcop] Settings UI library (part 3) - exception handling (#7385)

* Log general exceptions caught in Settings * Rethrow argument-related exceptions in debug mode * Log ColorPicker settings errors into Settings Logs
microsoft · Oct 21, 2020 · 86d7710 · 86d7710
1 parent 29ed39c
commit 86d7710
Show file tree

Hide file tree

Showing 9 changed files with 142 additions and 19 deletions.
diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/GeneralSettings.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/GeneralSettings.cs
@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 using System.Text.Json;
 using System.Text.Json.Serialization;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
@@ -53,6 +54,7 @@ public class GeneralSettings : ISettingsConfig
         [JsonPropertyName("download_updates_automatically")]
         public bool AutoDownloadUpdates { get; set; }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Any error from calling interop code should not prevent the program from loading.")]
         public GeneralSettings()
         {
             Packaged = false;
@@ -66,8 +68,9 @@ public GeneralSettings()
             {
                 PowertoysVersion = DefaultPowertoysVersion();
             }
-            catch
+            catch (Exception e)
             {
+                Logger.LogError("Exception encountered when getting PowerToys version", e);
                 PowertoysVersion = "v0.0.0";
             }
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/SettingsUtils.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/SettingsUtils.cs
@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
+using System.IO;
 using System.Text.Json;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
 using Microsoft.PowerToys.Settings.UI.Lib.Utilities;
@@ -100,6 +102,7 @@ private T GetFile<T>(string powertoyFolderName = DefaultModuleName, string fileN
         }
 
         // Save settings to a json file.
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "General exceptions will be logged until we can better understand runtime exception scenarios")]
         public void SaveSettings(string jsonSettings, string powertoy = DefaultModuleName, string fileName = DefaultFileName)
         {
             try
@@ -114,8 +117,15 @@ public void SaveSettings(string jsonSettings, string powertoy = DefaultModuleNam
                     _ioProvider.WriteAllText(GetSettingsPath(powertoy, fileName), jsonSettings);
                 }
             }
-            catch
+            catch (Exception e)
             {
+                Logger.LogError($"Exception encountered while saving {powertoy} settings.", e);
+#if DEBUG
+                if (e is ArgumentException || e is ArgumentNullException || e is PathTooLongException)
+                {
+                    throw e;
+                }
+#endif
             }
         }
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/Utilities/Logger.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/Utilities/Logger.cs
@@ -0,0 +1,65 @@
+// Copyright (c) Microsoft Corporation
+// The Microsoft Corporation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Diagnostics;
+using System.Globalization;
+using System.IO;
+
+namespace Microsoft.PowerToys.Settings.UI.Lib.Utilities
+{
+    public static class Logger
+    {
+        private static readonly string ApplicationLogPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "Microsoft\\PowerToys\\Settings Logs");
+
+        static Logger()
+        {
+            if (!Directory.Exists(ApplicationLogPath))
+            {
+                Directory.CreateDirectory(ApplicationLogPath);
+            }
+
+            var logFilePath = Path.Combine(ApplicationLogPath, "Log_" + DateTime.Now.ToString(@"yyyy-MM-dd", CultureInfo.InvariantCulture) + ".txt");
+
+            Trace.Listeners.Add(new TextWriterTraceListener(logFilePath));
+
+            Trace.AutoFlush = true;
+        }
+
+        public static void LogInfo(string message)
+        {
+            Log(message, "INFO");
+        }
+
+        public static void LogError(string message, Exception e)
+        {
+            Log(
+                message + Environment.NewLine +
+                e?.Message + Environment.NewLine +
+                "Inner exception: " + Environment.NewLine +
+                e?.InnerException?.Message + Environment.NewLine +
+                "Stack trace: " + Environment.NewLine +
+                e?.StackTrace,
+                "ERROR");
+        }
+
+        private static void Log(string message, string type)
+        {
+            Trace.WriteLine(type + ": " + DateTime.Now.TimeOfDay);
+            Trace.Indent();
+            Trace.WriteLine(GetCallerInfo());
+            Trace.WriteLine(message);
+            Trace.Unindent();
+        }
+
+        private static string GetCallerInfo()
+        {
+            StackTrace stackTrace = new StackTrace();
+
+            var methodName = stackTrace.GetFrame(3)?.GetMethod();
+            var className = methodName?.DeclaringType.Name;
+            return "[Method]: " + methodName?.Name + " [Class]: " + className;
+        }
+    }
+}
diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/FancyZonesViewModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/FancyZonesViewModel.cs
@@ -8,6 +8,7 @@
 using System.Runtime.CompilerServices;
 using Microsoft.PowerToys.Settings.UI.Lib.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
+using Microsoft.PowerToys.Settings.UI.Lib.Utilities;
 using Microsoft.PowerToys.Settings.UI.Lib.ViewModels.Commands;
 
 namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels
@@ -542,19 +543,21 @@ public void NotifyPropertyChanged([CallerMemberName] string propertyName = null)
 
         private static string ToRGBHex(string color)
         {
-            try
+            // Using InvariantCulture as these are expected to be hex codes.
+            bool success = int.TryParse(
+                color.Replace("#", string.Empty),
+                System.Globalization.NumberStyles.HexNumber,
+                CultureInfo.InvariantCulture,
+                out int argb);
+
+            if (success)
             {
-                // Using InvariantCulture as these are expected to be hex codes.
-                int argb = int.Parse(
-                    color.Replace("#", string.Empty),
-                    System.Globalization.NumberStyles.HexNumber,
-                    CultureInfo.InvariantCulture);
                 Color clr = Color.FromArgb(argb);
                 return "#" + clr.R.ToString("X2", CultureInfo.InvariantCulture) +
                     clr.G.ToString("X2", CultureInfo.InvariantCulture) +
                     clr.B.ToString("X2", CultureInfo.InvariantCulture);
             }
-            catch (Exception)
+            else
             {
                 return "#FFFFFF";
             }

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/GeneralViewModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/GeneralViewModel.cs
@@ -3,7 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
-using System.Globalization;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using Microsoft.PowerToys.Settings.UI.Lib.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
@@ -236,6 +236,7 @@ public bool AutoDownloadUpdates
             }
         }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "This may throw if the XAML page is not initialized in tests (https://github.com/microsoft/PowerToys/pull/2676)")]
         public bool IsDarkThemeRadioButtonChecked
         {
             get
@@ -253,15 +254,17 @@ public bool IsDarkThemeRadioButtonChecked
                     {
                         UpdateUIThemeCallBack(GeneralSettingsConfig.Theme);
                     }
-                    catch
+                    catch (Exception e)
                     {
+                        Logger.LogError("Exception encountered when changing Settings theme", e);
                     }
 
                     NotifyPropertyChanged();
                 }
             }
         }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "This may throw if the XAML page is not initialized in tests (https://github.com/microsoft/PowerToys/pull/2676)")]
         public bool IsLightThemeRadioButtonChecked
         {
             get
@@ -279,15 +282,17 @@ public bool IsLightThemeRadioButtonChecked
                     {
                         UpdateUIThemeCallBack(GeneralSettingsConfig.Theme);
                     }
-                    catch
+                    catch (Exception e)
                     {
+                        Logger.LogError("Exception encountered when changing Settings theme", e);
                     }
 
                     NotifyPropertyChanged();
                 }
             }
         }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "This may throw if the XAML page is not initialized in tests (https://github.com/microsoft/PowerToys/pull/2676)")]
         public bool IsSystemThemeRadioButtonChecked
         {
             get
@@ -305,8 +310,9 @@ public bool IsSystemThemeRadioButtonChecked
                     {
                         UpdateUIThemeCallBack(GeneralSettingsConfig.Theme);
                     }
-                    catch
+                    catch (Exception e)
                     {
+                        Logger.LogError("Exception encountered when changing Settings theme", e);
                     }
 
                     NotifyPropertyChanged();

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/ImageResizerViewModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/ImageResizerViewModel.cs
@@ -5,9 +5,12 @@
 using System;
 using System.Collections.ObjectModel;
 using System.ComponentModel;
+using System.Diagnostics.CodeAnalysis;
+using System.IO;
 using System.Linq;
 using Microsoft.PowerToys.Settings.UI.Lib.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
+using Microsoft.PowerToys.Settings.UI.Lib.Utilities;
 
 namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels
 {
@@ -23,6 +26,7 @@ public class ImageResizerViewModel : Observable
 
         private Func<string, int> SendConfigMSG { get; }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Exceptions should not crash the program but will be logged until we can understand common exception scenarios")]
         public ImageResizerViewModel(ISettingsUtils settingsUtils, ISettingsRepository<GeneralSettings> settingsRepository, Func<string, int> ipcMSGCallBackFunc)
         {
             _settingsUtils = settingsUtils ?? throw new ArgumentNullException(nameof(settingsUtils));
@@ -39,8 +43,15 @@ public ImageResizerViewModel(ISettingsUtils settingsUtils, ISettingsRepository<G
             {
                 Settings = _settingsUtils.GetSettings<ImageResizerSettings>(ModuleName);
             }
-            catch
+            catch (Exception e)
             {
+                Logger.LogError($"Exception encountered while reading {ModuleName} settings.", e);
+#if DEBUG
+                if (e is ArgumentException || e is ArgumentNullException || e is PathTooLongException)
+                {
+                    throw e;
+                }
+#endif
                 Settings = new ImageResizerSettings();
                 _settingsUtils.SaveSettings(Settings.ToJsonString(), ModuleName);
             }

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/KeyboardManagerViewModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/KeyboardManagerViewModel.cs
@@ -4,6 +4,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -58,8 +59,20 @@ public KeyboardManagerViewModel(ISettingsUtils settingsUtils, ISettingsRepositor
 
             if (_settingsUtils.SettingsExists(PowerToyName))
             {
-                // Todo: Be more resilient while reading and saving settings.
-                Settings = _settingsUtils.GetSettings<KeyboardManagerSettings>(PowerToyName);
+                try
+                {
+                    Settings = _settingsUtils.GetSettings<KeyboardManagerSettings>(PowerToyName);
+                }
+                catch (Exception e)
+                {
+                    Logger.LogError($"Exception encountered while reading {PowerToyName} settings.", e);
+#if DEBUG
+                    if (e is ArgumentException || e is ArgumentNullException || e is PathTooLongException)
+                    {
+                        throw e;
+                    }
+#endif
+                }
 
                 // Load profile.
                 if (!LoadProfile())
@@ -182,6 +195,7 @@ public void NotifyFileChanged()
             OnPropertyChanged(nameof(RemapShortcuts));
         }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Exceptions here (especially mutex errors) should not halt app execution, but they will be logged.")]
         public bool LoadProfile()
         {
             var success = true;
@@ -221,9 +235,10 @@ public bool LoadProfile()
                     }
                 }
             }
-            catch (Exception)
+            catch (Exception e)
             {
                 // Failed to load the configuration.
+                Logger.LogError($"Exception encountered when loading {PowerToyName} profile", e);
                 success = false;
             }
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/PowerRenameViewModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/PowerRenameViewModel.cs
@@ -3,9 +3,12 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
+using System.IO;
 using System.Runtime.CompilerServices;
 using Microsoft.PowerToys.Settings.UI.Lib.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
+using Microsoft.PowerToys.Settings.UI.Lib.Utilities;
 
 namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels
 {
@@ -23,6 +26,7 @@ public class PowerRenameViewModel : Observable
 
         private Func<string, int> SendConfigMSG { get; }
 
+        [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Exceptions should not crash the program but will be logged until we can understand common exception scenarios")]
         public PowerRenameViewModel(ISettingsUtils settingsUtils, ISettingsRepository<GeneralSettings> settingsRepository, Func<string, int> ipcMSGCallBackFunc, string configFileSubfolder = "")
         {
             // Update Settings file folder:
@@ -41,8 +45,15 @@ public PowerRenameViewModel(ISettingsUtils settingsUtils, ISettingsRepository<Ge
                 PowerRenameLocalProperties localSettings = _settingsUtils.GetSettings<PowerRenameLocalProperties>(GetSettingsSubPath(), "power-rename-settings.json");
                 Settings = new PowerRenameSettings(localSettings);
             }
-            catch
+            catch (Exception e)
             {
+                Logger.LogError($"Exception encountered while reading {ModuleName} settings.", e);
+#if DEBUG
+                if (e is ArgumentException || e is ArgumentNullException || e is PathTooLongException)
+                {
+                    throw e;
+                }
+#endif
                 PowerRenameLocalProperties localSettings = new PowerRenameLocalProperties();
                 Settings = new PowerRenameSettings(localSettings);
                 _settingsUtils.SaveSettings(localSettings.ToJsonString(), GetSettingsSubPath(), "power-rename-settings.json");

diff --git a/src/modules/colorPicker/ColorPickerUI/Settings/UserSettings.cs b/src/modules/colorPicker/ColorPickerUI/Settings/UserSettings.cs
@@ -6,7 +6,6 @@
 using System.ComponentModel.Composition;
 using System.IO;
 using System.Threading;
-using ColorPicker.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib;
 using Microsoft.PowerToys.Settings.UI.Lib.Utilities;