fix(widget): use app language instead of device language for widgets#578
fix(widget): use app language instead of device language for widgets#578MaorRocky wants to merge 2 commits intobetter-rail:mainfrom
Conversation
- Add native module method to save app language to shared UserDefaults (App Group) - Create LocaleManager.swift to read language from shared UserDefaults - Apply locale and layout direction to widget views based on app language - Add localized strings for en, he, ar, ru in widget extension - Use String(localized:) for all hardcoded strings in widget views - Pass correct locale to Live Activity server API
Summary of ChangesHello @MaorRocky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an inconsistency where iOS widgets were not respecting the language chosen by the user within the application. By integrating a new locale management system and leveraging shared user defaults, widgets now correctly display content in the app's selected language, including proper handling of right-to-left layouts. This enhances the user experience by providing a consistent linguistic interface across the main app and its associated widgets. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to align the language of iOS widgets with the application's language setting, rather than the device's default. The approach of passing the language code to the native side and storing it in shared UserDefaults for the widget extensions to access is sound. However, the implementation has a few critical issues that need addressing. There's a potential crash due to a force-unwrap in RNBetterRail.swift. Additionally, WidgetEntryView.swift contains a significant amount of duplicated code and a new hardcoded string that isn't localized, which undermines the goal of this PR. I've left specific comments with suggestions to fix these issues.
ios/BetterRail/RNBetterRail.swift
Outdated
| } | ||
|
|
||
| @objc func setAppGroupUserLocale(_ languageCode: String) -> Void { | ||
| UserDefaults(suiteName: "group.il.co.better-rail")!.set(languageCode, forKey: "userLocale") |
There was a problem hiding this comment.
Force-unwrapping UserDefaults with ! can cause a crash if the app group isn't configured correctly or if the suite name is invalid. It's much safer to use optional binding (guard let) to handle this potential failure gracefully and prevent the app from crashing.
guard let userDefaults = UserDefaults(suiteName: "group.il.co.better-rail") else {
// It's good practice to log an error here to help with debugging.
print("Error: Could not access UserDefaults for app group 'group.il.co.better-rail'.")
return
}
userDefaults.set(languageCode, forKey: "userLocale")| if (widgetFamily == .systemMedium || | ||
| widgetFamily == .systemLarge) { | ||
| VStack(alignment: .leading) { | ||
| Text(String(localized: "ARRIVAL")) | ||
| .preferredFont(size: 11).fontWeight(.medium) | ||
|
|
||
| Text(entry.arrivalTime).font(.system(size: 22, weight: .bold)) | ||
| }.foregroundColor(.gray) | ||
| .padding(.leading, | ||
| widgetFamily == .systemLarge && isMediumScreen | ||
| ? 8 : 2) | ||
|
|
||
| if (widgetFamily == .systemLarge) { | ||
| VStack(alignment: .leading) { | ||
| Text(String(localized: "PLATFORM")) | ||
| .preferredFont(size: 11).fontWeight(.medium) | ||
|
|
||
| Text(String(entry.platform)).font(.system(size: 22, weight: .bold)) | ||
| } | ||
| .foregroundColor(.gray) | ||
| .padding(.leading, isMediumScreen ? CGFloat(8.0) : 4.0) | ||
|
|
||
| if (UIScreen.main.bounds.width >= 360) { | ||
| VStack(alignment: .leading) { | ||
| Text(String(localized: isMediumScreen ? "TRAIN NO." : "TRAIN")) | ||
| .preferredFont(size: 11).fontWeight(.medium) | ||
|
|
||
| Text(String(entry.trainNumber)).font(.system(size: 22, weight: .bold)) | ||
| } | ||
| .foregroundColor(.gray) | ||
| .padding(.leading, 8) | ||
| } | ||
|
|
||
| } | ||
| } | ||
| .font(.system(size: nextTrainFontSize, weight: .bold)) | ||
|
|
||
| if (widgetFamily != .systemLarge) { | ||
| Text("Platform \(String(entry.platform))・Train \(String(entry.trainNumber))") | ||
| .font(.system(size: 11.5)).fontWeight(.medium).foregroundColor(.white).opacity(/*@START_MENU_TOKEN@*/0.8/*@END_MENU_TOKEN@*/) | ||
| } | ||
|
|
||
| if (widgetFamily == .systemMedium || | ||
| widgetFamily == .systemLarge) { | ||
| VStack(alignment: .leading) { |
There was a problem hiding this comment.
This added code block almost entirely duplicates the existing code that follows it (starting at line 93 in the file). It appears the intention was to replace the old block with this new localized version, but the old one was not removed. Please remove the duplicated, unlocalized code block to avoid future maintenance issues and potential bugs.
| Text("Platform \(String(entry.platform))・Train \(String(entry.trainNumber))") | ||
| .font(.system(size: 11.5)).fontWeight(.medium).foregroundColor(.white).opacity(/*@START_MENU_TOKEN@*/0.8/*@END_MENU_TOKEN@*/) |
There was a problem hiding this comment.
This Text view contains a hardcoded string that is not localized. This will result in English text being displayed for all users, which is contrary to the goal of this pull request. Please use a localized string from your .strings files.
For example, you can add the following to your .strings files:
en.lproj/Widget.strings:
"PLATFORM_TRAIN" = "Platform %1$@・Train %2$@";
And then update the code to use it.
Text(String(format: NSLocalizedString("PLATFORM_TRAIN", comment: "Platform and train number"), String(entry.platform), String(entry.trainNumber)))
.font(.system(size: 11.5)).fontWeight(.medium).foregroundColor(.white).opacity(0.8)| let appGroupUserDefaults = UserDefaults(suiteName: "group.il.co.better-rail") | ||
|
|
||
| func getAppLanguage() -> String { | ||
| return appGroupUserDefaults?.string(forKey: "userLocale") ?? "en" | ||
| } | ||
|
|
||
| func getAppLocale() -> Locale { | ||
| let languageCode = getAppLanguage() | ||
| return Locale(identifier: languageCode) | ||
| } | ||
|
|
||
| func isAppRTL() -> Bool { | ||
| let languageCode = getAppLanguage() | ||
| return languageCode == "he" || languageCode == "ar" | ||
| } | ||
|
|
||
| func getLayoutDirection() -> LayoutDirection { | ||
| return isAppRTL() ? .rightToLeft : .leftToRight | ||
| } |
There was a problem hiding this comment.
Using global functions and a global variable for locale management can make the code harder to maintain and test. It's better to encapsulate this logic in a singleton class. This improves code structure, enhances testability, and avoids polluting the global namespace. You would then update call sites to use LocaleManager.shared.getAppLanguage() and so on.
class LocaleManager {
static let shared = LocaleManager()
private let appGroupUserDefaults: UserDefaults?
private init() {
self.appGroupUserDefaults = UserDefaults(suiteName: "group.il.co.better-rail")
}
func getAppLanguage() -> String {
return appGroupUserDefaults?.string(forKey: "userLocale") ?? "en"
}
func getAppLocale() -> Locale {
return Locale(identifier: getAppLanguage())
}
func isAppRTL() -> Bool {
let languageCode = getAppLanguage()
return languageCode == "he" || languageCode == "ar"
}
func getLayoutDirection() -> LayoutDirection {
return isAppRTL() ? .rightToLeft : .leftToRight
}
}…e, localize strings, and use singleton pattern
guytepper
left a comment
guytepper
left a comment
There was a problem hiding this comment.
Hey, thanks for the contribution!
I'm not keen on supporting this as it goes against the OS guidelines.
However, if you're willing I'm open to make it accessible via the widget settings.
What do you think?
There was a problem hiding this comment.
Why is that needed when we have the existing Localizable.strings files?
There was a problem hiding this comment.
i might be wrong, since it been 4 yrs since I touched production Swift code, but from what I remember:
Widget extension = separate bundle = needs its own localization files
The main app's Localizable.strings is inaccessible to the widget at runtime
There was a problem hiding this comment.
It is a separate target, but we are sharing the localization files between targets, so I think it should work - it already does today 😅
Can you check what happens when you remove all localization changes you've done?
yeah sure, it sounds like a good option 😄 |
2 participants
Summary
Description
This PR fixes an issue where widgets were displaying in the device's system language instead of the app's selected language.
Changes Made:
Translations
The translations for the widget strings were generated using AI.
Testing