-
Notifications
You must be signed in to change notification settings - Fork 706
Refactor Application Settings with AppConfig and JSON Storage Support for Packaged/Unpackaged Apps #1924 #2001
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
Conversation
Replaces the previous ApplicationData-based settings management with a new AppConfig class that serializes settings to JSON files. Updates all settings access throughout the app to use the new SettingsHelper.Config object, removes SettingsKeys, and introduces extension methods for managing favorites and recently visited items. This change improves settings portability and maintainability, and prepares for future extensibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// <summary> | ||
| /// Gets or sets the maximum number of favorite samples allowed. | ||
| /// </summary> | ||
| public int MaxFavoriteSamples { get; set; } = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no maximum for the favorite samples. The old logic assumed that if the maximum was set to 0, it meant there was no limit. Since that logic has been removed, we no longer need this.
| /// <summary> | ||
| /// Represents the maximum number of recently visited samples to retain. | ||
| /// </summary> | ||
| public int MaxRecentlyVisitedSamples { get; set; } = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public int MaxRecentlyVisitedSamples { get; set; } = 7; | |
| public const int MaxRecentlyVisitedSamples { get; set; } = 7; |
This is supposed to be a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zakariathr22
If we decide to exclude the max properties from the JSON file, we should move those properties out of AppConfig.cs and into the SettingsHelper.cs class.
so this suggestion can not be commited here.
Thanks, @Zakariathr22 . I don’t have any issues with that, but let’s check with @marcelwgn as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, even the Scratchpad uses the legacy ApplicationData-based settings system, so we need to save the code in the local JSON file.
|
After internal discussion, we decided that we want to stick with the ApplicationData as the default mechanism and fall back to JSON if we are running unpackaged, sorry for the change of direction here. For this, I think we should have a common "IPersistenceProvider"/"ISettingsProvider" interface that depending on the packaging mode gets filled by an ApplicationData or JSON file based provider. I agree with @Zakariathr22 that MaxRecentlyVisitedSamples and MaxFavoriteSamples should not be persisted, those are policies and not settings. |
Okay, I think it's better to close this one and open a new PR with the requested changes. |
|
I'm closing this PR and have opened a new one with the requested changes. |
This PR replaces the legacy ApplicationData-based settings system with a new AppConfig class that serializes settings to JSON files. It introduces support for both packaged and unpackaged app scenarios.
Closes #1924.
Old Settings System:
appData.LocalSettings.Values[SettingsKeys.IsLeftMode] = true;New Settings System:
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes