-
Notifications
You must be signed in to change notification settings - Fork 5
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
Changes from 12 commits
7465bb5
cd99645
779c11b
fcefec4
39ff83c
07ec725
c21072f
bad5320
065eda1
fc426a8
fa4fbd8
e7b2491
ced517e
c4c52e2
0c7567b
2824bd8
c11f6db
043c9bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,201 @@ | ||
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; | ||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// <summary> | ||
/// Settings contract exposing properties for app settings. | ||
/// </summary> | ||
public interface ISettingsManager<T> where T : ISettings, new() | ||
{ | ||
/// <summary> | ||
/// 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. | ||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// </summary> | ||
/// <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 null if the settings are not cached or not available. | ||
/// </summary> | ||
/// <returns></returns> | ||
public T? GetFromCache(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than have this, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely going against this comment :D There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// <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() | ||
{ | ||
private readonly string _settingsFilePath; | ||
private readonly string _appName = "CoderDesktop"; | ||
private string _fileName; | ||
private readonly object _lock = new(); | ||
|
||
private T? _cachedSettings; | ||
|
||
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. | ||
/// Otherwise the settings file will be created in the user's local application data folder. | ||
/// </param> | ||
public SettingsManager(string? settingsFilePath = null) | ||
{ | ||
if (settingsFilePath is null) | ||
{ | ||
settingsFilePath = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); | ||
} | ||
else if (!Path.IsPathRooted(settingsFilePath)) | ||
{ | ||
throw new ArgumentException("settingsFilePath must be an absolute path if provided", nameof(settingsFilePath)); | ||
} | ||
|
||
var folder = Path.Combine( | ||
settingsFilePath, | ||
_appName); | ||
|
||
Directory.CreateDirectory(folder); | ||
|
||
_fileName = T.SettingsFileName; | ||
_settingsFilePath = Path.Combine(folder, _fileName); | ||
} | ||
|
||
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 | ||
{ | ||
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; | ||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
catch (OperationCanceledException) | ||
{ | ||
throw; // propagate caller-requested cancellation | ||
} | ||
catch (Exception ex) | ||
{ | ||
throw new InvalidOperationException( | ||
$"Failed to read settings from {_settingsFilePath}. " + | ||
"The file may be corrupted, malformed or locked.", ex); | ||
} | ||
finally | ||
{ | ||
_gate.Release(); | ||
} | ||
} | ||
|
||
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 | ||
{ | ||
// 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 persist settings to {_settingsFilePath}. " + | ||
"The file may be corrupted, malformed or locked.", ex); | ||
} | ||
finally | ||
{ | ||
_gate.Release(); | ||
} | ||
} | ||
|
||
public T? GetFromCache() | ||
{ | ||
return _cachedSettings; | ||
} | ||
} | ||
|
||
public interface ISettings | ||
{ | ||
/// <summary> | ||
/// Gets the version of the settings schema. | ||
/// </summary> | ||
int Version { get; } | ||
|
||
/// <summary> | ||
/// FileName where the settings are stored. | ||
/// </summary> | ||
static abstract string SettingsFileName { get; } | ||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/// <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 bool ConnectOnLaunch { get; set; } | ||
public static string SettingsFileName { get; } = "coder-connect-settings.json"; | ||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private const int VERSION = 1; // Default version for backward compatibility | ||
public CoderConnectSettings() | ||
{ | ||
Version = VERSION; | ||
ConnectOnLaunch = false; | ||
} | ||
|
||
public CoderConnectSettings(int? version, bool connectOnLogin) | ||
{ | ||
Version = version ?? VERSION; | ||
ConnectOnLaunch = connectOnLogin; | ||
} | ||
|
||
public CoderConnectSettings Clone() | ||
{ | ||
return new CoderConnectSettings(Version, ConnectOnLaunch); | ||
} | ||
|
||
|
||
ibetitsmike marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.