Refactor Application Settings and Support for Packaged/Unpackaged Mode #1924 #2014
Conversation
…centralize settings keys (#1900) ## Description - Replace `Windows.Storage.ApplicationData` with `Microsoft.Windows.Storage.ApplicationData`. - Centralize Settings Keys in a `SettingsKeys` Class. - define key constants in `ScratchPadPage`. ## Motivation and Context - Closes #1892 - Closes #1893 ## How Has This Been Tested? **Manually tested** ## Screenshots (if appropriate): ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) Co-authored-by: Niels Laute <[email protected]>
There was a problem hiding this comment.
Just a few points that I think are important, but of course after hearing @marcelwgn’s opinion about them.
WinUIGallery/Helpers/SettingsHelper/Internals/ListExtensions.cs
Outdated
Show resolved
Hide resolved
WinUIGallery/Helpers/SettingsHelper/Providers/ApplicationDataSettingsProvider.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the application settings system by replacing the legacy ApplicationData-based approach with a new SettingsHelper class that provides type-safe property access and supports both packaged and unpackaged app scenarios.
- Introduces a new SettingsHelper class with an ISettingsProvider abstraction
- Replaces string-based settings keys with strongly-typed properties
- Adds JSON-based settings storage for unpackaged apps while maintaining ApplicationData support for packaged apps
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| SettingsHelper/SettingsHelper.cs | New settings class with type-safe properties and update methods |
| SettingsHelper/Providers/* | Provider abstraction with ApplicationData and JSON implementations |
| SettingsHelper/ObservableSettings.cs | Base class providing property change notifications and storage operations |
| SettingsHelper/Internals/* | Support classes for JSON serialization and list operations |
| SettingsKeys.cs | Removed legacy string-based settings keys |
| Various pages/helpers | Updated to use new SettingsHelper.Current API instead of legacy methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| catch (Exception) | ||
| { | ||
| HandleCorruptedFile(); | ||
| } |
There was a problem hiding this comment.
[nitpick] Catching generic Exception can hide unexpected errors. Consider catching specific exceptions like JsonException or IOException for better error handling.
| catch (Exception) | |
| { | |
| HandleCorruptedFile(); | |
| } | |
| catch (JsonException) | |
| { | |
| HandleCorruptedFile(); | |
| } | |
| catch (IOException) | |
| { | |
| HandleCorruptedFile(); | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Having multiple specific exceptions can be useful for clarity, but in this case, it may not be necessary. We could handle multiple error cases with a single exception type, which would allow us to catch all errors uniformly instead of only the predefined ones.
| catch (Exception) | ||
| { | ||
| HandleCorruptedKey(key); | ||
| return default; | ||
| } |
There was a problem hiding this comment.
[nitpick] Catching generic Exception can hide unexpected errors. Consider catching specific exceptions like JsonException or InvalidCastException for better error handling.
| catch (Exception) | |
| { | |
| HandleCorruptedKey(key); | |
| return default; | |
| } | |
| catch (JsonException) | |
| { | |
| HandleCorruptedKey(key); | |
| return default; | |
| } | |
| catch (InvalidCastException) | |
| { | |
| HandleCorruptedKey(key); | |
| return default; | |
| } | |
| catch (FormatException) | |
| { | |
| HandleCorruptedKey(key); | |
| return default; | |
| } |
Copilot uses AI. Check for mistakes.
WinUIGallery/Helpers/SettingsHelper/Providers/ApplicationDataSettingsProvider.cs
Show resolved
Hide resolved
WinUIGallery/Helpers/SettingsHelper/Providers/ApplicationDataSettingsProvider.cs
Outdated
Show resolved
Hide resolved
WinUIGallery/Helpers/SettingsHelper/Providers/JsonSettingsProvider.cs
Outdated
Show resolved
Hide resolved
…ider.cs Co-authored-by: Copilot <[email protected]>
…ettingsProvider.cs Co-authored-by: Copilot <[email protected]>
This PR replaces the legacy ApplicationData-based settings system with a new SettingsHelper class. It adds support for both packaged and unpackaged app scenarios.
Closes #1924.
Old Settings System:
appData.LocalSettings.Values[SettingsKeys.IsLeftMode] = true;New Settings System:
SettingsHelper.Current.IsLeftMode = true;Note
Settings are automatically saved when changed.
SettingsHelper Architecture:
Uses an ISettingsProvider abstraction.
In packaged mode: uses ApplicationDataContainer.
In unpackaged mode: uses JsonSettingsProvider.
Adding a New Setting: