-
Notifications
You must be signed in to change notification settings - Fork 706
Refactor Application Settings and Support for Packaged/Unpackaged Mode #1924 #2014
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); | ||
| } |
Copilot
AI
Aug 30, 2025
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.
[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(); | |
| } |
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.
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; | ||
| } |
Copilot
AI
Aug 30, 2025
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.
[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; | |
| } |
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.
Like above
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]>
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.
Looks like there’s an issue when running the app unpackaged:
- Navigate to
App notificationpage - Click
Show notificationbutton
The app crashes with an unhandled exception.
The As described here, we need to register it. I tested it, and it works fine in the unpackaged scenario. So, this requires additional work and should be done in a separate PR (we can keep the current logic for packaged mode and add a registration/scenario for unpackaged mode). Here are the Microsoft docs and samples for unpackaged scenarios. |
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.
Thank you for working on this! There are some comments and one issue surrounding lists.
Previously, we used a string and comma separated the entries in there to represent that. The new code doesnt account for that which results in people losing their favorites and recently viewed as the old field (string) does not match the expected type (list of strings).
We need some conversion code for those so we can migrate users.
WinUIGallery/Helpers/SettingsHelper/Internals/ListExtensions.cs
Outdated
Show resolved
Hide resolved
WinUIGallery/Helpers/SettingsHelper/Providers/SettingsProviderFactory.cs
Outdated
Show resolved
Hide resolved
WinUIGallery/Helpers/SettingsHelper/Providers/ApplicationDataSettingsProvider.cs
Outdated
Show resolved
Hide resolved
WinUIGallery/Helpers/SettingsHelper/Providers/ApplicationDataSettingsProvider.cs
Outdated
Show resolved
Hide resolved
Renamed AddToFirst and AddToLast extension methods to AddAsFirst and AddAsLast for improved clarity and consistency.
Replaces usage of ProductNameAndVersion with ProductName
…tingsHelper to new SettingsHelper
|
Hi @marcelwgn |
|
|
||
| private const string RecentlyVisitedKey = "RecentlyVisited"; | ||
| private const string FavoritesKey = "Favorites"; | ||
| private const char delimiter = '\u001f'; |
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.
Nice, didnt know that existed!
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: