SettingsManager that manages settings located in AppData
Settings views to manage the settings
StartupManager that allows to control registry access to enable load on startup

ibetitsmike marked this pull request as ready for review

June 2, 2025 11:14

ibetitsmike requested a review fromdeansheather

June 2, 2025 11:14

deansheather reviewed

Jun 2, 2025

View reviewed changes

App/App.xaml.cs OutdatedShow resolvedHide resolved

App/ViewModels/SettingsViewModel.cs OutdatedShow resolvedHide resolved

App/Views/Pages/SettingsMainPage.xaml.cs OutdatedShow resolvedHide resolved

Tests.App/Services/SettingsManagerTest.cs OutdatedShow resolvedHide resolved

ibetitsmike added8 commits

June 3, 2025 15:48

added new settings dialog + settings manager

7465bb5

WIP

cd99645

added StartupManager to handle auto-start

779c11b

changed order of CredentialManager loading

settings manager moved from generic to explicit settings

fcefec4

added comments

39ff83c

PR review + fmt

07ec725

created Settings class to handle versioning

c21072f

async handling of dependency load in app

bad5320

ibetitsmike force-pushed themike/57-starting-coder-desktop-on-boot branch from9037758 tobad5320Compare

June 3, 2025 13:54

ibetitsmike requested a review fromdeansheather

June 3, 2025 13:55

ibetitsmike added2 commits

June 3, 2025 17:02

fmt fix

065eda1

JsonContext improvements and usage within Settings

fc426a8

deansheather reviewed

Jun 5, 2025

View reviewed changes

App/App.xaml.cs OutdatedShow resolvedHide resolved

App/App.xaml.csShow resolvedHide resolved

App/Services/SettingsManager.cs OutdatedShow resolvedHide resolved

App/Services/StartupManager.cs

		string exe = Process.GetCurrentProcess().MainModule!.FileName;
		try
		{
		using var key = Registry.CurrentUser.OpenSubKey(RunKey, writable: true)

Copy link

Member

Choose a reason for hiding this comment

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

Is there any (easy) way we can clean this up on uninstall?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

We can, but this will also basically clear the setting every team you update (I believe you run the uninstall then too, right)?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Follow up here:#121

App/Views/Pages/SettingsMainPage.xaml OutdatedShow resolvedHide resolved

ibetitsmike added2 commits

June 6, 2025 16:25

implemented a generic settings manager

fa4fbd8

formatting

e7b2491

deansheather reviewed

Jun 6, 2025

View reviewed changes

App/Services/SettingsManager.cs Outdated

		/// </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

deansheatherJun 6, 2025

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

ContributorAuthor

ibetitsmikeJun 6, 2025

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

deansheatherJun 6, 2025

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

ContributorAuthor

ibetitsmikeJun 6, 2025

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

deansheatherJun 6, 2025

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

App/Services/SettingsManager.cs OutdatedShow resolvedHide resolved

App/ViewModels/SettingsViewModel.cs OutdatedShow resolvedHide resolved

PR adjustments

ced517e

deansheather reviewed

Jun 7, 2025

View reviewed changes

App/Services/SettingsManager.cs

Comment on lines 152 to 165

		public interface ISettings
		{
		/// <summary>
		///Gets the version ofthe settingsschema.
		///FileName wherethe settingsare stored.
		/// </summary>
		int Version { get; }
		static abstract string SettingsFileName { get; }

		/// <summary>
		///FileName wherethe settingsare stored.
		///Gets the version ofthe settingsschema.
		/// </summary>
		static abstract string SettingsFileName { get; }
		int Version { get; }

		ISettings Clone();
		}

Copy link

Member

deansheatherJun 7, 2025

Choose a reason for hiding this comment

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

You could separate this intoISettings with theVersion field, and a new interface likeICloneable<T>

publicinterfaceICloneable<T>{TClone();}

which avoids the casting required after callingClone() inRead()

App/Services/SettingsManager.cs

		// deserialize; fall back to default(T) if empty or malformed
		var result = JsonSerializer.Deserialize<T>(json)!;
		_cachedSettings = result;
		return result;

Copy link

Member

deansheatherJun 7, 2025

Choose a reason for hiding this comment

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

Needs to be cloned.

App/Services/SettingsManager.cs

		@@ -1,4 +1,5 @@
		using Google.Protobuf.WellKnownTypes;
		using Serilog;

Copy link

Member

deansheatherJun 7, 2025

Choose a reason for hiding this comment

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

I think this got added by mistake, I can't see any code that uses it

App/ViewModels/SettingsViewModel.cs

		{
		_connectSettings = settingsCache.Clone();
		}
		var settingsCache = settingsManager.Read();

Copy link

Member

deansheatherJun 7, 2025

Choose a reason for hiding this comment

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

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

Labels

None yet

2 participants

Movatterモバイル変換

added new settings dialog + settings manager#113

Are you sure you want to change the base?

added new settings dialog + settings manager#113

Uh oh!

Conversation

ibetitsmike commentedMay 29, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ibetitsmike commentedMay 29, 2025•
edited
Loading