Issue 226 and 240 solved. - #259
Open
YCK03 wants to merge 19 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds two user-configurable callout settings (earcons and inter-call delays), persists them in SettingsContext with notifications, and gates callout earcon inclusion and callout delays across generators and callout composition. UI, localization, and storyboard were updated to expose the new settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as Settings UI
participant Settings as SettingsContext
participant Generators as Callout Generators
participant Callouts as Callout Implementations
participant Audio as Audio Output
User->>SettingsUI: Toggle earcon / delay
SettingsUI->>Settings: set calloutsEarconEnabled / calloutsDelayEnabled
Settings->>Settings: persist to UserDefaults
Settings->>Settings: post Notification
Generators->>Settings: query calloutsDelayEnabled
Generators->>Generators: compute calloutDelay (configured or 0.0)
Generators->>Callouts: create CalloutGroup (with delay)
Callouts->>Settings: query calloutsEarconEnabled
Callouts->>Callouts: include earcon if enabled (layer or fallback)
Callouts->>Audio: play callout (with/without earcon, with delay)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/GuideDogs/Code/Visual UI/View Controllers/Settings/SettingsViewController.swift (1)
58-65:⚠️ Potential issue | 🟠 MajorKeep
soundEffectsanddelaysvisible when automatic callouts are off.These two rows are being collapsed with the automatic-callouts children, but the settings are consumed outside automatic callouts too.
ExplorationGeneratorstill readscalloutsDelayEnabledfor manual exploration, andPreviewGeneratorstill readscalloutsEarconEnabledfor preview/manual callouts. With the current row-count/collapse logic, users who turn automatic callouts off lose access to settings that still affect their remaining callout experience.Also applies to: 94-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/GuideDogs/Code/Visual` UI/View Controllers/Settings/SettingsViewController.swift around lines 58 - 65, The collapsibleCalloutIndexPaths array currently includes CalloutsRow.soundEffects and CalloutsRow.delays which should remain visible even when automatic callouts are collapsed; remove IndexPath entries for CalloutsRow.soundEffects and CalloutsRow.delays from collapsibleCalloutIndexPaths (and from the other identical occurrence referenced around the later block) so those rows are not part of the collapse logic (keep Section.callouts and other CalloutsRow entries intact), and ensure any UI collapse/filtering code that reads collapsibleCalloutIndexPaths continues to operate without those two rows.
🤖 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/Audio/FadeableAudioPlayer.swift`:
- Line 8: Update the class comment for FadeableAudioPlayer to fix the typo
("whenver" → "whenever") and broaden the description to mention both fadeIn and
fadeOut functionality; locate the comment above the FadeableAudioPlayer
declaration (or near methods fadeIn/fadeOut) and change the text to something
like "Allows audio to fade in and out whenever needed" or equivalent concise
description that references both fadeIn and fadeOut behaviors.
In `@apps/ios/GuideDogs/Code/Behaviors/Default/AutoCalloutGenerator.swift`:
- Around line 121-123: The computed property calloutDelay currently reads
SettingsContext.shared.calloutsDelayEnabled which bypasses the injected settings
provider; change it to use the instance-level settings provider (e.g.,
settings.calloutsDelayEnabled or the appropriate injected
AutoCalloutSettingsProvider property) so calloutDelay returns 0.75 when the
injected provider indicates calloutsDelayEnabled and 0.0 otherwise; update the
calloutDelay getter to reference the injected provider instead of
SettingsContext.shared to avoid desync.
In `@apps/ios/GuideDogs/Code/Behaviors/Default/BeaconCalloutGenerator.swift`:
- Around line 65-67: The calloutDelay property in BeaconCalloutGenerator
(private var calloutDelay) changed the enabled delay to 1.00, causing a mismatch
with AutoCalloutGenerator which uses 0.75; change BeaconCalloutGenerator to use
the same default pacing (0.75) when SettingsContext.shared.calloutsDelayEnabled
is true, or better yet read a single shared constant/config value used by
AutoCalloutGenerator (e.g., a shared defaultCalloutDelay on SettingsContext) so
both generators use the same delay.
In `@apps/ios/GuideDogs/Code/Visual`
UI/Controls/Settings/CalloutSettingsCellView.swift:
- Around line 47-54: The cell configuration must fully reset all reused UI and
accessibility fields and use localization; in the CalloutSettingsCellView (the
switch/case that sets .soundEffects and .delays) ensure every case (including
.shake) assigns textLabel?.text, detailTextLabel?.text, settingSwitch.isOn and
accessibilityLabel/accessibilityValue (or clears them) so reused cells can't
keep stale values, and replace hardcoded English strings with localized
equivalents (e.g., NSLocalizedString keys matching the screen's pattern). Also
add a default/reset path that clears or sets sensible defaults for any fields
not explicitly set.
---
Outside diff comments:
In `@apps/ios/GuideDogs/Code/Visual` UI/View
Controllers/Settings/SettingsViewController.swift:
- Around line 58-65: The collapsibleCalloutIndexPaths array currently includes
CalloutsRow.soundEffects and CalloutsRow.delays which should remain visible even
when automatic callouts are collapsed; remove IndexPath entries for
CalloutsRow.soundEffects and CalloutsRow.delays from
collapsibleCalloutIndexPaths (and from the other identical occurrence referenced
around the later block) so those rows are not part of the collapse logic (keep
Section.callouts and other CalloutsRow entries intact), and ensure any UI
collapse/filtering code that reads collapsibleCalloutIndexPaths continues to
operate without those two rows.
🪄 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: 9430ad70-e976-4376-b046-dbbe6c17ced6
⛔ Files ignored due to path filters (1)
VibroGuide/VibroGuide_User_Evaluation_Survey.pdfis excluded by!**/*.pdf
📒 Files selected for processing (19)
apps/ios/GuideDogs/Code/App/Settings/SettingsContext.swiftapps/ios/GuideDogs/Code/Audio/AudioSessionManager.swiftapps/ios/GuideDogs/Code/Audio/DynamicAudioPlayer.swiftapps/ios/GuideDogs/Code/Audio/FadeableAudioPlayer.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/Callouts/POICallout.swiftapps/ios/GuideDogs/Code/Behaviors/Default/ExplorationGenerator.swiftapps/ios/GuideDogs/Code/Behaviors/Helpers/BeaconUpdateFilter.swiftapps/ios/GuideDogs/Code/Behaviors/Preview/PreviewGenerator.swiftapps/ios/GuideDogs/Code/Data/Preview/IntersectionDecisionPoint.swiftapps/ios/GuideDogs/Code/Generators/Callouts/Location Callouts/AlongRoadLocationCallout.swiftapps/ios/GuideDogs/Code/Generators/Callouts/Location Callouts/InsideLocationCallout.swiftapps/ios/GuideDogs/Code/Generators/Callouts/Location Callouts/LocationCallout.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
| // Licensed under the MIT License. | ||
| // | ||
|
|
||
| // Allows audio to fade in whenver needed. |
There was a problem hiding this comment.
Fix typo and incomplete description in comment.
The comment has two issues:
- Typo: "whenver" should be "whenever"
- The description mentions only fade in functionality, but the class also implements fade out (
fadeOutmethod)
📝 Proposed fix
-// Allows audio to fade in whenver needed.
+// Allows audio to fade in and fade out as needed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Allows audio to fade in whenver needed. | |
| // Allows audio to fade in and fade out as needed. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Audio/FadeableAudioPlayer.swift` at line 8, Update
the class comment for FadeableAudioPlayer to fix the typo ("whenver" →
"whenever") and broaden the description to mention both fadeIn and fadeOut
functionality; locate the comment above the FadeableAudioPlayer declaration (or
near methods fadeIn/fadeOut) and change the text to something like "Allows audio
to fade in and out whenever needed" or equivalent concise description that
references both fadeIn and fadeOut behaviors.
Comment on lines
+121
to
+123
| private var calloutDelay: Double { | ||
| return SettingsContext.shared.calloutsDelayEnabled ? 0.75 : 0.0 | ||
| } |
There was a problem hiding this comment.
Use the injected settings provider for calloutDelay, not the global singleton.
Line 121 currently reads from SettingsContext.shared, which bypasses settings and can desync behavior when a non-shared AutoCalloutSettingsProvider is supplied.
🔧 Proposed fix
- private var calloutDelay: Double {
- return SettingsContext.shared.calloutsDelayEnabled ? 0.75 : 0.0
- }
+ private var calloutDelay: Double {
+ return settings.calloutsDelayEnabled ? 0.75 : 0.0
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Behaviors/Default/AutoCalloutGenerator.swift` around
lines 121 - 123, The computed property calloutDelay currently reads
SettingsContext.shared.calloutsDelayEnabled which bypasses the injected settings
provider; change it to use the instance-level settings provider (e.g.,
settings.calloutsDelayEnabled or the appropriate injected
AutoCalloutSettingsProvider property) so calloutDelay returns 0.75 when the
injected provider indicates calloutsDelayEnabled and 0.0 otherwise; update the
calloutDelay getter to reference the injected provider instead of
SettingsContext.shared to avoid desync.
Comment on lines
+65
to
+67
| private var calloutDelay: Double { | ||
| return SettingsContext.shared.calloutsDelayEnabled ? 1.00 : 0.0 | ||
| } |
There was a problem hiding this comment.
Keep the beacon delay aligned with the existing default.
This toggle also changes beacon pacing from 0.75 to 1.0, while apps/ios/GuideDogs/Code/Behaviors/Default/AutoCalloutGenerator.swift:121-123 still uses 0.75. If the goal here is only to make the delay configurable, this introduces an unadvertised timing change and inconsistent cadence across generators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Behaviors/Default/BeaconCalloutGenerator.swift`
around lines 65 - 67, The calloutDelay property in BeaconCalloutGenerator
(private var calloutDelay) changed the enabled delay to 1.00, causing a mismatch
with AutoCalloutGenerator which uses 0.75; change BeaconCalloutGenerator to use
the same default pacing (0.75) when SettingsContext.shared.calloutsDelayEnabled
is true, or better yet read a single shared constant/config value used by
AutoCalloutGenerator (e.g., a shared defaultCalloutDelay on SettingsContext) so
both generators use the same delay.
Comment on lines
+47
to
+54
| case .soundEffects: | ||
| textLabel?.text = "Sound Effects" | ||
| detailTextLabel?.text = "Sounds played before callout" | ||
| settingSwitch.isOn = SettingsContext.shared.calloutsEarconEnabled | ||
| case .delays: | ||
| textLabel?.text = "Delays" | ||
| detailTextLabel?.text = "Short pauses between callouts" | ||
| settingSwitch.isOn = SettingsContext.shared.calloutsDelayEnabled |
There was a problem hiding this comment.
Fully reconfigure reused cells here.
SettingsViewController now reuses the "shakeCallouts" prototype for .shake, .soundEffects, and .delays, but only the new cases overwrite the title/subtitle. A cell previously configured as .soundEffects or .delays can be reused as .shake and keep the wrong text/accessibility unless every case resets those fields. Also, these new labels are hardcoded English, so they bypass the localization pattern used by the rest of this screen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/GuideDogs/Code/Visual`
UI/Controls/Settings/CalloutSettingsCellView.swift around lines 47 - 54, The
cell configuration must fully reset all reused UI and accessibility fields and
use localization; in the CalloutSettingsCellView (the switch/case that sets
.soundEffects and .delays) ensure every case (including .shake) assigns
textLabel?.text, detailTextLabel?.text, settingSwitch.isOn and
accessibilityLabel/accessibilityValue (or clears them) so reused cells can't
keep stale values, and replace hardcoded English strings with localized
equivalents (e.g., NSLocalizedString keys matching the screen's pattern). Also
add a default/reset path that clears or sets sensible defaults for any fields
not explicitly set.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/ios/GuideDogs/Code/Visual UI/Controls/Settings/CalloutSettingsCellView.swift (1)
45-54:⚠️ Potential issue | 🟡 MinorFully reset
.shakecell content to avoid stale reused labels.
.soundEffects/.delaysoverwrite title/subtitle, but.shakedoes not. With reused cells,.shakecan display stale text/detail from a previous type unless those fields are explicitly restored/cleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/GuideDogs/Code/Visual` UI/Controls/Settings/CalloutSettingsCellView.swift around lines 45 - 54, The .shake case in CalloutSettingsCellView is leaving textLabel/detailTextLabel unchanged, causing reused UITableViewCell instances to show stale titles/subtitles; update the .shake branch to explicitly set or clear textLabel?.text and detailTextLabel?.text (and ensure settingSwitch.isOn still reads SettingsContext.shared.shakeCalloutsEnabled) so the cell fully resets when reused.
🧹 Nitpick comments (1)
apps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swift (1)
30-31: Fix minor typo in the doc comment.
"enabled/"has a trailing slash; this should be cleaned up for clarity.Suggested patch
- /// Whether information callouts are enabled/ + /// Whether information callouts are enabled var informationSenseEnabled: Bool { get set }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swift` around lines 30 - 31, The doc comment for the property informationSenseEnabled contains a trailing slash; update the comment above var informationSenseEnabled in AutoCalloutSettingsProvider.swift to remove the extraneous "/" so it reads cleanly (e.g., "Whether information callouts are enabled").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/ios/GuideDogs/Code/Visual`
UI/Controls/Settings/CalloutSettingsCellView.swift:
- Around line 45-54: The .shake case in CalloutSettingsCellView is leaving
textLabel/detailTextLabel unchanged, causing reused UITableViewCell instances to
show stale titles/subtitles; update the .shake branch to explicitly set or clear
textLabel?.text and detailTextLabel?.text (and ensure settingSwitch.isOn still
reads SettingsContext.shared.shakeCalloutsEnabled) so the cell fully resets when
reused.
---
Nitpick comments:
In `@apps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swift`:
- Around line 30-31: The doc comment for the property informationSenseEnabled
contains a trailing slash; update the comment above var informationSenseEnabled
in AutoCalloutSettingsProvider.swift to remove the extraneous "/" so it reads
cleanly (e.g., "Whether information callouts are enabled").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55e890f7-0990-47e9-89dc-5e13f60d9c31
📒 Files selected for processing (3)
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.stringsapps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swiftapps/ios/GuideDogs/Code/Visual UI/Controls/Settings/CalloutSettingsCellView.swift
✅ Files skipped from review due to trivial changes (1)
- apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings
|
@YCK03 @MBtheOtaku quick question...re the work on tidying up the callouts experience, are you working together on this or separate from each other? Keen to avoid duplication of effort |
Contributor
Author
|
I have been working seperatly from MBtheOtaku. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/ios/GuideDogs/Code/Visual UI/Controls/Settings/CalloutSettingsCellView.swift (1)
47-54:⚠️ Potential issue | 🟡 MinorPrevent stale reused cell text when switching back to
.shake.At Line 45-54,
.soundEffects/.delaysoverwrite title/subtitle, but.shakedoes not. A reused cell can retain stale text/details when reconfigured as.shake. Please explicitly set (or reset) these fields for.shakeas well.Suggested fix
case .shake: + textLabel?.text = GDLocalizedString("callouts.shake") + detailTextLabel?.text = GDLocalizedString("callouts.shake.info") settingSwitch.isOn = SettingsContext.shared.shakeCalloutsEnabled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/GuideDogs/Code/Visual` UI/Controls/Settings/CalloutSettingsCellView.swift around lines 47 - 54, The .shake case in CalloutSettingsCellView currently doesn't set textLabel/detailTextLabel, so reused UITableViewCell instances can show stale text when reconfigured from .soundEffects or .delays; update the .shake branch in the cell configuration (where .soundEffects and .delays are handled) to explicitly set textLabel?.text and detailTextLabel?.text (and set settingSwitch.isOn to SettingsContext.shared.calloutsShakeEnabled) so all three cases (.shake, .soundEffects, .delays) fully initialize the visible fields and prevent reuse artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/ios/GuideDogs/Code/Visual`
UI/Controls/Settings/CalloutSettingsCellView.swift:
- Around line 47-54: The .shake case in CalloutSettingsCellView currently
doesn't set textLabel/detailTextLabel, so reused UITableViewCell instances can
show stale text when reconfigured from .soundEffects or .delays; update the
.shake branch in the cell configuration (where .soundEffects and .delays are
handled) to explicitly set textLabel?.text and detailTextLabel?.text (and set
settingSwitch.isOn to SettingsContext.shared.calloutsShakeEnabled) so all three
cases (.shake, .soundEffects, .delays) fully initialize the visible fields and
prevent reuse artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ae8efb6-fa98-4661-a5d4-2ae6c2230607
📒 Files selected for processing (4)
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.stringsapps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swiftapps/ios/GuideDogs/Code/Behaviors/Default/ExplorationGenerator.swiftapps/ios/GuideDogs/Code/Visual UI/Controls/Settings/CalloutSettingsCellView.swift
✅ Files skipped from review due to trivial changes (2)
- apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings
- apps/ios/GuideDogs/Code/App/Settings/AutoCalloutSettingsProvider.swift
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
3 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.
Summary by CodeRabbit
New Features
Bug Fixes
UI Updates