Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to remember shuffle and repeat mode settings across app restarts. The feature is controlled by a new user preference toggle "Remember Shuffle & Repeat" in the General Settings.
Key Changes:
- Added a new
rememberPlaybackSettingsboolean property to SettingsManager that defaults to false - Modified PlayerService to persist and restore shuffle and repeat mode states when the setting is enabled
- Added a UI toggle in GeneralSettingsView to control the feature
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Core/Services/SettingsManager.swift | Added rememberPlaybackSettings property with UserDefaults persistence |
| Core/Services/Player/PlayerService.swift | Added restoration logic in init and persistence logic in toggleShuffle/cycleRepeatMode |
| Views/macOS/GeneralSettingsView.swift | Added UI toggle for the new setting with helpful tooltip |
| Tests/KasetTests/SettingsManagerTests.swift | Added test to verify default value of rememberPlaybackSettings is false |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist repeat mode to UserDefaults if setting is enabled | ||
| if SettingsManager.shared.rememberPlaybackSettings { | ||
| let modeString = switch self.repeatMode { | ||
| case .off: | ||
| "off" | ||
| case .all: | ||
| "all" | ||
| case .one: | ||
| "one" | ||
| } | ||
| UserDefaults.standard.set(modeString, forKey: Self.repeatModeKey) | ||
| } |
There was a problem hiding this comment.
The repeat mode is persisted using string values ("off", "all", "one") rather than leveraging Swift's Codable or a raw value enum. While this works, consider making RepeatMode conform to RawRepresentable with String raw values, which would provide type safety and make the serialization/deserialization more maintainable and less error-prone.
| if let savedRepeatMode = UserDefaults.standard.string(forKey: Self.repeatModeKey) { | ||
| switch savedRepeatMode { | ||
| case "all": | ||
| self.repeatMode = .all | ||
| case "one": | ||
| self.repeatMode = .one | ||
| default: | ||
| self.repeatMode = .off | ||
| } | ||
| self.logger.info("Restored repeat mode: \(String(describing: self.repeatMode))") | ||
| } |
There was a problem hiding this comment.
The restoration logic uses string comparison for repeat mode but doesn't handle invalid or unexpected values gracefully. If a corrupted or unexpected value is stored in UserDefaults, it will silently default to 'off'. Consider adding a logging statement in the default case to help diagnose issues where saved settings might not be restored correctly.
| // Restore shuffle and repeat settings if enabled in settings | ||
| if SettingsManager.shared.rememberPlaybackSettings { | ||
| if UserDefaults.standard.object(forKey: Self.shuffleEnabledKey) != nil { | ||
| self.shuffleEnabled = UserDefaults.standard.bool(forKey: Self.shuffleEnabledKey) | ||
| self.logger.info("Restored shuffle state: \(self.shuffleEnabled)") | ||
| } | ||
|
|
||
| if let savedRepeatMode = UserDefaults.standard.string(forKey: Self.repeatModeKey) { | ||
| switch savedRepeatMode { | ||
| case "all": | ||
| self.repeatMode = .all | ||
| case "one": | ||
| self.repeatMode = .one | ||
| default: | ||
| self.repeatMode = .off | ||
| } | ||
| self.logger.info("Restored repeat mode: \(String(describing: self.repeatMode))") | ||
| } | ||
| } |
There was a problem hiding this comment.
There's inconsistency between how shuffle state is restored (with a nil check using object(forKey:)) versus repeat mode (only checking if the string exists). The shuffle restoration checks if the object exists before reading the bool value, but repeat mode just checks if the string is non-nil. For consistency and to avoid potential issues with default boolean values, both should use the same pattern.
| @Test("Default rememberPlaybackSettings is false") | ||
| func defaultRememberPlaybackSettings() { | ||
| let manager = SettingsManager.shared | ||
| #expect(manager.rememberPlaybackSettings == false) | ||
| } |
There was a problem hiding this comment.
The test only verifies the default value but doesn't test the persistence behavior. Consider adding a test that verifies the setting is properly persisted to UserDefaults when changed, and can be restored after reinitialization, similar to how other settings properties are tested.
| /// Whether to remember shuffle/repeat settings across app restarts. | ||
| var rememberPlaybackSettings: Bool { | ||
| didSet { | ||
| UserDefaults.standard.set(self.rememberPlaybackSettings, forKey: Keys.rememberPlaybackSettings) | ||
| } | ||
| } |
There was a problem hiding this comment.
When toggling shuffle or repeat mode while the remember setting is disabled, the old persisted values remain in UserDefaults. If a user later re-enables the remember setting, those stale values will be restored. Consider clearing the persisted shuffle and repeat values from UserDefaults when the rememberPlaybackSettings property is set to false in the didSet observer, to prevent unexpected restoration of old settings.
Description
fixes #45
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes