Add Manage Callouts toggles for sound effects and delays#258
Open
MBtheOtaku wants to merge 1 commit into
Open
Conversation
Add new Manage Callouts settings: Sound Effects Delays Place the new toggles directly after Allow Callouts in the callouts section. Add and persist new settings in SettingsContext and expose them via AutoCalloutSettingsProvider: calloutSoundEffectsEnabled calloutPausesEnabled Wire callout runtime behavior to respect these settings: Disable prefix/effect sounds when Sound Effects is off. Remove inter-callout delay when Delays is off. Apply behavior updates across automatic, beacon, and intersection callout flows. Add localization keys and updated accessible hint text: Sounds that play before the spoken messages Short pauses between callouts Update storyboard fallback text to match the final localized wording.
📝 WalkthroughWalkthroughThis PR introduces two new user-configurable callout settings for the iOS app: sound effects (controlling whether callouts include a leading sound) and delays (controlling pauses between announcements). The implementation spans protocol definitions, persistent settings storage, UI controls, callout generation logic, and storyboard layouts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsVC as SettingsViewController
participant Toggle as UISwitch
participant Settings as SettingsContext
participant UDefaults as UserDefaults
participant Generator as CalloutGenerator
participant Callout as POICallout
User->>SettingsVC: Taps sound effects toggle
SettingsVC->>Toggle: Captures value change
Toggle->>SettingsVC: onSettingValueChanged
SettingsVC->>Settings: Set calloutSoundEffectsEnabled
Settings->>UDefaults: Persist to UserDefaults
UDefaults-->>Settings: ✓ Saved
Note over Generator: When generating callout...
Generator->>Settings: Read calloutSoundEffectsEnabled
Settings-->>Generator: Return setting value
Generator->>Callout: Initialize with includePrefixSound
Callout-->>Generator: Callout created
Generator-->>User: Play callout with/without prefix sound
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/ios/GuideDogs/Code/Behaviors/Default/IntersectionGenerator.swift (1)
140-156: Consider using dependency injection for settings access.Unlike
AutoCalloutGeneratorwhich receivessettings: AutoCalloutSettingsProvidervia its initializer,IntersectionGeneratordirectly accessesSettingsContext.shared. This inconsistency makes unit testing harder since you can't inject a mock settings provider.The initializer already accepts other dependencies (
geoManager,data,geocoder), so adding a settings parameter would be consistent.♻️ Suggested refactor for dependency injection
class IntersectionGenerator: AutomaticGenerator { + private unowned let settings: AutoCalloutSettingsProvider // ... existing properties ... - init(_ owner: SoundscapeBehavior, geoManager: GeolocationManagerProtocol, data: SpatialDataProtocol, geocoder: ReverseGeocoder) { + init(_ owner: SoundscapeBehavior, geoManager: GeolocationManagerProtocol, data: SpatialDataProtocol, geocoder: ReverseGeocoder, settings: AutoCalloutSettingsProvider = SettingsContext.shared) { + self.settings = settings self.owner = owner // ... }Then replace
SettingsContext.shared.calloutSoundEffectsEnabledwithsettings.calloutSoundEffectsEnabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/GuideDogs/Code/Behaviors/Default/IntersectionGenerator.swift` around lines 140 - 156, IntersectionGenerator should accept a settings provider like AutoCalloutGenerator does: add a property (e.g., settings: AutoCalloutSettingsProvider) initialized via the IntersectionGenerator initializer and replace any direct uses of SettingsContext.shared.calloutSoundEffectsEnabled in methods (notably where callout is created and where event.geocodedLocation.buildCallout(...) is called) with settings.calloutSoundEffectsEnabled; update any callers to pass the concrete settings provider (or a mock in tests) so behavior remains the same but becomes testable.apps/ios/GuideDogs/Code/App/Settings/SettingsContext.swift (1)
434-450: Consider adding notifications for consistency with other settings.Several similar settings (e.g.,
automaticCalloutsEnabled,shakeCalloutsEnabled, sense settings) post notifications when changed. The newcalloutSoundEffectsEnabledandcalloutPausesEnabledproperties don't follow this pattern.While this isn't a functional issue (generators read these values on-demand during callout generation), adding notifications would improve consistency and allow other UI components to react to changes if needed in the future.
♻️ Optional: Add notifications for new settings
// In Notification.Name extension at the top of the file: + static let calloutSoundEffectsEnabledChanged = Notification.Name("GDACalloutSoundEffectsChanged") + static let calloutPausesEnabledChanged = Notification.Name("GDACalloutPausesChanged") // In the setters: var calloutSoundEffectsEnabled: Bool { get { return userDefaults.bool(forKey: Keys.calloutSoundEffectsEnabled) } set(newValue) { userDefaults.set(newValue, forKey: Keys.calloutSoundEffectsEnabled) + NotificationCenter.default.post(name: .calloutSoundEffectsEnabledChanged, object: self, userInfo: [Keys.enabled: newValue]) } } var calloutPausesEnabled: Bool { get { return userDefaults.bool(forKey: Keys.calloutPausesEnabled) } set(newValue) { userDefaults.set(newValue, forKey: Keys.calloutPausesEnabled) + NotificationCenter.default.post(name: .calloutPausesEnabledChanged, object: self, userInfo: [Keys.enabled: newValue]) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/GuideDogs/Code/App/Settings/SettingsContext.swift` around lines 434 - 450, Add notifications in the setters for calloutSoundEffectsEnabled and calloutPausesEnabled similar to the pattern used by automaticCalloutsEnabled and shakeCalloutsEnabled: after calling userDefaults.set(...), post the same NotificationCenter notification those other settings use so observers can react to changes (use the same Notification.Name constant/pattern those properties use).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ios/GuideDogs/Code/Behaviors/Default/Callouts/DestinationCallout.swift`:
- Line 56: The initializer for DestinationCallout currently has a defaulted
parameter includePrefixSound = true allowing callers to omit it; remove the
default so the init signature becomes init(_ calloutOrigin: CalloutOrigin, _
entityKey: String, _ causedAudioDisabled: Bool = false, _ includePrefixSound:
Bool) to force callers to pass the setting explicitly, then update all call
sites (notably where PreviewGenerator constructs DestinationCallout) to pass the
correct includePrefixSound value based on the Sound Effects toggle or preview
behavior.
In
`@apps/ios/GuideDogs/Code/Behaviors/Default/Callouts/IntersectionCallout.swift`:
- Line 42: The initializer for IntersectionCallout currently has a default true
for includePrefixSound which lets callers omit the argument and bypass user
settings; change the init signature in IntersectionCallout (init(_: _:_:_:_:_))
to remove the default so includePrefixSound is a required Bool, then update the
call sites in RouteGuidanceGenerator and TourGenerator (the places using the
4-arg initializer) to pass the explicit user preference (the Sound Effects
setting) when constructing IntersectionCallout so prefix sounds respect the
user's setting.
In `@apps/ios/GuideDogs/Code/Visual` UI/Views/Settings.storyboard:
- Line 438: Two storyboard accessibility entries still use the generic label
"Streets and Places" for callout rows; find the XML elements with the
accessibilityConfiguration attribute whose label is "Streets and Places" and
replace each label with a concise, descriptive VoiceOver string that matches the
actual control purpose (e.g., "Enable street callouts" or "Enable place
callouts" or whatever the control toggles). Update both occurrences so VoiceOver
announces the specific function for those switches instead of the incorrect
generic label.
---
Nitpick comments:
In `@apps/ios/GuideDogs/Code/App/Settings/SettingsContext.swift`:
- Around line 434-450: Add notifications in the setters for
calloutSoundEffectsEnabled and calloutPausesEnabled similar to the pattern used
by automaticCalloutsEnabled and shakeCalloutsEnabled: after calling
userDefaults.set(...), post the same NotificationCenter notification those other
settings use so observers can react to changes (use the same Notification.Name
constant/pattern those properties use).
In `@apps/ios/GuideDogs/Code/Behaviors/Default/IntersectionGenerator.swift`:
- Around line 140-156: IntersectionGenerator should accept a settings provider
like AutoCalloutGenerator does: add a property (e.g., settings:
AutoCalloutSettingsProvider) initialized via the IntersectionGenerator
initializer and replace any direct uses of
SettingsContext.shared.calloutSoundEffectsEnabled in methods (notably where
callout is created and where event.geocodedLocation.buildCallout(...) is called)
with settings.calloutSoundEffectsEnabled; update any callers to pass the
concrete settings provider (or a mock in tests) so behavior remains the same but
becomes testable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a04826d-af66-4823-bc4b-f18992143891
📒 Files selected for processing (11)
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.stringsapps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swiftapps/ios/GuideDogs/Code/App/Settings/SettingsContext.swiftapps/ios/GuideDogs/Code/Behaviors/Default/AutoCalloutGenerator.swiftapps/ios/GuideDogs/Code/Behaviors/Default/BeaconCalloutGenerator.swiftapps/ios/GuideDogs/Code/Behaviors/Default/Callouts/DestinationCallout.swiftapps/ios/GuideDogs/Code/Behaviors/Default/Callouts/IntersectionCallout.swiftapps/ios/GuideDogs/Code/Behaviors/Default/IntersectionGenerator.swiftapps/ios/GuideDogs/Code/Visual UI/Controls/Settings/CalloutSettingsCellView.swiftapps/ios/GuideDogs/Code/Visual UI/View Controllers/Settings/SettingsViewController.swiftapps/ios/GuideDogs/Code/Visual UI/Views/Settings.storyboard
| } | ||
|
|
||
| init(_ calloutOrigin: CalloutOrigin, _ entityKey: String, _ causedAudioDisabled: Bool = false) { | ||
| init(_ calloutOrigin: CalloutOrigin, _ entityKey: String, _ causedAudioDisabled: Bool = false, _ includePrefixSound: Bool = true) { |
There was a problem hiding this comment.
Require explicit includePrefixSound for destination callouts.
Line 56 defaults to true, so callers can omit the new setting. PreviewGenerator.swift (Line 100) currently does that, which keeps preview prefix sounds enabled regardless of the Sound Effects toggle.
Suggested fix
- init(_ calloutOrigin: CalloutOrigin, _ entityKey: String, _ causedAudioDisabled: Bool = false, _ includePrefixSound: Bool = true) {
+ init(_ calloutOrigin: CalloutOrigin, _ entityKey: String, _ causedAudioDisabled: Bool = false, _ includePrefixSound: Bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Behaviors/Default/Callouts/DestinationCallout.swift`
at line 56, The initializer for DestinationCallout currently has a defaulted
parameter includePrefixSound = true allowing callers to omit it; remove the
default so the init signature becomes init(_ calloutOrigin: CalloutOrigin, _
entityKey: String, _ causedAudioDisabled: Bool = false, _ includePrefixSound:
Bool) to force callers to pass the setting explicitly, then update all call
sites (notably where PreviewGenerator constructs DestinationCallout) to pass the
correct includePrefixSound value based on the Sound Effects toggle or preview
behavior.
| } | ||
|
|
||
| init(_ calloutOrigin: CalloutOrigin, _ intersectionKey: String, _ isRoundabout: Bool = false, _ userHeading: CLLocationDirection) { | ||
| init(_ calloutOrigin: CalloutOrigin, _ intersectionKey: String, _ isRoundabout: Bool = false, _ userHeading: CLLocationDirection, _ includePrefixSound: Bool = true) { |
There was a problem hiding this comment.
Make includePrefixSound explicit to prevent settings bypass.
Line 42 sets a default true, which lets call sites skip passing the user preference. RouteGuidanceGenerator.swift (Line 209) and TourGenerator.swift (Line 217) still use the 4-arg initializer, so intersection prefix sounds stay enabled there even when Sound Effects is off.
Suggested fix
- init(_ calloutOrigin: CalloutOrigin, _ intersectionKey: String, _ isRoundabout: Bool = false, _ userHeading: CLLocationDirection, _ includePrefixSound: Bool = true) {
+ init(_ calloutOrigin: CalloutOrigin, _ intersectionKey: String, _ isRoundabout: Bool = false, _ userHeading: CLLocationDirection, _ includePrefixSound: Bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Behaviors/Default/Callouts/IntersectionCallout.swift`
at line 42, The initializer for IntersectionCallout currently has a default true
for includePrefixSound which lets callers omit the argument and bypass user
settings; change the init signature in IntersectionCallout (init(_: _:_:_:_:_))
to remove the default so includePrefixSound is a required Bool, then update the
call sites in RouteGuidanceGenerator and TourGenerator (the places using the
4-arg initializer) to pass the explicit user preference (the Sound Effects
setting) when constructing IntersectionCallout so prefix sounds respect the
user's setting.
| <switch opaque="NO" contentMode="scaleToFill" horizontalHuggingPriority="750" verticalHuggingPriority="750" contentHorizontalAlignment="center" contentVerticalAlignment="center" on="YES" translatesAutoresizingMaskIntoConstraints="NO" id="aCR-Yf-X9R"> | ||
| <rect key="frame" x="318" y="6" width="59" height="31"/> | ||
| <autoresizingMask key="autoresizingMask" flexibleMinX="YES" flexibleMaxY="YES"/> | ||
| <accessibility key="accessibilityConfiguration" label="Streets and Places"/> |
There was a problem hiding this comment.
Fix switch accessibility labels for the new callout rows.
Lines 438 and 486 still use "Streets and Places", which doesn’t describe these controls. This will read incorrectly in VoiceOver.
Suggested fix
- <accessibility key="accessibilityConfiguration" label="Streets and Places"/>
+ <accessibility key="accessibilityConfiguration" label="Sound Effects"/>
...
- <accessibility key="accessibilityConfiguration" label="Streets and Places"/>
+ <accessibility key="accessibilityConfiguration" label="Delays"/>Also applies to: 486-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Visual` UI/Views/Settings.storyboard at line 438, Two
storyboard accessibility entries still use the generic label "Streets and
Places" for callout rows; find the XML elements with the
accessibilityConfiguration attribute whose label is "Streets and Places" and
replace each label with a concise, descriptive VoiceOver string that matches the
actual control purpose (e.g., "Enable street callouts" or "Enable place
callouts" or whatever the control toggles). Update both occurrences so VoiceOver
announces the specific function for those switches instead of the incorrect
generic label.
Contributor
|
@MBtheOtaku I take it this replaces #247 ? Please could you close #247 if so. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add new Manage Callouts settings:
Sound Effects
Delays
Place the new toggles directly after Allow Callouts in the callouts section. Add and persist new settings in SettingsContext and expose them via AutoCalloutSettingsProvider: calloutSoundEffectsEnabled
calloutPausesEnabled
Wire callout runtime behavior to respect these settings: Disable prefix/effect sounds when Sound Effects is off. Remove inter-callout delay when Delays is off.
Apply behavior updates across automatic, beacon, and intersection callout flows. Add localization keys and updated accessible hint text: Sounds that play before the spoken messages
Short pauses between callouts
Update storyboard fallback text to match the final localized wording.
Summary by CodeRabbit