Skip to content

added new settings dialog + settings manager #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Jun 10, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions App/App.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public partial class App : Application
private readonly ILogger<App> _logger;
private readonly IUriHandler _uriHandler;

private readonly ISettingsManager _settingsManager;
private readonly ISettingsManager<CoderConnectSettings> _settingsManager;

private readonly IHostApplicationLifetime _appLifetime;

Expand Down Expand Up @@ -94,7 +94,7 @@ public App()
// FileSyncListMainPage is created by FileSyncListWindow.
services.AddTransient<FileSyncListWindow>();

services.AddSingleton<ISettingsManager, SettingsManager>();
services.AddSingleton<ISettingsManager<CoderConnectSettings>, SettingsManager<CoderConnectSettings>>();
services.AddSingleton<IStartupManager, StartupManager>();
// SettingsWindow views and view models
services.AddTransient<SettingsViewModel>();
Expand All @@ -118,10 +118,10 @@ public App()
services.AddTransient<TrayWindow>();

_services = services.BuildServiceProvider();
_logger = (ILogger<App>)_services.GetService(typeof(ILogger<App>))!;
_uriHandler = (IUriHandler)_services.GetService(typeof(IUriHandler))!;
_settingsManager = (ISettingsManager)_services.GetService(typeof(ISettingsManager))!;
_appLifetime = (IHostApplicationLifetime)_services.GetRequiredService<IHostApplicationLifetime>();
_logger = _services.GetRequiredService<ILogger<App>>();
_uriHandler = _services.GetRequiredService<IUriHandler>();
_settingsManager = _services.GetRequiredService<ISettingsManager<CoderConnectSettings>>();
_appLifetime = _services.GetRequiredService<IHostApplicationLifetime>();

InitializeComponent();
}
Expand Down Expand Up @@ -167,12 +167,15 @@ private async Task InitializeServicesAsync(CancellationToken cancellationToken =
using var credsCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
credsCts.CancelAfter(TimeSpan.FromSeconds(15));

Task loadCredsTask = credentialManager.LoadCredentials(credsCts.Token);
Task reconnectTask = rpcController.Reconnect(cancellationToken);
var loadCredsTask = credentialManager.LoadCredentials(credsCts.Token);
var reconnectTask = rpcController.Reconnect(cancellationToken);
var settingsTask = _settingsManager.Read(cancellationToken);

var dependenciesLoaded = true;

try
{
await Task.WhenAll(loadCredsTask, reconnectTask);
await Task.WhenAll(loadCredsTask, reconnectTask, settingsTask);
}
catch (Exception)
{
Expand All @@ -184,10 +187,17 @@ private async Task InitializeServicesAsync(CancellationToken cancellationToken =
_logger.LogError(reconnectTask.Exception!.GetBaseException(),
"Failed to connect to VPN service");

return;
if (settingsTask.IsFaulted)
_logger.LogError(settingsTask.Exception!.GetBaseException(),
"Failed to fetch Coder Connect settings");

// Don't attempt to connect if we failed to load credentials or reconnect.
// This will prevent the app from trying to connect to the VPN service.
dependenciesLoaded = false;
}

if (_settingsManager.ConnectOnLaunch)
var attemptCoderConnection = settingsTask.Result?.ConnectOnLaunch ?? false;
if (dependenciesLoaded && attemptCoderConnection)
{
try
{
Expand Down
216 changes: 114 additions & 102 deletions App/Services/SettingsManager.cs
Original file line number Diff line number Diff line change
@@ -1,65 +1,57 @@
using Google.Protobuf.WellKnownTypes;
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;

namespace Coder.Desktop.App.Services;

/// <summary>
/// Settings contract exposing properties for app settings.
/// </summary>
public interface ISettingsManager
public interface ISettingsManager<T> where T : ISettings, new()
{
/// <summary>
/// Returns the value of the StartOnLogin setting. Returns <c>false</c> if the key is not found.
/// Reads the settings from the file system.
/// Always returns the latest settings, even if they were modified by another instance of the app.
/// Returned object is always a fresh instance, so it can be modified without affecting the stored settings.
/// </summary>
bool StartOnLogin { get; set; }

/// <param name="ct"></param>
/// <returns></returns>
public 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);
/// <summary>
/// Returns the value of the ConnectOnLaunch setting. Returns <c>false</c> if the key is not found.
/// Returns null if the settings are not cached or not available.
/// </summary>
bool ConnectOnLaunch { get; set; }
/// <returns></returns>
public T? GetFromCache();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than have this, Read should probably return from cache by default. I know this goes against a point I made in a previous review but if we're going with the Read/Write API then it should just be all in

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Read should probably not fail if it failed to read/parse the file. IDK what good behavior is, but I think for now just returning a default config is OK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely going against this comment :D
#113 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm like 50/50 on whether we should crash or just use the default config... IDK what the best option is. For now with a single option maybe it's fine to just keep going, but in the future if we add config options that are more sensitive to being clobbered it might be bad to just ignore a parse failure...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it throws an exception now and I think that's OK considering - if you fail to operate the file there's a high likelihood that the feature won't work ata ll.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fair. We can revisit this if it's causing issues

}

/// <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 : ISettingsManager
public sealed class SettingsManager<T> : ISettingsManager<T> where T : ISettings, new()
{
private readonly string _settingsFilePath;
private Settings _settings;
private readonly string _fileName = "app-settings.json";
private readonly string _appName = "CoderDesktop";
private string _fileName;
private readonly object _lock = new();

public const string ConnectOnLaunchKey = "ConnectOnLaunch";
public const string StartOnLoginKey = "StartOnLogin";
private T? _cachedSettings;

public bool StartOnLogin
{
get
{
return Read(StartOnLoginKey, false);
}
set
{
Save(StartOnLoginKey, value);
}
}

public bool ConnectOnLaunch
{
get
{
return Read(ConnectOnLaunchKey, false);
}
set
{
Save(ConnectOnLaunchKey, value);
}
}
private readonly SemaphoreSlim _gate = new(1, 1);
private static readonly TimeSpan LockTimeout = TimeSpan.FromSeconds(3);

/// <param name="settingsFilePath">
/// For unit‑tests you can pass an absolute path that already exists.
Expand All @@ -81,109 +73,129 @@ public SettingsManager(string? settingsFilePath = null)
_appName);

Directory.CreateDirectory(folder);

_fileName = T.SettingsFileName;
_settingsFilePath = Path.Combine(folder, _fileName);
}

if (!File.Exists(_settingsFilePath))
public async Task<T> Read(CancellationToken ct = default)
{
// try to get the lock with short timeout
if (!await _gate.WaitAsync(LockTimeout, ct).ConfigureAwait(false))
throw new InvalidOperationException(
$"Could not acquire the settings lock within {LockTimeout.TotalSeconds} s.");

try
{
// Create the settings file if it doesn't exist
_settings = new();
File.WriteAllText(_settingsFilePath, JsonSerializer.Serialize(_settings, SettingsJsonContext.Default.Settings));
if (!File.Exists(_settingsFilePath))
return new();

var json = await File.ReadAllTextAsync(_settingsFilePath, ct)
.ConfigureAwait(false);

// deserialize; fall back to default(T) if empty or malformed
var result = JsonSerializer.Deserialize<T>(json)!;
_cachedSettings = result;
return result;
}
else
catch (OperationCanceledException)
{
_settings = Load();
throw; // propagate caller-requested cancellation
}
}

private void Save(string name, bool value)
{
lock (_lock)
catch (Exception ex)
{
try
{
// We lock the file for the entire operation to prevent concurrent writes
using var fs = new FileStream(_settingsFilePath,
FileMode.OpenOrCreate,
FileAccess.ReadWrite,
FileShare.None);

// Ensure cache is loaded before saving
var freshCache = JsonSerializer.Deserialize(fs, SettingsJsonContext.Default.Settings) ?? new();
_settings = freshCache;
_settings.Options[name] = JsonSerializer.SerializeToElement(value);
fs.Position = 0; // Reset stream position to the beginning before writing

JsonSerializer.Serialize(fs, _settings, SettingsJsonContext.Default.Settings);

// This ensures the file is truncated to the new length
// if the new content is shorter than the old content
fs.SetLength(fs.Position);
}
catch
{
throw new InvalidOperationException($"Failed to persist settings to {_settingsFilePath}. The file may be corrupted, malformed or locked.");
}
throw new InvalidOperationException(
$"Failed to read settings from {_settingsFilePath}. " +
"The file may be corrupted, malformed or locked.", ex);
}
}

private bool Read(string name, bool defaultValue)
{
lock (_lock)
finally
{
if (_settings.Options.TryGetValue(name, out var element))
{
try
{
return element.Deserialize<bool?>() ?? defaultValue;
}
catch
{
// malformed value – return default value
return defaultValue;
}
}
return defaultValue; // key not found – return default value
_gate.Release();
}
}

private Settings Load()
public async Task Write(T settings, CancellationToken ct = default)
{
// try to get the lock with short timeout
if (!await _gate.WaitAsync(LockTimeout, ct).ConfigureAwait(false))
throw new InvalidOperationException(
$"Could not acquire the settings lock within {LockTimeout.TotalSeconds} s.");

try
{
using var fs = File.OpenRead(_settingsFilePath);
return JsonSerializer.Deserialize(fs, SettingsJsonContext.Default.Settings) ?? new();
// overwrite the settings file with the new settings
var json = JsonSerializer.Serialize(
settings, new JsonSerializerOptions() { WriteIndented = true });
_cachedSettings = settings; // cache the settings
await File.WriteAllTextAsync(_settingsFilePath, json, ct)
.ConfigureAwait(false);
}
catch (OperationCanceledException)
{
throw; // let callers observe cancellation
}
catch (Exception ex)
{
throw new InvalidOperationException($"Failed to load settings from {_settingsFilePath}. The file may be corrupted or malformed. Exception: {ex.Message}");
throw new InvalidOperationException(
$"Failed to persist settings to {_settingsFilePath}. " +
"The file may be corrupted, malformed or locked.", ex);
}
finally
{
_gate.Release();
}
}

public T? GetFromCache()
{
return _cachedSettings;
}
}

public class Settings
public interface ISettings
{
/// <summary>
/// User settings version. Increment this when the settings schema changes.
/// Gets the version of the settings schema.
/// </summary>
int Version { get; }

/// <summary>
/// FileName where the settings are stored.
/// </summary>
static abstract string SettingsFileName { get; }
}

/// <summary>
/// CoderConnect settings class that holds the settings for the CoderConnect feature.
/// </summary>
public class CoderConnectSettings : ISettings
{
/// <summary>
/// CoderConnect 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>
public int Version { get; set; }
public Dictionary<string, JsonElement> Options { get; set; }
public bool ConnectOnLaunch { get; set; }
public static string SettingsFileName { get; } = "coder-connect-settings.json";

private const int VERSION = 1; // Default version for backward compatibility
public Settings()
public CoderConnectSettings()
{
Version = VERSION;
Options = [];
ConnectOnLaunch = false;
}

public Settings(int? version, Dictionary<string, JsonElement> options)
public CoderConnectSettings(int? version, bool connectOnLogin)
{
Version = version ?? VERSION;
Options = options;
ConnectOnLaunch = connectOnLogin;
}
}

[JsonSerializable(typeof(Settings))]
[JsonSourceGenerationOptions(WriteIndented = true)]
public partial class SettingsJsonContext : JsonSerializerContext;
public CoderConnectSettings Clone()
{
return new CoderConnectSettings(Version, ConnectOnLaunch);
}


}
Loading
Loading