From 86d77103e9c69686c297490acb04775d43ef8b76 Mon Sep 17 00:00:00 2001 From: Luthfi Mawarid Date: Wed, 21 Oct 2020 12:32:53 -0700 Subject: [PATCH] [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 --- .../GeneralSettings.cs | 5 +- .../SettingsUtils.cs | 12 +++- .../Utilities/Logger.cs | 65 +++++++++++++++++++ .../ViewModels/FancyZonesViewModel.cs | 17 +++-- .../ViewModels/GeneralViewModel.cs | 14 ++-- .../ViewModels/ImageResizerViewModel.cs | 13 +++- .../ViewModels/KeyboardManagerViewModel.cs | 21 +++++- .../ViewModels/PowerRenameViewModel.cs | 13 +++- .../ColorPickerUI/Settings/UserSettings.cs | 1 - 9 files changed, 142 insertions(+), 19 deletions(-) create mode 100644 src/core/Microsoft.PowerToys.Settings.UI.Lib/Utilities/Logger.cs diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/GeneralSettings.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/GeneralSettings.cs index ea0c0107da..eafa9c9979 100644 --- 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib [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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib { 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 index 681727ba22..b5e1da3968 100644 --- 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib } // 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib _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 new file mode 100644 index 0000000000..e90d49b5f8 --- /dev/null +++ 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 index 86acc34cc8..21979ed88f 100644 --- 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.Globalization; 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels 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 index fe6a83c0fc..232bbf0be2 100644 --- 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels } } + [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,8 +254,9 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels { UpdateUIThemeCallBack(GeneralSettingsConfig.Theme); } - catch + catch (Exception e) { + Logger.LogError("Exception encountered when changing Settings theme", e); } NotifyPropertyChanged(); @@ -262,6 +264,7 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels } } + [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,8 +282,9 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels { UpdateUIThemeCallBack(GeneralSettingsConfig.Theme); } - catch + catch (Exception e) { + Logger.LogError("Exception encountered when changing Settings theme", e); } NotifyPropertyChanged(); @@ -288,6 +292,7 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels } } + [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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels { 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 index e86b59c9ad..003c210d4d 100644 --- 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels private Func 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 settingsRepository, Func ipcMSGCallBackFunc) { _settingsUtils = settingsUtils ?? throw new ArgumentNullException(nameof(settingsUtils)); @@ -39,8 +43,15 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels { Settings = _settingsUtils.GetSettings(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 index ba50ea8d05..efc8855b42 100644 --- 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels if (_settingsUtils.SettingsExists(PowerToyName)) { - // Todo: Be more resilient while reading and saving settings. - Settings = _settingsUtils.GetSettings(PowerToyName); + try + { + Settings = _settingsUtils.GetSettings(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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels } } } - 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 index fae8014c6a..740450d505 100644 --- 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 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels private Func 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 settingsRepository, Func ipcMSGCallBackFunc, string configFileSubfolder = "") { // Update Settings file folder: @@ -41,8 +45,15 @@ namespace Microsoft.PowerToys.Settings.UI.Lib.ViewModels PowerRenameLocalProperties localSettings = _settingsUtils.GetSettings(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 index 87466f352a..815913ea2b 100644 --- a/src/modules/colorPicker/ColorPickerUI/Settings/UserSettings.cs +++ b/src/modules/colorPicker/ColorPickerUI/Settings/UserSettings.cs @@ -6,7 +6,6 @@ using System; 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;