feat: update localization handling and improve UI responsiveness#378
feat: update localization handling and improve UI responsiveness#378serhii-londar wants to merge 3 commits into#367from
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors localization handling in the Crowdin iOS SDK and improves UI responsiveness in the example app. The main changes include deprecating the currentLocalization property setter in favor of setCurrentLocalization(_:completion:), adding async completion handlers for localization changes, and enhancing the example app's UI to provide loading feedback during localization updates.
Changes:
- Deprecated the
currentLocalizationproperty setter and introducedsetCurrentLocalization(_:completion:)as the preferred API for changing localization with async completion support - Refactored localization lookup to use
Localization.currentLocalizationdirectly instead of accessing throughCrowdinSDK.currentLocalization - Enhanced the example app's Settings UI with loading indicators, disabled state during localization changes, and animated UI reloading
- Improved MainVC layout with dynamic footer inset calculation and search bar repositioned as table header view
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/CrowdinSDK/CrowdinSDK/CrowdinSDK.swift | Deprecated currentLocalization setter, added setCurrentLocalization(_:completion:) and deprecated setLocalization(_:completion:) alias |
| Sources/CrowdinSDK/Providers/Crowdin/Extensions/CrowdinSDK+CrowdinProvider.swift | Updated to use Localization.currentLocalization instead of CrowdinSDK.currentLocalization |
| Sources/CrowdinSDK/Providers/Crowdin/CrowdinRemoteLocalizationStorage.swift | Modified localization detection logic with fallback chain |
| Sources/CrowdinSDK/Features/RealtimeUpdateFeature/RealtimeUpdateFeature.swift | Updated localization property with extended fallback chain |
| Example/AppleRemindersUITests/AppleRemindersUITestsCrowdinScreenhsotTests.swift | Updated to use new setCurrentLocalization API |
| Example/AppleReminders/SceneDelegate.swift | Refactored UI setup into reusable method with animation support and added reloadLocalizedUI() |
| Example/AppleReminders/Controllers/SettingsVC.swift | Added loading indicator, error handling, and async completion support for localization changes |
| Example/AppleReminders/Controllers/MainVC.swift | Changed search bar to table header view, added dynamic footer inset calculation, and improved layout |
Sources/CrowdinSDK/Features/RealtimeUpdateFeature/RealtimeUpdateFeature.swift
Outdated
Show resolved
Hide resolved
| /// - Parameters: | ||
| /// - localization: Localization code to use. If `nil`, localization will be auto-detected. | ||
| /// - completion: Completion handler called when localization refresh finishes. | ||
| @available(*, deprecated, message: "Please use setCurrentLocalization(_:completion:) instead.") |
There was a problem hiding this comment.
The new setLocalization method is immediately deprecated with a message pointing to setCurrentLocalization(_:completion:). Since this is a new addition, consider not adding it at all or removing the deprecation marker. Adding a method as deprecated doesn't provide value to users.
| @available(*, deprecated, message: "Please use setCurrentLocalization(_:completion:) instead.") |
| private func setupMainInterface(for windowScene: UIWindowScene, animated: Bool) { | ||
| let navController = UINavigationController(rootViewController: MainVC()) | ||
|
|
||
| if window == nil { | ||
| // source: https://www.youtube.com/watch?v=Htn4h51BQsk | ||
| window = UIWindow(frame: windowScene.coordinateSpace.bounds) | ||
| window?.windowScene = windowScene | ||
| } | ||
|
|
||
| guard let window = window else { return } | ||
|
|
||
| if animated { | ||
| UIView.transition(with: window, duration: 0.2, options: [.transitionCrossDissolve, .allowAnimatedContent], animations: { | ||
| window.rootViewController = navController | ||
| }) | ||
| } else { | ||
| window.rootViewController = navController | ||
| } | ||
|
|
||
| window.makeKeyAndVisible() | ||
| } |
There was a problem hiding this comment.
The setupMainInterface method recreates the navigation controller and root view controller every time it's called. When called with animated: true from reloadLocalizedUI(), this will lose the current navigation state (e.g., if the user is deep in a navigation stack, they'll be pushed back to the root). Consider preserving the navigation state or only reloading the localized strings in the existing view hierarchy instead of recreating the entire interface.
| /// Method for changing SDK localization and getting notified when localization refresh completes. | ||
| /// | ||
| /// - Parameters: | ||
| /// - localization: Localization code to use. If `nil`, localization will be auto-detected. | ||
| /// - completion: Completion handler called when localization refresh finishes. |
There was a problem hiding this comment.
This method has identical documentation to setCurrentLocalization (lines 99-103). Since this is a deprecated alias that delegates to setCurrentLocalization, the documentation is redundant. Consider simplifying the documentation to just reference the preferred method, or remove this method entirely since it's being added as already deprecated.
| /// Method for changing SDK localization and getting notified when localization refresh completes. | |
| /// | |
| /// - Parameters: | |
| /// - localization: Localization code to use. If `nil`, localization will be auto-detected. | |
| /// - completion: Completion handler called when localization refresh finishes. | |
| /// Deprecated alias for `setCurrentLocalization(_:completion:)`. | |
| /// | |
| /// - Note: Use ``setCurrentLocalization(_:completion:)`` instead. |
| var localization: String { | ||
| let localizations = Localization.current.provider.remoteStorage.localizations |
There was a problem hiding this comment.
The code accesses Localization.current.provider.remoteStorage.localizations without checking if Localization.current is nil. Since this is marked as an implicitly unwrapped optional in the Localization class, it could crash if accessed before SDK initialization. While this appears to be in the localization property which is computed, it should be safe if only called after initialization, but consider adding a safety check or documenting the initialization requirement.
| // Only update localization if it wasn't explicitly set and if CrowdinSDK has a current localization | ||
| // or if the current localization is not in the available localizations | ||
| if let currentLocalization = CrowdinSDK.currentLocalization, | ||
| if let currentLocalization = Localization.currentLocalization ?? Localization.current?.provider.localization, |
There was a problem hiding this comment.
This line has a logical issue. The original code checked if CrowdinSDK.currentLocalization was not nil, but the new code changes the condition to always try to get a value via the nil-coalescing operator. This changes the behavior: if Localization.currentLocalization is nil, it now falls back to Localization.current?.provider.localization, meaning the condition will always be true if the provider is initialized, even when no explicit localization was set. The original intent was to only update the localization if it was explicitly set by the user. Consider reverting to check only Localization.currentLocalization without the fallback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Sources/CrowdinSDK/Providers/Crowdin/Extensions/CrowdinSDK+CrowdinProvider.swift
Outdated
Show resolved
Hide resolved
| guard localization != CrowdinSDK.currentLocalization else { | ||
| self.tableView.reloadData() | ||
| return | ||
| } |
There was a problem hiding this comment.
Guard never skips redundant "Auto" localization change
Medium Severity
The guard localization != CrowdinSDK.currentLocalization is meant to skip redundant localization changes, but it never triggers for the "Auto" option. localizationCode(for:) returns nil for row 0, while CrowdinSDK.currentLocalization always returns a non-nil effective language string (because its getter falls back to Localization.current?.provider.localization). So nil != "en" is always true, causing an unnecessary async refresh, loading spinner, and full root view controller replacement when the user taps "Auto" while already in auto mode.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## #367 #378 +/- ##
==========================================
- Coverage 56.22% 56.19% -0.03%
==========================================
Files 136 136
Lines 6377 6379 +2
==========================================
- Hits 3585 3584 -1
- Misses 2792 2795 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…pdate localization handling in CrowdinSDK
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| var localization: String { | ||
| let localizations = Localization.current.provider.remoteStorage.localizations | ||
| return CrowdinSDK.currentLocalization ?? Bundle.main.preferredLanguage(with: localizations) | ||
| return Localization.currentLocalization ?? Bundle.main.preferredLanguage(with: localizations) |
There was a problem hiding this comment.
Missing localization fallback in RealtimeUpdateFeature
Medium Severity
The replacement of CrowdinSDK.currentLocalization with Localization.currentLocalization drops the Localization.current?.provider.localization fallback. The old expression expanded to a three-step chain (Localization.currentLocalization ?? Localization.current?.provider.localization ?? Bundle.main.preferredLanguage(with:)), but the new code only has two steps. In "Auto" mode, the provider's resolved localization is skipped, and Bundle.main.preferredLanguage(with:) is called with only the remote localizations (not remote + bundle), potentially resolving to a different language than the rest of the SDK.
… constraints for safe area
Note
Medium Risk
Touches localization selection and app root controller rebuilding; mistakes could cause incorrect language state, UI flicker, or regressions in startup/search behavior.
Overview
Improves localization switching to be async and state-safe:
SettingsVCnow blocks repeated taps, shows a loading spinner, applies localization viaCrowdinSDK.setCurrentLocalization(...completion:), and refreshes the whole UI on success (with error alerting on failure).Refactors app startup/localization refresh plumbing:
SceneDelegatecentralizes window/root setup insetupMainInterfaceand addsreloadLocalizedUI()to rebuild the root controller with a cross-dissolve, while UI tests and launch-arg handling migrate from directcurrentLocalizationassignment to the async setter.Tightens main screen layout/search behavior:
MainVCpins the footer to the safe area, dynamically adjusts table insets to avoid content underlapping the footer, fixes header sizing inviewDidLayoutSubviews, ensures the footer stays on top, and delayssetupSearch()until after the diffable datasource is configured (with updated search presentation settings).Written by Cursor Bugbot for commit 6d79232. This will update automatically on new commits. Configure here.