feat(setting): implement comprehensive sound settings #129
Conversation
Add granular sound control allowing users to enable/disable different sound categories with a master sound toggle implementing bidirectional synchronization. Key Features: - SoundManagerService with smart caching and settings-aware playback - Individual sound controls for task completion, habit completion, timer control, timer alarm - Master-slave relationship with 4 sync rules and immediate visual feedback - Optimistic UI updates with background saving to prevent race conditions - Comprehensive localization across 21 languages with proper translation keys Technical Implementation: - CQRS pattern with MediatR for settings management - Dependency injection setup with Kiwi container - StatefulBuilder for dynamic dialog updates - Background database operations with Future.wait() for批量 updates Fixes Issue #98 (Sound Toggles) with responsive UI and reliable persistence. BREAKING CHANGE: Updated sound call sites to use SoundManagerService instead of direct AudioPlayerSoundPlayer
Summary of ChangesHello @ahmet-cetinkaya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, highly configurable sound settings feature, allowing users to precisely control audio feedback within the application. It centralizes sound management through a dedicated service, ensuring that sound playback respects user preferences and provides a responsive user experience with optimistic UI updates. The changes also include comprehensive localization for global accessibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Reviewer's GuideThis PR adds comprehensive, granular sound control by introducing a SoundManagerService abstraction with caching and per-category play methods, a new SoundSettings UI component with master-slave toggle logic and optimistic updates backed by CQRS persistence, and full localization support for the new settings across all supported languages. Sequence diagram for master-slave sound toggle logicsequenceDiagram
actor User
participant SoundSettings
participant SoundManagerService
participant SettingRepository
User->>SoundSettings: Toggle master sound switch
SoundSettings->>SoundManagerService: clearSettingsCache()
SoundSettings->>SettingRepository: Save master sound setting
SoundSettings->>SettingRepository: Save all sub-settings (enable/disable)
SoundSettings->>SoundSettings: Update UI optimistically
User->>SoundSettings: Toggle sub-setting switch
SoundSettings->>SoundManagerService: clearSettingsCache()
SoundSettings->>SettingRepository: Save sub-setting
SoundSettings->>SettingRepository: If all sub-settings disabled, save master as disabled
SoundSettings->>SoundSettings: Update UI optimistically
Entity relationship diagram for sound settings persistenceerDiagram
SETTING {
string key
string value
string valueType
}
SETTING ||--o| SOUND_SETTINGS : contains
SOUND_SETTINGS {
bool soundEnabled
bool taskCompletionSoundEnabled
bool habitCompletionSoundEnabled
bool timerControlSoundEnabled
bool timerAlarmSoundEnabled
}
Class diagram for new and updated sound management typesclassDiagram
class ISoundManagerService {
+playTaskCompletion()
+playHabitCompletion()
+playTimerControl()
+playTimerAlarm()
+playTimerTick()
+playTimerTock()
+clearSettingsCache()
}
class SoundManagerService {
-ISoundPlayer _soundPlayer
-ISettingRepository _settingRepository
-bool? _soundEnabled
-bool? _taskCompletionSoundEnabled
-bool? _habitCompletionSoundEnabled
-bool? _timerControlSoundEnabled
-bool? _timerAlarmSoundEnabled
+clearSettingsCache()
+playTaskCompletion()
+playHabitCompletion()
+playTimerControl()
+playTimerAlarm()
+playTimerTick()
+playTimerTock()
}
ISoundManagerService <|.. SoundManagerService
SoundManagerService o-- ISoundPlayer
SoundManagerService o-- ISettingRepository
Class diagram for SoundSettings UI componentclassDiagram
class SoundSettings {
+onLoaded: VoidCallback?
}
class _SoundSettingsState {
-ISettingRepository _settingRepository
-ITranslationService _translationService
-IThemeService _themeService
-Mediator _mediator
-ISoundManagerService _soundManagerService
-bool _soundEnabled
-bool _taskCompletionSoundEnabled
-bool _habitCompletionSoundEnabled
-bool _timerControlSoundEnabled
-bool _timerAlarmSoundEnabled
-bool _isLoading
+_loadSoundSettings()
+_applyMasterSlaveRulesForSubSettingChange()
+_saveSoundSettingBackground()
+_showSoundModal()
+_getSoundDescription()
}
SoundSettings o-- _SoundSettingsState
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The SoundSettings widget is quite large and combines UI, state management, and persistence logic — consider extracting the master-slave rules and switch rows into smaller widgets or helpers to improve readability and maintainability.
- In SoundManagerService, playTimerTick/playTimerTock are gated by the timerAlarm setting instead of timerControl — double-check that you’re using the correct toggle for ticking sounds.
- Your onChanged handlers call both setDialogState and setState with the same logic; consolidating these into a single update would reduce duplication and avoid potential state inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SoundSettings widget is quite large and combines UI, state management, and persistence logic — consider extracting the master-slave rules and switch rows into smaller widgets or helpers to improve readability and maintainability.
- In SoundManagerService, playTimerTick/playTimerTock are gated by the timerAlarm setting instead of timerControl — double-check that you’re using the correct toggle for ticking sounds.
- Your onChanged handlers call both setDialogState and setState with the same logic; consolidating these into a single update would reduce duplication and avoid potential state inconsistencies.
## Individual Comments
### Comment 1
<location> `src/lib/presentation/ui/features/settings/components/sound_settings.dart:219-221` </location>
<code_context>
+ }
+
+ void _showSoundModal() {
+ showDialog(
+ context: context,
+ builder: (context) => StatefulBuilder(
+ builder: (context, setDialogState) {
+ return AlertDialog(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** StatefulBuilder usage may lead to state desynchronization.
Managing state in both the dialog and parent can cause inconsistencies. Prefer handling dialog state internally and update the parent only when closing the dialog or through a callback.
</issue_to_address>
### Comment 2
<location> `src/lib/presentation/ui/shared/services/sound_manager_service.dart:78-79` </location>
<code_context>
+ }
+
+ @override
+ void playTaskCompletion() async {
+ if (await _isTaskCompletionSoundEnabled()) {
+ _soundPlayer.play(SharedSounds.done);
+ }
</code_context>
<issue_to_address>
**suggestion:** play* methods are async but not marked as Future.
Change the return type of these methods to Future<void> to ensure proper error handling and allow callers to await their completion.
Suggested implementation:
```
@override
Future<void> playTaskCompletion() async {
if (await _isTaskCompletionSoundEnabled()) {
_soundPlayer.play(SharedSounds.done);
}
```
You should apply the same change to all other `play*` methods in this class (e.g., `playHabitCompletion`, `playTimerControl`, etc.), updating their return type from `void` to `Future<void>`.
</issue_to_address>
### Comment 3
<location> `src/lib/presentation/ui/shared/services/sound_manager_service.dart:58-60` </location>
<code_context>
// Play completion sound if task was completed
if (newCompletedAt != null) {
try {
- final soundPlayer = container.resolve<ISoundPlayer>();
- soundPlayer.play(SharedSounds.done, volume: 1.0);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error handling in _getBoolSetting may mask underlying issues.
Catching all exceptions and returning a default value may conceal ongoing problems with the settings repository. Logging errors or surfacing repeated failures would improve visibility and maintainability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| showDialog( | ||
| context: context, | ||
| builder: (context) => StatefulBuilder( |
There was a problem hiding this comment.
suggestion (bug_risk): StatefulBuilder usage may lead to state desynchronization.
Managing state in both the dialog and parent can cause inconsistencies. Prefer handling dialog state internally and update the parent only when closing the dialog or through a callback.
src/lib/presentation/ui/shared/services/sound_manager_service.dart
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive implementation of granular sound settings. The new SoundManagerService provides a clean abstraction for handling sound playback, and the master-slave relationship between the sound toggles is well-thought-out. The optimistic UI updates create a very responsive user experience, and the extensive localization is a great touch.
I've identified a critical bug where the timer's ticking sounds are linked to the wrong setting toggle. Additionally, there are a few areas for improvement regarding state management, potential race conditions, and code clarity that will enhance the robustness and maintainability of this new feature. My detailed comments below address these points.
|
|
||
| @override | ||
| void playTimerTick() async { | ||
| if (await _isTimerAlarmSoundEnabled()) { |
There was a problem hiding this comment.
The timer's ticking sound (playTimerTick) is incorrectly checking if _isTimerAlarmSoundEnabled. According to the feature description and UI, ticking sounds should be part of "Timer Control" sounds, not "Timer Alarm" sounds. This means the ticking sound is currently controlled by the wrong toggle in the settings. It should check _isTimerControlSoundEnabled instead.
| if (await _isTimerAlarmSoundEnabled()) { | |
| if (await _isTimerControlSoundEnabled()) { |
|
|
||
| @override | ||
| void playTimerTock() async { | ||
| if (await _isTimerAlarmSoundEnabled()) { |
There was a problem hiding this comment.
Similar to playTimerTick, the playTimerTock method is also checking the wrong setting (_isTimerAlarmSoundEnabled). This should be governed by the "Timer Control" setting. Please change this to check _isTimerControlSoundEnabled to ensure the sound is controlled by the correct user setting.
| if (await _isTimerAlarmSoundEnabled()) { | |
| if (await _isTimerControlSoundEnabled()) { |
| Future.delayed(const Duration(milliseconds: 500), () { | ||
| if (mounted) { | ||
| _loadSoundSettings(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The use of Future.delayed to reload settings is a potential code smell. It suggests an attempt to work around a race condition but isn't a guaranteed fix. If background save operations take longer than 500ms, or if the user interacts with the switches rapidly, the UI state could become inconsistent with the database. A more robust solution would be to manage a 'saving' state to prevent concurrent modifications or to use a state management approach that handles asynchronous operations and their side effects more gracefully, ensuring the UI always reflects a consistent state.
| // Set looping for continuous alarm (needs direct sound player access) | ||
| _soundPlayer.setLoop(true); |
There was a problem hiding this comment.
This is an abstraction leak. The component uses _soundManagerService to play the alarm, but then directly accesses _soundPlayer to set looping. The SoundManagerService should encapsulate all interactions with the sound player. This direct access breaks the abstraction and couples the component to the specific sound player implementation.
To fix this, the ISoundManagerService should be extended to handle looping sounds. For example, you could add an optional loop parameter to the playTimerAlarm method or create dedicated methods like startLoopingTimerAlarm() and stopLoopingAlarm().
| onChanged: (value) { | ||
| // Optimistic UI update - update both states immediately | ||
| setDialogState(() { | ||
| _soundEnabled = value; | ||
| // Show immediate visual feedback for Rule 3 & 4 | ||
| if (value) { | ||
| _taskCompletionSoundEnabled = true; | ||
| _habitCompletionSoundEnabled = true; | ||
| _timerControlSoundEnabled = true; | ||
| _timerAlarmSoundEnabled = true; | ||
| } else { | ||
| _taskCompletionSoundEnabled = false; | ||
| _habitCompletionSoundEnabled = false; | ||
| _timerControlSoundEnabled = false; | ||
| _timerAlarmSoundEnabled = false; | ||
| } | ||
| }); | ||
| setState(() { | ||
| _soundEnabled = value; | ||
| if (value) { | ||
| _taskCompletionSoundEnabled = true; | ||
| _habitCompletionSoundEnabled = true; | ||
| _timerControlSoundEnabled = true; | ||
| _timerAlarmSoundEnabled = true; | ||
| } else { | ||
| _taskCompletionSoundEnabled = false; | ||
| _habitCompletionSoundEnabled = false; | ||
| _timerControlSoundEnabled = false; | ||
| _timerAlarmSoundEnabled = false; | ||
| } | ||
| }); | ||
| // Save in background without blocking UI | ||
| _saveSoundSettingBackground(SettingKeys.soundEnabled, value); | ||
| }, |
There was a problem hiding this comment.
The state update logic within this onChanged handler is duplicated inside the setDialogState and setState calls. This violates the Don't Repeat Yourself (DRY) principle and makes the code harder to maintain. You can refactor this by extracting the state update logic into a local function within the handler. This will reduce code duplication and improve readability.
onChanged: (value) {
// Optimistic UI update - update both states immediately
void updateState() {
_soundEnabled = value;
// Show immediate visual feedback for Rule 3 & 4
if (value) {
_taskCompletionSoundEnabled = true;
_habitCompletionSoundEnabled = true;
_timerControlSoundEnabled = true;
_timerAlarmSoundEnabled = true;
} else {
_taskCompletionSoundEnabled = false;
_habitCompletionSoundEnabled = false;
_timerControlSoundEnabled = false;
_timerAlarmSoundEnabled = false;
}
}
setDialogState(updateState);
setState(updateState);
// Save in background without blocking UI
_saveSoundSettingBackground(SettingKeys.soundEnabled, value);
},| final allOtherSubSettingsDisabled = [ | ||
| if (key != SettingKeys.taskCompletionSoundEnabled) _taskCompletionSoundEnabled else false, | ||
| if (key != SettingKeys.habitCompletionSoundEnabled) _habitCompletionSoundEnabled else false, | ||
| if (key != SettingKeys.timerControlSoundEnabled) _timerControlSoundEnabled else false, | ||
| if (key != SettingKeys.timerAlarmSoundEnabled) _timerAlarmSoundEnabled else false, | ||
| ].every((enabled) => !enabled); |
There was a problem hiding this comment.
The logic to determine if all sub-settings are disabled is overly complex and hard to read. Since the state variables (_taskCompletionSoundEnabled, etc.) are already updated via setState before this method is called, you can directly check their values. This can be simplified significantly for better readability and maintainability.
final allOtherSubSettingsDisabled = !_taskCompletionSoundEnabled &&
!_habitCompletionSoundEnabled &&
!_timerControlSoundEnabled &&
!_timerAlarmSoundEnabled;| } catch (e) { | ||
| // If there's an error accessing the setting, return the default value | ||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
The catch block in _getBoolSetting silently swallows the exception and returns a default value. While returning a default is a safe fallback, the error should be logged to help with debugging potential issues with the settings repository or data corruption. Please add logging for the exception using the Logger.
} catch (e) {
Logger.error('Failed to get setting "$key": $e');
// If there's an error accessing the setting, return the default value
return defaultValue;
}…agement - Remove direct ISoundPlayer dependency from multiple components - Update SoundManagerService to handle all sound operations - Add proper loop control and alarm management methods - Implement debounced settings saving in sound settings component - Replace individual sound player calls with centralized service calls
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-architected sound settings feature. The implementation of a SoundManagerService to centralize sound logic is a great improvement, and the use of optimistic UI updates with debounced background saving provides a responsive user experience. The master-slave relationship between sound toggles is handled robustly. My review includes a few suggestions for improvement, primarily concerning a regression in timer tick volume control and some opportunities for code refinement to enhance maintainability.
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/habits/components/habit_calendar_view.dart
Outdated
Show resolved
Hide resolved
…tation - Add volume support to SoundManagerService for timer tick/tock sounds - Remove unused oldValue parameter from _applyMasterSlaveRulesForSubSettingChange - Refactor duplicated onChanged handler logic into _handleSubSettingToggle helper method - Simplify sub-setting keys check using Set for improved readability and maintainability - Fix inconsistent indentation in habit component files (2-space standard) - Fix setDialogState parameter passing in helper method
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive sound settings feature, which is a great addition. The implementation of a SoundManagerService centralizes sound logic effectively, and the use of optimistic UI updates provides a responsive user experience. The code is generally well-structured.
I've identified a critical issue in the background saving logic for sound settings that could lead to data inconsistencies between the UI and the database. Additionally, there are a couple of medium-severity issues related to user experience and code design that could be improved. My review comments provide specific feedback and suggestions for these points.
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
…and race condition fixes - Move tickingEnabled check from timer component to SoundManagerService for centralized sound logic - Simplify background saving with _saveAllSoundSettingsWithDebounce to eliminate race conditions - Remove unnecessary _loadSoundSettings() call to prevent UI flicker during dialog close - Replace complex debouncing mechanism with simpler UI state-based saving approach - Remove _pendingChanges map and _applyMasterSlaveRulesInBackground method - Update timer component to rely on SoundManagerService for all sound playback decisions - Maintain optimistic UI updates while ensuring consistent database state
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a comprehensive and well-architected sound settings feature. The creation of a SoundManagerService to centralize sound logic is a great improvement, abstracting away the details of sound playback and settings checks. The UI for granular sound control is intuitive, and the implementation of the master-slave relationship with optimistic updates provides a very responsive user experience. The refactoring across the app to use the new service is clean, and the extensive localization is commendable. I've found a couple of issues related to performance and a minor regression in the timer's ticking sound behavior, but overall this is a high-quality contribution.
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/settings/components/sound_settings.dart
Outdated
Show resolved
Hide resolved
…ess and performance fixes - Add SingleChildScrollView to AlertDialog content to prevent overflow on small screens - Extract sub-setting keys to static const Set _subSettingKeys for better maintainability - Fix audio focus regression - ensure ticking sounds don't interrupt other audio playback - Cache tickingEnabled setting in SoundManagerService to improve performance and avoid repeated database calls - Refactor playTimerTick and playTimerTock methods to use shared _playTimerSoundWithAudioFocus helper - Update clearSettingsCache to include new _tickingEnabled cache
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that implements a comprehensive and well-architected sound settings feature. The introduction of the SoundManagerService with caching, the clean separation of concerns using an interface, and the thoughtful implementation of the master-slave relationship for sound toggles are all commendable. The optimistic UI updates provide a great user experience, and the refactoring of existing sound call sites is done cleanly and consistently. I have one minor suggestion for improving code conciseness.
Remove redundant volume null check in _playTimerSoundWithAudioFocus. The _soundPlayer.play method already handles nullable volume parameter properly, making the conditional unnecessary and improving code conciseness and maintainability.
feat(setting): implement comprehensive sound settings
Summary
This PR implements Issue #98 (Sound Toggles) with comprehensive granular sound control featuring a master-slave relationship between the general sound setting and individual sound categories.
Key Features Implemented
🎵 Sound Manager Service
ISoundManagerService🎚️ Granular Sound Controls
🔗 Master-Slave Relationship
4 Bidirectional Sync Rules:
⚡ Optimistic UI Updates
🌍 Comprehensive Localization
Technical Implementation
Architecture
UI Components
Backend Services
Test Plan
Functional Testing
UI Testing
Localization Testing
Breaking Changes
SoundManagerServiceinstead of directAudioPlayerSoundPlayerFiles Changed
New Files:
src/lib/presentation/ui/features/settings/components/sound_settings.dartsrc/lib/presentation/ui/shared/services/abstraction/i_sound_manager_service.dartsrc/lib/presentation/ui/shared/services/sound_manager_service.dartUpdated Files:
Impact
This implementation provides users with granular control over app sounds while maintaining an intuitive master-slave relationship. The optimistic UI updates ensure a responsive user experience, and the comprehensive localization support makes the feature accessible to all users regardless of language preference.
Summary by Sourcery
Implement comprehensive granular sound settings by introducing a SoundSettings UI with master and category toggles, a centralized SoundManagerService for cached and settings-aware playback, replace all direct audio player calls, and add full localization support.
New Features:
Enhancements:
Build:
Documentation: