[fxcop] Settings UI library (part 2) (#7257)

* Suppress warnings for read-only collection properties (see code comments) * Call ConfigureAwait on tasks * Add CultureInfo and StringComparison policy for certain string operations * Add checks and exceptions for null arguments to public methods * Rename RaisePropertyChanged to NotifyPropertyChanged * Suppress CA1000 warning on SettingsRepository class * Implement Disposable pattern in HotkeySettingsControlHook * Modify null argument handling in KeyboardManagerViewModel::CombineShortcutLists
microsoft · Oct 19, 2020 · 688f134 · 688f134
1 parent 03509e7
commit 688f134
Show file tree

Hide file tree

Showing 23 changed files with 326 additions and 65 deletions.
diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/AppSpecificKeysDataModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/AppSpecificKeysDataModel.cs
@@ -2,6 +2,7 @@
 // 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.Collections.Generic;
 using System.Text.Json.Serialization;
 
@@ -24,7 +25,15 @@ public new List<string> GetMappedNewRemapKeys()
 
         public bool Compare(AppSpecificKeysDataModel arg)
         {
-            return OriginalKeys.Equals(arg.OriginalKeys) && NewRemapKeys.Equals(arg.NewRemapKeys) && TargetApp.Equals(arg.TargetApp);
+            if (arg == null)
+            {
+                throw new ArgumentNullException(nameof(arg));
+            }
+
+            // Using Ordinal comparison for internal text
+            return OriginalKeys.Equals(arg.OriginalKeys, StringComparison.Ordinal) &&
+                NewRemapKeys.Equals(arg.NewRemapKeys, StringComparison.Ordinal) &&
+                TargetApp.Equals(arg.TargetApp, StringComparison.Ordinal);
         }
     }
 }
diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ColorPickerSettings.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ColorPickerSettings.cs
@@ -2,6 +2,7 @@
 // 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.Text.Json;
 using System.Text.Json.Serialization;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
@@ -30,6 +31,11 @@ public virtual void Save(ISettingsUtils settingsUtils)
                 WriteIndented = true,
             };
 
+            if (settingsUtils == null)
+            {
+                throw new ArgumentNullException(nameof(settingsUtils));
+            }
+
             settingsUtils.SaveSettings(JsonSerializer.Serialize(this, options), ModuleName);
         }
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/CustomAction/SendCustomAction.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/CustomAction/SendCustomAction.cs
@@ -25,7 +25,8 @@ public string ToJsonString()
             {
                 PropertyNamingPolicy = new CustomNamePolicy((propertyName) =>
                 {
-                    return propertyName.Equals("ModuleAction") ? moduleName : propertyName;
+                    // Using Ordinal as this is an internal property name
+                    return propertyName.Equals("ModuleAction", System.StringComparison.Ordinal) ? moduleName : propertyName;
                 }),
             };
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/HotkeySettingsControlHook.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/HotkeySettingsControlHook.cs
@@ -13,7 +13,7 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 
     public delegate bool FilterAccessibleKeyboardEvents(int key, UIntPtr extraInfo);
 
-    public class HotkeySettingsControlHook
+    public class HotkeySettingsControlHook : IDisposable
     {
         private const int WmKeyDown = 0x100;
         private const int WmKeyUp = 0x101;
@@ -24,6 +24,7 @@ public class HotkeySettingsControlHook
         private KeyEvent _keyDown;
         private KeyEvent _keyUp;
         private IsActive _isActive;
+        private bool disposedValue;
 
         private FilterAccessibleKeyboardEvents _filterKeyboardEvent;
 
@@ -62,10 +63,24 @@ private bool FilterKeyboardEvents(KeyboardEvent ev)
             return _filterKeyboardEvent(ev.key, (UIntPtr)ev.dwExtraInfo);
         }
 
+        protected virtual void Dispose(bool disposing)
+        {
+            if (!disposedValue)
+            {
+                if (disposing)
+                {
+                    // Dispose the KeyboardHook object to terminate the hook threads
+                    _hook.Dispose();
+                }
+
+                disposedValue = true;
+            }
+        }
+
         public void Dispose()
         {
-            // Dispose the KeyboardHook object to terminate the hook threads
-            _hook.Dispose();
+            Dispose(disposing: true);
+            GC.SuppressFinalize(this);
         }
     }
 }
diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ImageSize.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ImageSize.cs
@@ -2,6 +2,7 @@
 // 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.ComponentModel;
 using System.Runtime.CompilerServices;
 using System.Text.Json;
@@ -222,6 +223,11 @@ public void OnPropertyChanged([CallerMemberName] string propertyName = null)
 
         public void Update(ImageSize modifiedSize)
         {
+            if (modifiedSize == null)
+            {
+                throw new ArgumentNullException(nameof(modifiedSize));
+            }
+
             Id = modifiedSize.Id;
             Name = modifiedSize.Name;
             Fit = modifiedSize.Fit;

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ImageresizerSizes.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ImageresizerSizes.cs
@@ -10,8 +10,13 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 {
     public class ImageResizerSizes
     {
+        // Suppressing this warning because removing the setter breaks
+        // deserialization with System.Text.Json. This affects the UI display.
+        // See: https://github.com/dotnet/runtime/issues/30258
         [JsonPropertyName("value")]
+#pragma warning disable CA2227 // Collection properties should be read only
         public ObservableCollection<ImageSize> Value { get; set; }
+#pragma warning restore CA2227 // Collection properties should be read only
 
         public ImageResizerSizes()
         {

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/PowerLauncherSettings.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/PowerLauncherSettings.cs
@@ -2,6 +2,7 @@
 // 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.Text.Json;
 using System.Text.Json.Serialization;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
@@ -30,6 +31,11 @@ public virtual void Save(ISettingsUtils settingsUtils)
                 WriteIndented = true,
             };
 
+            if (settingsUtils == null)
+            {
+                throw new ArgumentNullException(nameof(settingsUtils));
+            }
+
             settingsUtils.SaveSettings(JsonSerializer.Serialize(this, options), ModuleName);
         }
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/PowerRenameSettings.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/PowerRenameSettings.cs
@@ -2,6 +2,7 @@
 // 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.Text.Json.Serialization;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
 
@@ -23,6 +24,11 @@ public PowerRenameSettings()
 
         public PowerRenameSettings(PowerRenameLocalProperties localProperties)
         {
+            if (localProperties == null)
+            {
+                throw new ArgumentNullException(nameof(localProperties));
+            }
+
             Properties = new PowerRenameProperties();
             Properties.PersistState.Value = localProperties.PersistState;
             Properties.MRUEnabled.Value = localProperties.MRUEnabled;

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/RemapKeysDataModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/RemapKeysDataModel.cs
@@ -10,8 +10,13 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 {
     public class RemapKeysDataModel
     {
+        // Suppressing this warning because removing the setter breaks
+        // deserialization with System.Text.Json. This affects the UI display.
+        // See: https://github.com/dotnet/runtime/issues/30258
         [JsonPropertyName("inProcess")]
+#pragma warning disable CA2227 // Collection properties should be read only
         public List<KeysDataModel> InProcessRemapKeys { get; set; }
+#pragma warning restore CA2227 // Collection properties should be read only
 
         public RemapKeysDataModel()
         {

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/SettingsRepository`1.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/SettingsRepository`1.cs
@@ -20,7 +20,11 @@ public class SettingsRepository<T> : ISettingsRepository<T>
 
         private T settingsConfig;
 
+        // Suppressing the warning as this is a singleton class and this method is
+        // necessarily static
+#pragma warning disable CA1000 // Do not declare static members on generic types
         public static SettingsRepository<T> GetInstance(ISettingsUtils settingsUtils)
+#pragma warning restore CA1000 // Do not declare static members on generic types
         {
             // To ensure that only one instance of Settings Repository is created in a multi-threaded environment.
             lock (_SettingsRepoLock)

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ShortcutsKeyDataModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ShortcutsKeyDataModel.cs
@@ -10,11 +10,18 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 {
     public class ShortcutsKeyDataModel
     {
+        // Suppressing these warnings because removing the setter breaks
+        // deserialization with System.Text.Json. This affects the UI display.
+        // See: https://github.com/dotnet/runtime/issues/30258
         [JsonPropertyName("global")]
+#pragma warning disable CA2227 // Collection properties should be read only
         public List<KeysDataModel> GlobalRemapShortcuts { get; set; }
+#pragma warning restore CA2227 // Collection properties should be read only
 
         [JsonPropertyName("appSpecific")]
+#pragma warning disable CA2227 // Collection properties should be read only
         public List<AppSpecificKeysDataModel> AppSpecificRemapShortcuts { get; set; }
+#pragma warning restore CA2227 // Collection properties should be read only
 
         public ShortcutsKeyDataModel()
         {

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/Utilities/Helper.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/Utilities/Helper.cs
@@ -92,6 +92,15 @@ public static int CompareVersions(string version1, string version2)
             {
                 // Split up the version strings into int[]
                 // Example: v10.0.2 -> {10, 0, 2};
+                if (version1 == null)
+                {
+                    throw new ArgumentNullException(nameof(version1));
+                }
+                else if (version2 == null)
+                {
+                    throw new ArgumentNullException(nameof(version2));
+                }
+
                 var v1 = version1.Substring(1).Split('.').Select(int.Parse).ToArray();
                 var v2 = version2.Substring(1).Split('.').Select(int.Parse).ToArray();
 

diff --git a/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/ColorPickerViewModel.cs b/src/core/Microsoft.PowerToys.Settings.UI.Lib/ViewModels/ColorPickerViewModel.cs
@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Globalization;
 using System.Text.Json;
 using Microsoft.PowerToys.Settings.UI.Lib.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib.Interface;
@@ -24,6 +25,11 @@ public class ColorPickerViewModel : Observable
         public ColorPickerViewModel(ISettingsUtils settingsUtils, ISettingsRepository<GeneralSettings> settingsRepository, Func<string, int> ipcMSGCallBackFunc)
         {
             // Obtain the general PowerToy settings configurations
+            if (settingsRepository == null)
+            {
+                throw new ArgumentNullException(nameof(settingsRepository));
+            }
+
             GeneralSettingsConfig = settingsRepository.SettingsConfig;
 
             _settingsUtils = settingsUtils ?? throw new ArgumentNullException(nameof(settingsUtils));
@@ -121,8 +127,13 @@ public int CopiedColorRepresentationIndex
 
         private void NotifySettingsChanged()
         {
+            // Using InvariantCulture as this is an IPC message
             SendConfigMSG(
-                   string.Format("{{ \"powertoys\": {{ \"{0}\": {1} }} }}", ColorPickerSettings.ModuleName, JsonSerializer.Serialize(_colorPickerSettings)));
+                   string.Format(
+                       CultureInfo.InvariantCulture,
+                       "{{ \"powertoys\": {{ \"{0}\": {1} }} }}",
+                       ColorPickerSettings.ModuleName,
+                       JsonSerializer.Serialize(_colorPickerSettings)));
         }
     }
 }