Refactor Application Settings with AppConfig and JSON Storage Support for Packaged/Unpackaged Apps #1924#2001
Refactor Application Settings with AppConfig and JSON Storage Support for Packaged/Unpackaged Apps #1924#2001ghost1372 wants to merge 5 commits intomicrosoft:mainfrom
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.
Zakariathr22
left a comment
Zakariathr22
left a comment
There was a problem hiding this comment.
Just a few suggested changes:
Also, I don't think there's any need to save MaxRecentlyVisitedSamples and MaxFavoriteSamples in the local JSON file.
| /// <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.
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.
| 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.
@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. |
Zakariathr22
left a comment
Zakariathr22
left a comment
There was a problem hiding this comment.
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. |
4 participants
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