added new settings dialog + settings manager #113
Conversation
changed order of CredentialManager loading
ibetitsmike
force-pushed
the
mike/57-starting-coder-desktop-on-boot
branch
from
9037758 to
bad5320
Compare
| string exe = Process.GetCurrentProcess().MainModule!.FileName; | ||
| try | ||
| { | ||
| using var key = Registry.CurrentUser.OpenSubKey(RunKey, writable: true) |
There was a problem hiding this comment.
Is there any (easy) way we can clean this up on uninstall?
There was a problem hiding this comment.
We can, but this will also basically clear the setting every team you update (I believe you run the uninstall then too, right)?
There was a problem hiding this comment.
Follow up here: #121
App/Services/SettingsManager.cs
Outdated
| /// </summary> | ||
| bool ConnectOnLaunch { get; set; } | ||
| /// <returns></returns> | ||
| public T? GetFromCache(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This is definitely going against this comment :D
#113 (comment)
There was a problem hiding this comment.
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.
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.
I think that's fair. We can revisit this if it's causing issues
| public interface ISettings | ||
| { | ||
| /// <summary> | ||
| /// Gets the version of the settings schema. | ||
| /// FileName where the settings are stored. | ||
| /// </summary> | ||
| int Version { get; } | ||
| static abstract string SettingsFileName { get; } | ||
|
|
||
| /// <summary> | ||
| /// FileName where the settings are stored. | ||
| /// Gets the version of the settings schema. | ||
| /// </summary> | ||
| static abstract string SettingsFileName { get; } | ||
| int Version { get; } | ||
|
|
||
| ISettings Clone(); | ||
| } |
There was a problem hiding this comment.
You could separate this into ISettings with the Version field, and a new interface like ICloneable<T>
public interface ICloneable<T> {
T Clone();
}which avoids the casting required after calling Clone() in Read()
| // deserialize; fall back to default(T) if empty or malformed | ||
| var result = JsonSerializer.Deserialize<T>(json)!; | ||
| _cachedSettings = result; | ||
| return result; |
| @@ -1,4 +1,5 @@ | |||
| using Google.Protobuf.WellKnownTypes; | |||
| using Serilog; | |||
There was a problem hiding this comment.
I think this got added by mistake, I can't see any code that uses it
| { | ||
| _connectSettings = settingsCache.Clone(); | ||
| } | ||
| var settingsCache = settingsManager.Read(); |
There was a problem hiding this comment.
This code seems like it's not doing anything. Read is async, and _connectSettings doesn't get populated. It's not going to be easy cuz it's async but there are existing patterns in other view models with async gets in the ctor
Closes: #57 & #55
Adds: