Attempt at addressing: Announce GPS accuracy #103#216
Attempt at addressing: Announce GPS accuracy #103#216ajiteshbankulaa wants to merge 15 commits intosoundscape-community:mainfrom
Conversation
…ange from meters to feet, added machine translated temp translations
…t based on the setting
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GPS accuracy announcements: new AutomaticGenerator source and Xcode project entry; settings toggle and notification; settings UI, storyboard and localization entries; a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Lifecycle
participant Gen as GPSAccuracyAnnouncementGenerator
participant Settings as SettingsContext
participant Loc as LocationManager
participant Callout as CalloutSystem
Note over App,Gen: Initialization
App->>Gen: init(settings, motionActivity)
Gen->>Settings: subscribe to gpsAnnouncementsEnabled
Note over App,Gen: App events (launch / wake)
App->>Gen: GlyphEvent (launch / wake)
Gen->>Gen: set pending (startup/wake)
Note over Loc,Gen: Location updates drive announcements
Loc->>Gen: LocationUpdatedEvent
alt gpsAnnouncementsEnabled == false
Gen->>Gen: ignore event
else
alt pending == startup/wake
Gen->>Gen: compute accuracy (m/ft)
Gen->>Callout: enqueue initial accuracy message
Gen->>Gen: mark pending -> done
else improved accuracy detected
Gen->>Callout: enqueue improved accuracy message (once)
Gen->>Gen: mark improved announced
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 21
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/Assets/Localization/fr-CA.lproj/Localizable.strings (1)
4386-4399: Fix the GPS accuracy placeholders.These new strings mix
{value}and even%d{value}, so the runtime won’t substitute the accuracy number (and%d{value}is invalid format syntax). Align with the rest of the app by using a standard format placeholder (e.g.%@) soString(format:)can inject the formatted value.-"gps.accuracy.announce.feet" = "La précision GPS est de ±{value} pieds."; -"gps.accuracy.announce.meters" = "La précision GPS est de ±%d{value} mètres."; -"gps.accuracy.improved.feet" = "La précision GPS s’est améliorée à ±%{value} pieds."; -"gps.accuracy.improved.meters" = "La précision GPS s’est améliorée à ±{value} mètres."; +"gps.accuracy.announce.feet" = "La précision GPS est de ±%@ pieds."; +"gps.accuracy.announce.meters" = "La précision GPS est de ±%@ mètres."; +"gps.accuracy.improved.feet" = "La précision GPS s’est améliorée à ±%@ pieds."; +"gps.accuracy.improved.meters" = "La précision GPS s’est améliorée à ±%@ mètres.";
🧹 Nitpick comments (1)
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings (1)
3787-3788: Polish the settings description copy
"toggles on and off GPS Accuracy Anncouncements"reads awkwardly and contains a typo. Please rephrase to something like"Toggle GPS accuracy announcements on or off."so the setting description is clear and correctly spelled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
apps/ios/GuideDogs.xcodeproj/project.pbxproj(4 hunks)apps/ios/GuideDogs.xcodeproj/xcshareddata/xcschemes/Soundscape.xcscheme(1 hunks)apps/ios/GuideDogs/Assets/Localization/da-DK.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/de-DE.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/el-GR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings(2 hunks)apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/es-ES.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/fa-IR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/fi-FI.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/fr-CA.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/fr-FR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/it-IT.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/ja-JP.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/nb-NO.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/nl-NL.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/pt-BR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/pt-PT.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/sv-SE.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Code/App/Settings/SettingsContext.swift(4 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/SoundscapeBehavior.swift(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift(3 hunks)apps/ios/GuideDogs/Code/Visual UI/Controls/Main Menu/MenuView.swift(3 hunks)apps/ios/GuideDogs/Code/Visual UI/Controls/Main Menu/MenuViewController.swift(2 hunks)apps/ios/GuideDogs/Code/Visual UI/Controls/Settings/CalloutSettingsCellView.swift(3 hunks)apps/ios/GuideDogs/Code/Visual UI/View Controllers/Settings/SettingsViewController.swift(6 hunks)apps/ios/GuideDogs/Code/Visual UI/Views/Settings.storyboard(21 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (3)
apps/ios/GuideDogs/Code/Behaviors/Route Guidance/RouteGuidance.swift (2)
wake(269-278)sleep(256-267)apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift (1)
handle(94-203)apps/ios/GuideDogs/Code/Sensors/Geolocation/GPX Simulator/GPXSimulator.swift (1)
location(582-602)
🪛 SwiftLint (0.57.0)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
[Warning] 21-21: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 52-52: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 90-90: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 166-166: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 231-231: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
🔇 Additional comments (8)
apps/ios/GuideDogs/Assets/Localization/it-IT.lproj/Localizable.strings (1)
4384-4384: Verify machine-translated strings with native Italian speaker.The comment indicates these GPS accuracy strings are machine-translated rather than human-translated. Before release, have a native Italian speaker review these strings to ensure they sound natural and use appropriate terminology in the Italian localization context.
While the translations appear grammatically correct, professional translation review will ensure:
- Natural phrasing for Italian users
- Consistency with established GPS/navigation terminology in Italian
- Appropriate formality level matching the rest of the app
apps/ios/GuideDogs/Assets/Localization/pt-BR.lproj/Localizable.strings (3)
4397-4398: LGTM: Button accessibility strings follow conventions.The GPS accuracy check button and hint strings are properly formatted and consistent with other accessibility hints in the file (e.g., lines 634, 638, 646).
4400-4401: LGTM: Settings strings follow existing patterns.The GPS announcements settings strings are consistent with other callout settings in the file (e.g.,
settings.callouts.*keys at lines 1874-1916).
4387-4387: Please re-run the checks using glob patterns instead of the unsupported--type strings. For example:#!/bin/bash echo "=== Checking for incorrect {value} placeholder format ===" rg -n '\{value\}' -g '*.lproj/*.strings' echo "" echo "=== Inspecting GPS accuracy entries for correct %@ format ===" rg -n 'gps\.accuracy\.(announce|improved)\.(feet|meters)' -g '*.lproj/*.strings' -A1This will list any
{value}occurrences and show adjacent GPS accuracy lines to confirm they use%@.apps/ios/GuideDogs/Assets/Localization/fi-FI.lproj/Localizable.strings (1)
4386-4399: Verify machine-translated Finnish strings with a native speaker.The comment on line 4386 indicates these GPS-related strings are machine-translated. While the translations appear grammatically plausible at first glance, machine translations can produce unnatural phrasing or subtle grammatical issues that affect clarity, especially for accessibility features.
Specific concerns:
- Lines 4390-4391: The illative case usage ("jalkaan", "metriin") for expressing improvement is grammatically defensible but may sound slightly unnatural depending on context
- Compound words and technical terminology should be verified for correct Finnish conventions
- Accessibility hints (line 4396) should be reviewed for natural phrasing
Recommendation: Have a native Finnish speaker review these strings before release, or consider using a professional localization service for critical accessibility features.
apps/ios/GuideDogs/Assets/Localization/de-DE.lproj/Localizable.strings (1)
4395-4399: Incorrect placeholder format for iOS localization.The strings use
{value}as the placeholder syntax, which is not compatible with iOS.stringsfiles. iOS localization uses format specifiers like%@(for strings/objects),%d(for integers), or%f(for floats).At runtime, these strings will display literally as "±{value}" instead of the actual GPS accuracy values, breaking the functionality.
Apply this diff to fix the placeholder format (assuming numeric values):
// MARK: GPS Accuracy Announcement -"gps.accuracy.announce.feet" = "Die GPS-Genauigkeit beträgt ±{value} Fuß."; -"gps.accuracy.announce.meters" = "Die GPS-Genauigkeit beträgt ±{value} Meter."; -"gps.accuracy.improved.feet" = "Die GPS-Genauigkeit hat sich auf ±{value} Fuß verbessert."; -"gps.accuracy.improved.meters" = "Die GPS-Genauigkeit hat sich auf ±{value} Meter verbessert."; +"gps.accuracy.announce.feet" = "Die GPS-Genauigkeit beträgt ±%@ Fuß."; +"gps.accuracy.announce.meters" = "Die GPS-Genauigkeit beträgt ±%@ Meter."; +"gps.accuracy.improved.feet" = "Die GPS-Genauigkeit hat sich auf ±%@ Fuß verbessert."; +"gps.accuracy.improved.meters" = "Die GPS-Genauigkeit hat sich auf ±%@ Meter verbessert."; "gps.accuracy.unavailable" = "Die GPS-Genauigkeit ist nicht verfügbar.";Note: Verify whether the implementation passes a formatted string or a number. If it's a number, you may need
%d(integer) or%.1f(float with 1 decimal) instead of%@.Run the following script to check how other locale files handle similar placeholders and verify the correct format:
apps/ios/GuideDogs/Assets/Localization/es-ES.lproj/Localizable.strings (1)
4465-4465: Verify machine-translated GPS accuracy strings with a native Spanish speaker
Have a native Spanish speaker or professional translator review the GPS accuracy strings (line 4465) to ensure the phrasing is natural and appropriate for text-to-speech.apps/ios/GuideDogs/Assets/Localization/nb-NO.lproj/Localizable.strings (1)
4384-4396: Native speaker review needed for machine-translated GPS accuracy strings (lines 4384–4396)
Have a native Norwegian speaker verify all gps.accuracy.* and related accessibility entries for idiomatic phrasing, consistent GPS terminology, and alignment with iOS localization conventions. If unavailable before merge, file a follow-up issue to address before release.
apps/ios/GuideDogs.xcodeproj/xcshareddata/xcschemes/Soundscape.xcscheme
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/da-DK.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/de-DE.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/el-GR.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift
Outdated
Show resolved
Hide resolved
|
In addition to the comments left by the AI, I think you should change the logic for the localization for the GPS accuracy announcements. Rather than checking if the language and region is U.S. or other countries, you should check if the "Units of Measure" setting is set to Imperial or Metric. Also the GPS callout toggle switch and the accessibility hint should be rewritten to match the existing callout switches. Instead of "GPS Accuracy Announcements," the callout switch should simply say "GPS Accuracy" since a callout is understood to be an announcement. The accessibility hint reads "Toggles GPS accuracy announcements on or off." It should read something like this: "Announces GPS accuracy when the app is launched". Voiceover will already let the user know that it is a toggle switch. The period should not be part of the hint because a comma will automatically be put at the end of the hint, and Voiceover may read the hint funny if the user sets the punctuation in Voiceover to All. As for the GPS accuracy announcement button for manual checking, that may not be necessary because the GPS accuracy can already be displayed in the "Troubleshooting" section and read by voiceover. I think that Jchudge explained in issue 103 that we just need the announcement to play when the app is launched or when woken up, the user can look it up later in Settings, and the way to look it up is already in the Troubleshooting section. This means that the manual check button is redundant to the current implementation. |
|
@ajiteshbankulaa Thanks for working on this. To generate the strings for the announcements, you can use LanguageFormatter.string(from: distance, rounded: false which will automatically use the user's preference. I'm not sure about including machine generated translations in a Pr. Ideally we would have a translation service setup on weblate and the strings would be marked as machine translated for native speakers to review, see #170 . If anyone wants to look into automatic translation on weblate, let me know and I'll give you admin access. |
|
cool thanks for the feedback, I'll try and fix all these issues soon |
…hough and fix one thing
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 (3)
apps/ios/GuideDogs/Assets/Localization/fr-FR.lproj/Localizable.strings (2)
614-618: Fix unit mix‑up: “meters” string currently says “pieds.”User-facing guidance is wrong for the metric variant.
-"troubleshooting.gps_status.explanation.meters" = "Cette valeur est une estimation de la précision de Soundscape quant à votre position. Moins de ±10 pieds, c’est bien. Moins de ±20 pieds, ça va. Toute autre valeur est médiocre. Les données d’emplacement sont plus précises lorsque vous êtes à l’extérieur et non sous des structures."; +"troubleshooting.gps_status.explanation.meters" = "Cette valeur est une estimation de la précision de Soundscape quant à votre position. Moins de ±10 mètres, c’est bien. Moins de ±20 mètres, ça va. Toute autre valeur est médiocre. Les données d’emplacement sont plus précises lorsque vous êtes à l’extérieur et non sous des structures.";
3622-3626: Typo: “auidio” → “audio”.Minor copy fix.
-"first_launch.prompt.message" = "Vous êtes prêt pour votre première promenade avec Soundscape. Essayez-le maintenant en choisissant une destination proche, démarrez la balise auidio et vous l’entendrez en direction de votre destination."; +"first_launch.prompt.message" = "Vous êtes prêt pour votre première promenade avec Soundscape. Essayez-le maintenant en choisissant une destination proche, démarrez la balise audio et vous l’entendrez en direction de votre destination.";apps/ios/GuideDogs/Assets/Localization/sv-SE.lproj/Localizable.strings (1)
3392-3392: Remove the duplicate localization key.This line introduces a duplicate entry for
faq.tip.two_finger_double_tap. When a.stringsfile contains duplicate keys, the behavior is undefined—one entry silently overrides the other. Remove this duplicate to ensure predictable localization.
♻️ Duplicate comments (1)
apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift (1)
156-196: Fix template substitution mismatch on line 187.The imperial GPS accuracy code has a critical bug: the template contains
{value}but line 187 attempts to replace%@:if usesImperialUnits(localeIdentifier) { let feet = Int(acc.rounded()*3.28084) - let template = GDLocalizedString("gps.accuracy.announce.feet", "GPS accuracy is ±{value} feet.") - message = template.replacingOccurrences(of: "%@", with: "\(feet)") + let template = GDLocalizedString("gps.accuracy.announce.feet", "GPS accuracy is ±%d feet.") + message = String(format: template, feet) } else { let meters = Int(acc.rounded()) let template = GDLocalizedString("gps.accuracy.announce.meters", "GPS accuracy is ±%d meters.") message = String(format: template, meters) }This aligns the imperial path with the metric path (both use
String(format:)) and fixes the template/format specifier mismatch. Users will now hear the actual accuracy value instead of a literal%@.
🧹 Nitpick comments (4)
apps/ios/GuideDogs/Assets/Localization/fr-FR.lproj/Localizable.strings (1)
4383-4384: Optional: align with common phrasing used elsewhere (“Appuyez deux fois…”)For consistency and brevity, consider:
-"faq.tip.two_finger_double_tap" = "Appuyer sur l’écran à deux reprises avec deux doigts désactive le son de toutes les notifications actives."; +"faq.tip.two_finger_double_tap" = "Appuyez deux fois avec deux doigts sur l’écran pour désactiver le son de toutes les notifications en cours.";apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (3)
204-245: Consider using LanguageFormatter for distance formatting.The localization templates are now correct (previous review issue resolved), but the implementation still uses locale-based unit detection. Per the PR objectives, consider refactoring to use
LanguageFormatter.string(from:distance, rounded:false)so the announcements respect the user's "Units of Measure" preference in Settings → General Settings. This would simplify the code by removing the need for separate imperial/metric branches and theaccuracyValueandusesImperialUnitshelpers.Based on learnings from PR objectives.
249-275: Consider using LanguageFormatter for consistency.Same recommendation as the previous method: consider using
LanguageFormatter.string(from:distance, rounded:false)to respect the user's measurement preference and maintain consistency across the app.Based on learnings from PR objectives.
1-287: Clean up typos, unused code, and MARK formatting.Several code quality issues throughout the file:
Unused code:
- Line 21:
localeIdentifieris stored but then explicitly ignored at line 177 (_ = localeIdentifier). Remove the stored property if it's not needed.- Line 32:
var first: Bool = trueis declared but never used. Remove it.Typos in comments:
- Line 36: "threadhold" → "threshold"
- Line 51: "listenders" → "listeners", "handels" → "handles"
- Line 102: "Atuomatic" → "Automatic", "Protocal" → "Protocol"
- Line 104: The comment "(not used)" is misleading—this method is part of the
AutomaticGeneratorprotocol.- Line 109: "shoudl" → "should"
- Line 277: "appropirate" → "appropriate"
SwiftLint warnings:
The MARK comments don't follow Swift conventions. Replace:
//MARK: Dependencies→// MARK: - Dependencies//MARK: initialization→// MARK: - Initialization//MARK: Deinit→// MARK: - Deinit//MARK: Atuomatic Generator Protocal Functions needed→// MARK: - AutomaticGenerator Protocol//MARK: HELPER Methods/Functions→// MARK: - Helper MethodsAs per coding guidelines (SwiftLint).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/ios/GuideDogs/Assets/Localization/da-DK.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/de-DE.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/el-GR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings(2 hunks)apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/es-ES.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/fa-IR.lproj/Localizable.strings(0 hunks)apps/ios/GuideDogs/Assets/Localization/fi-FI.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/fr-FR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/it-IT.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/ja-JP.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/nb-NO.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/nl-NL.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/pt-BR.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/pt-PT.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Assets/Localization/sv-SE.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift(3 hunks)apps/ios/GuideDogs/Code/Visual UI/Views/Settings.storyboard(23 hunks)
💤 Files with no reviewable changes (1)
- apps/ios/GuideDogs/Assets/Localization/fa-IR.lproj/Localizable.strings
✅ Files skipped from review due to trivial changes (2)
- apps/ios/GuideDogs/Assets/Localization/ja-JP.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/nb-NO.lproj/Localizable.strings
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/ios/GuideDogs/Assets/Localization/fi-FI.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/es-ES.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/pt-BR.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/el-GR.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/de-DE.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/da-DK.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings
- apps/ios/GuideDogs/Assets/Localization/nl-NL.lproj/Localizable.strings
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (1)
apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift (2)
handle(93-200)usesImperialUnits(204-222)
🪛 SwiftLint (0.57.0)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
[Warning] 18-18: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 49-49: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 95-95: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 171-171: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 247-247: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 277-277: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
🔇 Additional comments (5)
apps/ios/GuideDogs/Assets/Localization/it-IT.lproj/Localizable.strings (1)
4382-4383: LGTM — clear, idiomatic phrasing.The new tip reads well and matches the tone used elsewhere.
apps/ios/GuideDogs/Assets/Localization/pt-PT.lproj/Localizable.strings (1)
4384-4385: Verify translation workflow aligns with project guidelines.Based on the PR comments, there's a preference for using Weblate for translations rather than committing them directly to the codebase. RDMurray specifically mentioned concerns about including machine-generated translations directly in PRs and recommended using Weblate with translations marked for review by native speakers (see issue #170).
Please confirm whether this Portuguese translation has been reviewed by a native speaker, or if it should go through the Weblate workflow instead.
Based on PR comments.
apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift (2)
12-13: LGTM!The new
AnnounceGPSAccuracyEventfollows the established pattern for user-initiated events.
77-78: LGTM!Correctly registers the new event type in the
eventTypesarray.apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (1)
52-94: LGTM!The initialization properly subscribes to app operation state and settings changes. The previous issue with the missing
.store(in: &cancellables)call has been fixed (line 72), so both subscriptions are now retained correctly.
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift
Outdated
Show resolved
Hide resolved
|
Hi @ajiteshbankulaa did you mean to close this? It's good work and almost ready to merge. |
|
I think RD Murray is right. Either reopen it, or open a brand-new pull request with a clean version. All we need to do is remove the button to announce GPS accuracy manually, and fix the automatic announcement punctuations. In this case the AI is right. The automatic announcement needs a period to separate the statements so we can hear a pause. These announcements will be called out by Soundscape, not reviewed with Voiceover. And Soundscape doesn't verbally announce punctuations in the first place. I was only referring to the callout switch in the "Manage Callouts" section where the Voiceover hint shouldn't have a period. The "Manage Callouts" hint will be reviewed by Voiceover, and the period may make the formatting look weird. Once this gets merged, I'll get to work and translate everything to Spanish. |
|
Since this is a fast action navigation app, I think we could shorten the announcement. If you think the current format is good enough, and separators like commas and periods get added so that Soundscape pauses properly, I suggest the poor accuracy announcement goes like this, "GPS accuracy is poor, about %@ meters. Move around for better accuracy." The ± sign may not be pronounced correctly by Soundscape, and the word "about" is faster to say. Now if we want to make the announcement even shorter, we could rewrite it this way, "Poor GPS accuracy, about %@ meters, move for better accuracy." This shorter, more urgent announcement suggests that the user needs to find some place else quickly. The good accuracy announcement should say "GPS accuracy is good, about %@ meters." I believe that rewriting the good accuracy message "Good GPS accuracy, about %@ meters" is more confusing, but the shorter poor accuracy message above makes the user feel like there's something wrong. |
|
Hi I just closed because I didn't mean to commit this into this yet. I am almost done so yea I'll open a new pull probably or I might just reopen. I will also do the rewriting suggestions for the +- part and the periods. |
|
I think I have another idea for the poor accuracy announcement. Since the troubleshooting says that under 10 meters or 30 feet is good, under 20 meters or 60 feet is ok, anything else is poor, we could rewrite the poor accuracy announcement to be more consistent with the troubleshooting explanation. This new version could sound like this, "Poor GPS accuracy, exceeded 20 meter limit, move for better accuracy." The imperial version would say "Poor GPS accuracy, exceeded 60 foot limit, move for better accuracy." The user probably doesn't need to know the exact number once we get over 20 meters or over 60 feet. The user could still see the exact number in "Troubleshooting" displayed on screen if they want to. |
|
Now we may need an announcement for the accuracy between 10 and 20 meters (between 30 and 60 feet) to line up with the Troubleshooting description. This would say "GPS accuracy is ok, about %@ meters." Now here's another suggestion, you may not like it though, it's just another possibility. We could forget the good, ok, poor distinction and just announce the accuracy in all cases like this, "GPS accuracy: %@ meters", and the user can learn what to do by reading the troubleshooting section. Either way is fine with me, maybe RD Murray or Jchudge could chime in. |
|
I like the idea here: "Poor GPS accuracy, exceeded 20 meter limit, move for better accuracy.". It makes sense I dought people will care for the specific number. So I did change that. I also added the GPS accuracy is ok part also. The button is removed now and also fixed all the labeling and whatnot issues form before I believe. as for the all cases annouce as: Im personaly open to anything based on what everyone thinks would be better. Another thing is I could also put another toggle or something in settings so the user can decide which way they would like it. It should be pretty simple to do so, but I don't know if it would feel janky then. But i'm overall open to whatever. |
…ations, added a inbetween for poor and good which is called ok which tells ok if between 10 and 20 meters.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (1)
210-242: Consider clarifying the threshold logic and fixing the typo.The threshold logic is functionally correct but somewhat confusing:
- Line 210 checks
(acc >= 10.0) && (acc <= 20.0)for "ok" accuracy.- Line 221 checks
acc > Self.poorAccuracyThreashold(where threshold is 10.0) for "poor" accuracy.The second condition conceptually means "accuracy worse than 20 meters" but is expressed as "> 10.0" because the first condition already handles the 10-20 range. This works but isn't immediately clear to readers.
Additionally, line 37 defines
poorAccuracyThreasholdwith a typo—should bepoorAccuracyThreshold.Consider refactoring for clarity:
// At line 37 static let goodAccuracyThreshold: CLLocationAccuracy = 10.0 static let okAccuracyThreshold: CLLocationAccuracy = 20.0 // In handleFirstGPSAnnouncement if acc <= Self.goodAccuracyThreshold { // GOOD accuracy // ... } else if acc <= Self.okAccuracyThreshold { // OK accuracy // ... } else { // POOR accuracy // ... }This makes the thresholds explicit and the logic easier to follow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings(2 hunks)apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift
- apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (3)
apps/ios/GuideDogs/Code/Behaviors/Route Guidance/RouteGuidance.swift (2)
wake(269-278)sleep(256-267)apps/ios/GuideDogs/Code/Behaviors/Default/SystemGenerator.swift (2)
respondsTo(89-91)handle(93-159)apps/ios/GuideDogs/Code/App/AppContext.swift (1)
snooze(307-319)
🔇 Additional comments (1)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings (1)
4500-4515: Fix format specifiers and remove unused strings.Several issues with this block:
Format specifier mismatch: Lines 4500, 4504, 4508, 4512 use
%@but the Swift code passes anIntvalue (lines 206, 256 of GPSAccuracyAnnouncementGenerator.swift). The fallback strings in the Swift code use%d. Use%dfor integer values to match the code and avoid formatting errors.UK spelling: Lines 4504, 4512 use "meters" instead of "metres".
Unused strings: The
.announce.feetand.announce.meterskeys (lines 4500, 4504) are not referenced anywhere in GPSAccuracyAnnouncementGenerator.swift. The.unavailablekey (line 4515) is also unused. Either remove these strings or implement the missing functionality.Apply this diff to fix the format specifiers and UK spelling for the strings that are actually used:
-"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %@ feet"; +"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %d feet"; -"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %@ meters"; +"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %d metres";For the unused strings, either remove them or clarify their intended use.
Likely an incorrect or invalid review comment.
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
| private func usesImperialUnits(_ localeIdentifier: String) -> Bool { | ||
| let locale = Locale.autoupdatingCurrent | ||
| if #available(iOS 16.0, *) { | ||
| switch locale.measurementSystem { | ||
| case .metric: | ||
| return false | ||
| case .us, .uk: | ||
| return true | ||
| default: | ||
| return !locale.usesMetricSystem | ||
| } | ||
| } else { | ||
| // Pre iOS 16 best way to check | ||
| return !locale.usesMetricSystem | ||
| } | ||
| } | ||
|
|
||
|
|
||
| private func accuracyValue(forMeters acc: CLLocationAccuracy, localeIdentifier: String) -> Int { | ||
| if usesImperialUnits(localeIdentifier) { | ||
| return Int((acc * 3.28084).rounded()) // feet | ||
| } else { | ||
| return Int(acc.rounded()) // meters | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use LanguageFormatter to respect user's Units of Measure preference.
This helper checks Locale.measurementSystem to determine whether to use imperial or metric units, but this doesn't respect the user's explicit "Units of Measure" preference in Settings. According to the PR objectives, the GPS announcements should use the app's Units of Measure setting (Imperial vs Metric), not the system locale.
Replace this approach with LanguageFormatter.string(from:distance, rounded:false), which already handles unit conversion according to the user's preference and eliminates code duplication with SystemGenerator.swift.
Based on learnings from PR objectives.
Consider refactoring handleFirstGPSAnnouncement and handleImprovedGPSAnnouncement to use LanguageFormatter directly for distance formatting:
let distance = Measurement(value: acc, unit: UnitLength.meters)
let formattedDistance = LanguageFormatter.string(from: distance, rounded: false)
// Then use formattedDistance in the announcement stringThis approach would also eliminate the need to maintain separate .feet and .meters localization keys—you could use a single key with the formatted distance string.
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (1)
171-195: Respect Units-of-Measure preferenceThis helper still sniffs
Localeto decide feet vs meters, so the GPS callouts ignore the app’s explicit Units of Measure setting and recreate logic we already removed elsewhere. Please route the accuracy values throughLanguageFormatter.string(from:distance, rounded:false)(or otherwise defer toLanguageFormatter) instead of maintaining this manual unit switch.apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings (1)
4531-4536: Fix multiple issues: thresholds, UK spelling, punctuation, and consistency.This block has several issues that need correction:
Incorrect threshold (Line 4535): States "above 10 metres" but PR objectives specify poor accuracy is ≥20 metres (matching the 60 feet threshold on line 4532).
UK spelling (Line 4535): Uses "metres" correctly here but line 4536 uses "metres" - ensure consistency.
Missing sentence separators (Lines 4532, 4535): Two sentences run together ("...feet, move..." and "...metres, move..."). A full stop is needed before "move".
Inconsistent wording (Lines 4533, 4536): The "ok" strings lack "about" which appears in the "good" variants. They also need commas for better readability.
Grammar: All strings would read better with a comma after "is good", "is ok", "is poor".
Apply this diff:
-"gps.accuracy.good.feet" = "GPS accuracy is good, about %d feet"; +"gps.accuracy.good.feet" = "GPS accuracy is good, about %d feet"; -"gps.accuracy.poor.feet" = "GPS accuracy is poor and above 30 feet, move for better accuracy"; +"gps.accuracy.poor.feet" = "GPS accuracy is poor, above 60 feet. Move for better accuracy."; -"gps.accuracy.ok.feet" = "GPS accuracy is ok %d feet"; +"gps.accuracy.ok.feet" = "GPS accuracy is ok, about %d feet"; -"gps.accuracy.good.meters" = "GPS accuracy is good, about %d metres"; +"gps.accuracy.good.meters" = "GPS accuracy is good, about %d metres"; -"gps.accuracy.poor.meters" = "GPS accuracy is poor and above 10 metres, move for better accuracy"; +"gps.accuracy.poor.meters" = "GPS accuracy is poor, above 20 metres. Move for better accuracy."; -"gps.accuracy.ok.meters" = "GPS accuracy is ok %d metres"; +"gps.accuracy.ok.meters" = "GPS accuracy is ok, about %d metres";Note: Line 4532 should also use 60 feet to match the PR's stated threshold (poor ≥20m ≈ 60ft), not 30 feet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings(2 hunks)apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (2)
apps/ios/GuideDogs/Code/Behaviors/Route Guidance/RouteGuidance.swift (2)
wake(269-278)sleep(256-267)apps/ios/GuideDogs/Code/App/AppContext.swift (1)
snooze(307-319)
🪛 SwiftLint (0.57.0)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift
[Warning] 18-18: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 49-49: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 93-93: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 169-169: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 248-248: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
[Warning] 278-278: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
|
The "ok" accuracy message is still missing the comma found in the "good" accuracy message. That should look more like this: "GPS accuracy is ok, about %d meters." I think you need to change the ranges for "good," "ok," and "poor" to match the existing explanation in the "Troubleshooting" section. "Less than +-10 meters is good. Less than +-20 meters is ok. Anything else is poor". That means that the poor message should be above 20 meters or above 60 feet. The OK range should be between 10 and 20 meters, between 30 and 60 feet. |
|
On second thought, the simple version of just announcing the GPS accuracy without the good, fair, poor scale isn't as beginner friendly. Voice Vista already announces accuracy like that, and we want Soundscape to be more beginner friendly than Voice Vista, but without sounding patronizing for experienced users. We're continuing to follow the direction from when Soundscape used to be a Microsoft App, and I think our current version looks like a Microsoft feature in spirit. Don't worry about adding a button to change the announcement style. We'll have too many buttons to overcomplicate the UI. |
Also fixed the commas and the meters to UK spelling
|
It looks like the UK message for poor GPS accuracy still says "10 metres". That needs to be updated. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings (2)
4500-4515: Critical: Placeholder type mismatch still present despite previous fix.Lines 4500, 4504, 4508, and 4512 still use
%@placeholders, but a previous review confirmed the code passesIntvalues to these strings. Lines 4531-4536 correctly use%dfor the same type of data. This inconsistency will cause incorrect formatting at runtime.Apply this diff to fix the placeholder types:
-"gps.accuracy.announce.feet" = "GPS accuracy is about %@ feet"; +"gps.accuracy.announce.feet" = "GPS accuracy is about %d feet"; -"gps.accuracy.announce.meters" = "GPS accuracy is about %@ metres"; +"gps.accuracy.announce.meters" = "GPS accuracy is about %d metres"; -"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %@ feet"; +"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %d feet"; -"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %@ metres"; +"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %d metres";
4531-4536: Add sentence separators and improve consistency.Multiple issues in the "ok" and "poor" accuracy strings:
Missing sentence separators (lines 4532, 4535): The instruction runs into the accuracy statement without proper punctuation. Add a full stop and capitalize "Move".
Inconsistent wording (lines 4533, 4536): The "ok" strings omit "about" while the "good" strings include it. Add "about" for consistency.
Grammar (lines 4532, 4535): "is poor and above" reads awkwardly; "is poor, above" is clearer.
Apply this diff:
-"gps.accuracy.poor.feet" = "GPS accuracy is poor and above 60 feet, move for better accuracy"; +"gps.accuracy.poor.feet" = "GPS accuracy is poor, above 60 feet. Move for better accuracy."; -"gps.accuracy.ok.feet" = "GPS accuracy is ok, %d feet"; +"gps.accuracy.ok.feet" = "GPS accuracy is ok, about %d feet"; -"gps.accuracy.poor.meters" = "GPS accuracy is poor and above 20 metres, move for better accuracy"; +"gps.accuracy.poor.meters" = "GPS accuracy is poor, above 20 metres. Move for better accuracy."; -"gps.accuracy.ok.meters" = "GPS accuracy is ok, %d metres"; +"gps.accuracy.ok.meters" = "GPS accuracy is ok, about %d metres";
🧹 Nitpick comments (1)
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings (1)
3757-3772: Use a preformatted distance string (%@) to fix pluralization and localizationSwitch these to accept a localized distance string (from LanguageFormatter.string(from:rounded:)) instead of
%d. This avoids “1 feet/1 meters”, respects user units/formatting, and reduces l10n risk. Also update the comments to refer to Units of Measure (Imperial/Metric), not locale.Apply:
-/* Announce GPS accuracy in feet (for en_US locale). - {NumberedPlaceholder="%d"} represents the accuracy value in feet. */ -"gps.accuracy.announce.feet" = "GPS accuracy is about %d feet"; +/* Announce GPS accuracy (Imperial units). Pass a localized distance string (e.g., "30 feet"). */ +"gps.accuracy.announce.feet" = "GPS accuracy is about %@"; -/* Announce GPS accuracy in meters (for non-US locales). - {NumberedPlaceholder="%d"} represents the accuracy value in meters. */ -"gps.accuracy.announce.meters" = "GPS accuracy is about %d meters"; +/* Announce GPS accuracy (Metric units). Pass a localized distance string (e.g., "9 meters"). */ +"gps.accuracy.announce.meters" = "GPS accuracy is about %@"; -/* Announce improved GPS accuracy in feet (for en_US locale). - {NumberedPlaceholder="%d"} represents the improved accuracy in feet. */ -"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %d feet"; +/* Announce improved GPS accuracy (Imperial). */ +"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %@"; -/* Announce improved GPS accuracy in meters (for non-US locales). - {NumberedPlaceholder="%d"} represents the improved accuracy in meters. */ -"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %d meters"; +/* Announce improved GPS accuracy (Metric). */ +"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %@";If you keep
%d, please cast to a C Int type at the callsite or use%ldfor Swift Int to avoid formatting warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings(2 hunks)apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings(1 hunks)apps/ios/GuideDogs/Code/Data/Models/JSON Parsing/OSM/GeoJsonGeometry.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/ios/GuideDogs/Code/Data/Models/JSON Parsing/OSM/GeoJsonGeometry.swift
🔇 Additional comments (1)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings (1)
4527-4528: LGTM!The settings strings follow the PR review guidance correctly: the toggle label is concise ("GPS Accuracy" without "Announcements"), and the accessibility hint is clear and lacks the trailing period, consistent with other callout toggles.
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings (1)
4500-4512: Critical: Fix placeholder types - previous fix not applied.A past review comment (marked as addressed) identified that these GPS accuracy strings use
%@placeholders but the implementation passesIntvalues toString(format:). The correct placeholder for integers is%d. This issue appears to still be present in the current code.Apply this diff to fix the placeholder types:
-"gps.accuracy.announce.feet" = "GPS accuracy is about %@ feet"; +"gps.accuracy.announce.feet" = "GPS accuracy is about %d feet"; -"gps.accuracy.announce.meters" = "GPS accuracy is about %@ metres"; +"gps.accuracy.announce.meters" = "GPS accuracy is about %d metres"; -"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %@ feet"; +"gps.accuracy.improved.feet" = "GPS accuracy has improved to about %d feet"; -"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %@ metres"; +"gps.accuracy.improved.meters" = "GPS accuracy has improved to about %d metres";apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings (1)
3780-3785: Fix inclusive threshold wording and TTS punctuation; also fix spacing in ok.feet
- Make “poor” thresholds inclusive (“… or more.”) per spec (poor ≥ 20 m / ≥ 60 ft) and split the “Move…” clause into a new sentence for clearer pauses.
- Add missing space after comma in ok.feet.
Apply this diff:
-"gps.accuracy.good.feet" = "GPS accuracy is good, about %d feet"; -"gps.accuracy.poor.feet" = "GPS accuracy is poor and above 60 feet, move for better accuracy"; -"gps.accuracy.ok.feet" = "GPS accuracy is ok,about %d feet"; +"gps.accuracy.good.feet" = "GPS accuracy is good, about %d feet"; +"gps.accuracy.poor.feet" = "GPS accuracy is poor, 60 feet or more. Move for better accuracy"; +"gps.accuracy.ok.feet" = "GPS accuracy is ok, about %d feet"; -"gps.accuracy.good.meters" = "GPS accuracy is good, about %d meters"; -"gps.accuracy.poor.meters" = "GPS accuracy is poor and above 20 meters, move for better accuracy"; -"gps.accuracy.ok.meters" = "GPS accuracy is ok, about %d meters"; +"gps.accuracy.good.meters" = "GPS accuracy is good, about %d meters"; +"gps.accuracy.poor.meters" = "GPS accuracy is poor, 20 meters or more. Move for better accuracy"; +"gps.accuracy.ok.meters" = "GPS accuracy is ok, about %d meters";Based on PR comments.
🧹 Nitpick comments (1)
apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings (1)
3776-3778: Reflect wake behavior in the settings blurbFeature triggers on launch and when the app wakes. Update info to match.
"settings.callouts.gps_announcements.title" = "GPS Accuracy"; -"settings.callouts.gps_announcements.info" = "Announces GPS accuracy when the app is launched"; +"settings.callouts.gps_announcements.info" = "Announces GPS accuracy when the app launches or wakes";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings(2 hunks)apps/ios/GuideDogs/Assets/Localization/en-US.lproj/Localizable.strings(1 hunks)
|
The messages look pretty good now. So let's move on to the imperial and metric conversion itself. I explained that there might be a way to get the number from Soundscape itself so you don't need to do the calculations. I think RD Murray left a comment to use LanguageFormatter to look up the value. The AI explained that this should shorten the code, and you don't have to repeat some code. |
|
Not sure if LanguageFormatter is better. I can change to that but I just updated to use: |
|
Thanks, I think I already put in my two cents. I'll leave it up to anybody else to look over what needs to be done. |
|
We should definitely use |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift`:
- Line 18: Update the malformed MARK comments in
GPSAccuracyAnnouncementGenerator.swift by inserting a space after the slashes so
they conform to SwiftLint's mark rule (change occurrences like "//MARK:
Dependencies" to "// MARK: Dependencies" and likewise for the other MARK
comments in this file, e.g., the ones containing "Dependencies" and the other
MARK blocks). Ensure every "//MARK:" and "//MARK: - ..." is replaced with "//
MARK:" or "// MARK: - ..." respectively so SwiftLint no longer flags them.
- Around line 36-37: Rename the misspelled static constant
poorAccuracyThreashold to poorAccuracyThreshold (type CLLocationAccuracy) and
correct the preceding comment to read something like "threshold for accuracy
(meters)"; update every usage/reference of poorAccuracyThreashold to the new
poorAccuracyThreshold to keep callers compiling and maintain the same value
(20.0).
- Around line 231-235: The localized template uses a %d int placeholder but you
pass a String from LanguageFormatter.string(from:), which is undefined; update
the GDLocalizedString usage in GPSAccuracyAnnouncementGenerator (tmpl) to use %@
instead of %d (or alternatively pass the numeric value directly), then format
with String(format: tmpl, value) so the %@ receives the String returned by
LanguageFormatter.string(from:); ensure references: value, tmpl,
LanguageFormatter.string(from:), and GDLocalizedString("gps.accuracy.improved")
are updated accordingly.
🧹 Nitpick comments (2)
apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift (2)
21-21: Remove unused properties.
localeIdentifier(Line 21) andfirst(Line 32) are declared but never referenced anywhere in this file. Dead code adds confusion.Proposed fix
- private let localeIdentifier: String = Locale.current.identifier- var first: Bool = trueAlso applies to: 30-32
94-98: Nit: explicit cancellation indeinitis redundant.
AnyCancellableautomatically cancels on deallocation, so theforEach { $0.cancel() }loop is unnecessary. Harmless, but you could simplify to justdeinit { }or remove it entirely.
|
|
||
| final class GPSAccuracyAnnouncementGenerator: AutomaticGenerator { | ||
|
|
||
| //MARK: Dependencies |
There was a problem hiding this comment.
Fix MARK comment formatting (SwiftLint mark rule).
Multiple MARK comments lack the required space after //. SwiftLint expects // MARK: ... or // MARK: - .... Affected lines: 18, 49, 93, 100, 222.
Example fix:
- //MARK: Dependencies
+ // MARK: - DependenciesAlso applies to: 49-49, 93-93, 100-100, 222-222
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 18-18: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
🤖 Prompt for AI Agents
In
`@apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift`
at line 18, Update the malformed MARK comments in
GPSAccuracyAnnouncementGenerator.swift by inserting a space after the slashes so
they conform to SwiftLint's mark rule (change occurrences like "//MARK:
Dependencies" to "// MARK: Dependencies" and likewise for the other MARK
comments in this file, e.g., the ones containing "Dependencies" and the other
MARK blocks). Ensure every "//MARK:" and "//MARK: - ..." is replaced with "//
MARK:" or "// MARK: - ..." respectively so SwiftLint no longer flags them.
| //threadhold for acuracy (meters) | ||
| static let poorAccuracyThreashold: CLLocationAccuracy = 20.0 |
There was a problem hiding this comment.
Fix typos in constant name and comment.
poorAccuracyThreashold → poorAccuracyThreshold, and the comment has "threadhold" and "acuracy". Since this is a static let visible within the module, the misspelling will propagate to any future callers.
Proposed fix
- //threadhold for acuracy (meters)
- static let poorAccuracyThreashold: CLLocationAccuracy = 20.0
+ // Threshold for accuracy (meters)
+ static let poorAccuracyThreshold: CLLocationAccuracy = 20.0🤖 Prompt for AI Agents
In
`@apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift`
around lines 36 - 37, Rename the misspelled static constant
poorAccuracyThreashold to poorAccuracyThreshold (type CLLocationAccuracy) and
correct the preceding comment to read something like "threshold for accuracy
(meters)"; update every usage/reference of poorAccuracyThreashold to the new
poorAccuracyThreshold to keep callers compiling and maintain the same value
(20.0).
| let value = LanguageFormatter.string(from: acc) | ||
| let message: String | ||
| let tmpl = GDLocalizedString("gps.accuracy.improved","GPS accuracy has improved to about %d feet.") | ||
|
|
||
| message = String(format: tmpl, value) |
There was a problem hiding this comment.
%d format specifier with a String argument — will crash or produce garbage.
Line 231: value is a String from LanguageFormatter.string(from:).
Line 233: The fallback uses %d (expects Int).
Line 235: String(format: tmpl, value) passes a String into a %d slot — this is undefined behavior.
Fix: change the localized string placeholder to %@, or pass the numeric value directly. Since LanguageFormatter already appends the unit, a single %@ placeholder is sufficient and the separate .feet / .meters keys from before are no longer needed.
Proposed fix
let value = LanguageFormatter.string(from: acc)
let message: String
- let tmpl = GDLocalizedString("gps.accuracy.improved","GPS accuracy has improved to about %d feet.")
-
- message = String(format: tmpl, value)
+ let tmpl = GDLocalizedString("gps.accuracy.improved",
+ "GPS accuracy has improved to about %@.")
+ message = String(format: tmpl, value)🤖 Prompt for AI Agents
In
`@apps/ios/GuideDogs/Code/Behaviors/Default/GPSAccuracyAnnouncementGenerator.swift`
around lines 231 - 235, The localized template uses a %d int placeholder but you
pass a String from LanguageFormatter.string(from:), which is undefined; update
the GDLocalizedString usage in GPSAccuracyAnnouncementGenerator (tmpl) to use %@
instead of %d (or alternatively pass the numeric value directly), then format
with String(format: tmpl, value) so the %@ receives the String returned by
LanguageFormatter.string(from:); ensure references: value, tmpl,
LanguageFormatter.string(from:), and GDLocalizedString("gps.accuracy.improved")
are updated accordingly.
|
Reading the code back, it looks like it may need some serious cleaning. I don't understand the logic for the feet vs meters message the way this current pull request is written. I checked Weblate, and currently there is no string that just says "feet" or "meters" all by itself. Are you trying to say that the word "feet" is part of the "%@" value or not? If it is part of the value, we may need to add new strings to the localization that just say "meters" or "feet" or maybe "%@ meters" and "%@ feet" so that they could be translated. If not, the word "meters" or "feet" should be part of the message itself and not part of the value "%@", and we still need separate messages for the units of measure. |
|
Or maybe it could be a good idea to add threshold strings to the localization for both meters and feet since thresholds don't change. Possibly RD Murray may have a better solution. |
|
@JJGatchalian it's okay, the localisation strings used for @ajiteshbankulaa The app already comprehensibly handles formatting of distances. Locale specifics don't belong here at all. I would say it should be
|
|
@JJGatchalian All distances are meters internally, so it's fine to define the threshold in meters. The languageFormatter takes a distance in meters and rounds it after converting the the user's preferred units. |
|
But what about the thresholds, which are at a fixed distance, 20 meters or 60 feet? Should we hard code them into the poor accuracy messages? For example, "GPS accuracy is poor and above 60 feet. Move for better accuracy." |
|
|
||
| /* */ | ||
| "faq.tip.two_finger_double_tap" = "Hvis du trykker på skærmen to gange med to fingre, lukkes der for alle aktive lydbeskeder."; | ||
|
|
There was a problem hiding this comment.
Nit: unrelated change. I guess it was probably Xcodes fault but we should remove these extra blank lines from strings files to keep the pull request clean. if xcode doesn't want to remove the blank line you can use git:
git restore --source=main --staged --worktree -- apps/ios/GuideDogs/Assets/Localization/da-DK.lproj/Localizable.stringsdo the same for any other files that have unwanted changes then commit.
| "mail.default" = "Standard"; | ||
| "mail.gmail" = "Gmail"; | ||
| "mail.msoutlook" = "Microsoft Outlook"; | ||
|
|
| @@ -4490,3 +4491,40 @@ | |||
| "troubleshooting.user_data" = "User Data"; | |||
| "troubleshooting.user_data.button" = "Delete All Markers and Routes"; | |||
| "troubleshooting.user_data.explanation" = "Delete all map data, including saved markers and routes. This action cannot be undone."; | |||
|
|
|||
|
|
|||
| // MARK: GPS Accuracy Announcement | |||
|
|
|||
| /* Announce GPS accuracy in feet (for en_US locale). | |||
| {NumberedPlaceholder="%d"} represents the accuracy value in feet. */ | |||
| "gps.accuracy.announce.feet" = "GPS accuracy is about %@ feet"; | |||
|
|
|||
| /* Announce GPS accuracy in meters (for non-US locales). | |||
| {NumberedPlaceholder="%d"} represents the accuracy value in meters. */ | |||
| "gps.accuracy.announce.meters" = "GPS accuracy is about %@ metres"; | |||
|
|
|||
| /* Announce improved GPS accuracy in feet (for en_US locale). | |||
| {NumberedPlaceholder="%d"} represents the improved accuracy in feet. */ | |||
| "gps.accuracy.improved.feet" = "GPS accuracy has improved to about %@ feet"; | |||
|
|
|||
| /* Announce improved GPS accuracy in meters (for non-US locales). | |||
| {NumberedPlaceholder="%d"} represents the improved accuracy in meters. */ | |||
| "gps.accuracy.improved.meters" = "GPS accuracy has improved to about %@ metres"; | |||
|
|
|||
| /* Announce when GPS accuracy information is unavailable. */ | |||
| "gps.accuracy.unavailable" = "GPS accuracy is unavailable."; | |||
|
|
|||
| // MARK: GPS Accuracy Button Accessibility | |||
|
|
|||
| "settings.callouts.gps_announcements.title" = "GPS Accuracy"; | |||
| "settings.callouts.gps_announcements.info" = "Announces GPS accuracy when the app is launched"; | |||
|
|
|||
| /* Announcements: poor / good (first announcement flow) */ | |||
| "gps.accuracy.good.feet" = "GPS accuracy is good, about %d feet"; | |||
| "gps.accuracy.poor.feet" = "GPS accuracy is poor and above 60 feet, move for better accuracy"; | |||
| "gps.accuracy.ok.feet" = "GPS accuracy is ok, about %d feet"; | |||
| "gps.accuracy.good.meters" = "GPS accuracy is good, about %d metres"; | |||
| "gps.accuracy.poor.meters" = "GPS accuracy is poor and above 20 metres, move for better accuracy"; | |||
| "gps.accuracy.ok.meters" = "GPS accuracy is ok, about %d metres"; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Please make these generic and let LanguageFormatter handle the units
There was a problem hiding this comment.
Same as above, change unit specific strings to generic ones.
There was a problem hiding this comment.
Nit: spurious unrelated whitespace change. remove changes to this file with above git command.
There was a problem hiding this comment.
I'm pretty sure this table is managed entirely by the ViewController. Are the storyboard changes really necessary? We do have a medium term goal of getting rid of all Storyboards and replacing them with UIKit or swifUI.
|
@JJGatchalian good point, those should also use `LanguageFormatter.string(from:) to format the threshold. That will result in 10 meters or 30 feet when the parameter is 10.0, etc. |
|
So you're saying that we should still use language formatter, but the distance should be capped at 20 meters or 60 feet for poor accuracy. I'm just making sure that it lines up with the Troubleshooting messages "+-10 meters is a good distance, +- 20 meters is ok, anything else is poor." I don't want Soundscape to accidentaly say something like "GPS accuracy is poor and above 35 meters..." |
|
I'll do a final review of higher level stuff like content of messages etc once the code is in good shape. Then we can discuss threshold values. |
|
Hello,
So I Updated code to use LanguageFormatter.string(from:) so the inserted value is already like “10 meters” or “30 feet”. Updated localization placeholders to %@ since we’re inserting a String (not %d).
So the “poor accuracy above …” message now formats the threshold (20m → “20 meters” / “60 feet”) using LanguageFormatter, so it stays consistent and never says something wrong. Clean PR / whitespace: Do tell if there is any other issues or errors you can find and ill fix as soon as I can. |
|
I'm currently working on a Weblate translation update that reverts a lot of unnecessary changes to the Spanish translation for the most part. Sometimes we push things a little too far or translate too closely to the English so I'm going back to the Microsoft text and changing back some of the wording to the original. I still need to check the Weblate changes tonight so it's not done yet. I'm just curious. After this patch gets cleaned up, which patch should get merged first? Both patches have to do with localization to some degree. So will the safer option be to merge the Weblate pull request or this one first? Thanks. |
|
Is this pushing it a bit too far? Since 10 meters and 30 feet is not exactly the same, do you think it would be alright to hard code separate thresholds for imperial like 9 meters and 18 meters for the thresholds since they are a bit more accurate to the troubleshooting? I don't want to revise the troubleshooting saying about 33 feet is good, about 65 feet is ok...; it makes it harder for a beginner to relate. |
|
I was going to get to the issue of good vs okay distance once the other problems were addressed, since imho 20m accuracy is far from okay; it's almost worse than no signal at all since it will provide wrong information. I think it's better to assume the user is intelligent and just provide the raw info, rounded reasonably. |
|
@ajiteshbankulaa looks like there are still unit-specific strings in localisations. Sorry I haven't looked through the whole diff, but please check and make sure all changes are necessary. The files changed tab here on the pr can be useful, or you can add .diff to the end of the address bar to get a raw diff. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8911 🏁 Script executed: Length of output: 4621 🏁 Script executed: Length of output: 2307
❌ Major Issue: en-GB still has unit-specific stringsThe Keys to DELETE from en-GB: Keys to ADD to en-GB (matching en-US pattern): Note: Use
|
|
@ajiteshbankulaa some spurious localization changes in the diff may be because this is now behind main. I suggest rebasing followed by a cleanup. |
3 participants
Summary
In reference to soundscape-community/soundscape#103, I’ve implemented a new behavior that improves user awareness of GPS accuracy during startup and wake events.
Implementation Details
Startup/Wake Behavior:
On app startup or wake, the system now checks the GPS accuracy.
If the accuracy is better than the defined threshold (±10 meters), the app will announce that GPS accuracy is good.
If the accuracy is poor, the app will inform the user and suggest moving around to improve signal quality.
Once the accuracy improves beyond the threshold, the app announces that the GPS accuracy has improved.
User Preference Setting:
A new setting has been added to allow users to enable or disable GPS accuracy announcements.
By default, this setting is on false to prevent extra notification anncouncments from the GPS Accuracy feature for those who dont want.
But for users who prefer the GPS Acc. updates they can though enable this option to hear accuracy feedback on startup or wake.
Manual Accuracy Check Button:
A new button allows users to manually check the current GPS accuracy at any time.
This gives users more control while minimizing potential over-announcements.
Ovreall this aims to adress issue #103 by improve accessibility for users who want GPS feedback, while being flexibile for those who prefer a quieter experience and dont care for it being alwasy on startup.
Any feedback or suggestions for improvement would be greatly appreciated.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes