PR review fixes

Author: ibetitsmike

App/App.xaml.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
 using System.Threading;
 using System.Threading.Tasks;

App/Models/SettingsGroup.cs

@@ -0,0 +1,62 @@
+using System;
+
+namespace Coder.Desktop.App.Models;
+
+
+public interface ISettings<T> : ICloneable<T>
+{
+    /// <summary>
+    /// FileName where the settings are stored.
+    /// </summary>
+    static abstract string SettingsFileName { get; }
+
+    /// <summary>
+    /// Gets the version of the settings schema.
+    /// </summary>
+    int Version { get; }
+}
+
+public interface ICloneable<T>
+{
+    /// <summary>
+    /// Creates a deep copy of the settings object.
+    /// </summary>
+    /// <returns>A new instance of the settings object with the same values.</returns>
+    T Clone();
+}
+
+/// <summary>
+/// CoderConnect settings class that holds the settings for the CoderConnect feature.
+/// </summary>
+public class CoderConnectSettings : ISettings<CoderConnectSettings>
+{
+    public static string SettingsFileName { get; } = "coder-connect-settings.json";
+    public int Version { get; set; }
+    public bool ConnectOnLaunch { get; set; }
+
+    /// <summary>
+    /// CoderConnect current settings version. Increment this when the settings schema changes.
+    /// In future iterations we will be able to handle migrations when the user has
+    /// an older version.
+    /// </summary>
+    private const int VERSION = 1;
+
+    public CoderConnectSettings()
+    {
+        Version = VERSION;
+
+        ConnectOnLaunch = false;
+    }
+
+    public CoderConnectSettings(int? version, bool connectOnLaunch)
+    {
+        Version = version ?? VERSION;
+
+        ConnectOnLaunch = connectOnLaunch;
+    }
+
+    public CoderConnectSettings Clone()
+    {
+        return new CoderConnectSettings(Version, ConnectOnLaunch);
+    }
+}

App/Services/SettingsManager.cs

@@ -1,20 +1,16 @@
-using Google.Protobuf.WellKnownTypes;
-using Serilog;
 using System;
-using System.Collections.Generic;
 using System.IO;
 using System.Text.Json;
-using System.Text.Json.Serialization;
 using System.Threading;
 using System.Threading.Tasks;
-using System.Xml.Linq;
+using Coder.Desktop.App.Models;
 
 namespace Coder.Desktop.App.Services;
 
 /// <summary>
 /// Settings contract exposing properties for app settings.
 /// </summary>
-public interface ISettingsManager<T> where T : ISettings, new()
+public interface ISettingsManager<T> where T : ISettings<T>, new()
 {
     /// <summary>
     /// Reads the settings from the file system.
@@ -23,26 +19,25 @@ namespace Coder.Desktop.App.Services;
     /// </summary>
     /// <param name="ct"></param>
     /// <returns></returns>
-    public Task<T> Read(CancellationToken ct = default);
+    Task<T> Read(CancellationToken ct = default);
     /// <summary>
     /// Writes the settings to the file system.
     /// </summary>
     /// <param name="settings">Object containing the settings.</param>
     /// <param name="ct"></param>
     /// <returns></returns>
-    public Task Write(T settings, CancellationToken ct = default);
+    Task Write(T settings, CancellationToken ct = default);
 }
 
 /// <summary>
 /// Implemention of <see cref="ISettingsManager"/> that persists settings to a JSON file
 /// located in the user's local application data folder.
 /// </summary>
-public sealed class SettingsManager<T> : ISettingsManager<T> where T : ISettings, new()
+public sealed class SettingsManager<T> : ISettingsManager<T> where T : ISettings<T>, new()
 {
     private readonly string _settingsFilePath;
     private readonly string _appName = "CoderDesktop";
     private string _fileName;
-    private readonly object _lock = new();
 
     private T? _cachedSettings;
 
@@ -79,7 +74,7 @@ public async Task<T> Read(CancellationToken ct = default)
         if (_cachedSettings is not null)
         {
             // return cached settings if available
-            return (T)_cachedSettings.Clone();
+            return _cachedSettings.Clone();
         }
 
         // try to get the lock with short timeout
@@ -98,7 +93,7 @@ public async Task<T> Read(CancellationToken ct = default)
             // deserialize; fall back to default(T) if empty or malformed
             var result = JsonSerializer.Deserialize<T>(json)!;
             _cachedSettings = result;
-            return result;
+            return _cachedSettings.Clone(); // return a fresh instance of the settings
         }
         catch (OperationCanceledException)
         {
@@ -148,57 +143,3 @@ await File.WriteAllTextAsync(_settingsFilePath, json, ct)
         }
     }
 }
-
-public interface ISettings
-{
-    /// <summary>
-    /// FileName where the settings are stored.
-    /// </summary>
-    static abstract string SettingsFileName { get; }
-
-    /// <summary>
-    /// Gets the version of the settings schema.
-    /// </summary>
-    int Version { get; }
-
-    ISettings Clone();
-}
-
-/// <summary>
-/// CoderConnect settings class that holds the settings for the CoderConnect feature.
-/// </summary>
-public class CoderConnectSettings : ISettings
-{
-    public static string SettingsFileName { get; } = "coder-connect-settings.json";
-    public int Version { get; set; }
-    public bool ConnectOnLaunch { get; set; }
-
-    /// <summary>
-    /// CoderConnect current settings version. Increment this when the settings schema changes.
-    /// In future iterations we will be able to handle migrations when the user has
-    /// an older version.
-    /// </summary>
-    private const int VERSION = 1;
-
-    public CoderConnectSettings()
-    {
-        Version = VERSION;
-        ConnectOnLaunch = false;
-    }
-
-    public CoderConnectSettings(int? version, bool connectOnLogin)
-    {
-        Version = version ?? VERSION;
-        ConnectOnLaunch = connectOnLogin;
-    }
-
-    ISettings ISettings.Clone()
-    {
-        return Clone();
-    }
-
-    public CoderConnectSettings Clone()
-    {
-        return new CoderConnectSettings(Version, ConnectOnLaunch);
-    }
-}

App/ViewModels/SettingsViewModel.cs

@@ -1,8 +1,7 @@
+using Coder.Desktop.App.Models;
 using Coder.Desktop.App.Services;
 using CommunityToolkit.Mvvm.ComponentModel;
 using Microsoft.Extensions.Logging;
-using Microsoft.UI.Dispatching;
-using Microsoft.UI.Xaml;
 using System;
 
 namespace Coder.Desktop.App.ViewModels;
@@ -29,9 +28,7 @@ public SettingsViewModel(ILogger<SettingsViewModel> logger, ISettingsManager<Cod
         _connectSettingsManager = settingsManager;
         _startupManager = startupManager;
         _logger = logger;
-        // Application settings are loaded on application startup,
-        // so we expect the settings to be available immediately.
-        var settingsCache = settingsManager.Read();
+        _connectSettings = settingsManager.Read().GetAwaiter().GetResult();
         StartOnLogin = startupManager.IsEnabled();
         ConnectOnLaunch = _connectSettings.ConnectOnLaunch;
 

App/Views/Pages/TrayWindowLoginRequiredPage.xaml

@@ -36,7 +36,7 @@
         </HyperlinkButton>
 
         <HyperlinkButton
-            Command="{x:Bind ViewModel.ExitCommand, Mode=OneWay}"
+            Command="{x:Bind ViewModel.ExitCommand}"
             Margin="-12,-8,-12,-5"
             HorizontalAlignment="Stretch"
             HorizontalContentAlignment="Left"> 

Tests.App/Services/SettingsManagerTest.cs

@@ -1,3 +1,4 @@
+using Coder.Desktop.App.Models;
 using Coder.Desktop.App.Services;
 
 namespace Coder.Desktop.Tests.App.Services;
@@ -24,7 +25,7 @@ public void TearDown()
     [Test]
     public void Save_Persists()
     {
-        bool expected = true;
+        var expected = true;
         var settings = new CoderConnectSettings
         {
             Version = 1,