Repository navigation
#2350 Serve downloaded translations to Services, notifications and the application context with LingoHub.wrap() - #29
Conversation
…350) Lookups only ever ran on the main thread, from Activities that exist after Application.onCreate() has called configure(). LingoHub.wrap() (next commit) brings them to the Application, which Android asks for strings before onCreate() runs, and to Services and workers on their own threads: - LingoHubLogger starts with a silent logger instead of a lateinit one, and LingoHub creates its BundleHelper up front. Both threw UninitializedPropertyAccessException before configure(). - The repository cache lives inside the release it was built from: BundleHelper publishes the bundles and their repositories as one volatile snapshot. A lookup racing an update resolves against one release, and a repository built from the old release can no longer land in the cache after the refresh cleared it, where it stayed until the next update. The plain map it replaces was also written from several threads. - LocaleProvider keeps the selected locale in one volatile field instead of two plain ones, and SnapKitHelper's map is a ConcurrentHashMap. addRepository() and clearRepositories() go away; the only caller was a test that injected an unused mock. Tests: BundleHelperTest covers the state before the first refresh, repository reuse, invalidation on refresh, and lookups on four threads racing 200 refreshes.
…ontext (org #2350)
LingoHub.wrap(context) returns a context whose string lookups serve the
downloaded release. The Application and Services return its resources
from getResources():
private val lingoHubContext by lazy { LingoHub.wrap(baseContext) }
override fun getResources(): Resources = lingoHubContext.resources
The issue proposed passing the wrapped context to attachBaseContext()
instead. For the Application, that crashes the app on every broadcast to
a receiver declared in the manifest, including the ones Firebase
Messaging and WorkManager declare: ActivityThread.handleReceiver casts
the application's base context to ContextImpl (API 24 through 36;
reproduced on an API 36 emulator: "LingoHubContextWrapper cannot be cast
to android.app.ContextImpl"). Overriding getResources() leaves the base
context alone, and manifest receivers are covered too, because Android
hands them a context that delegates to the Application.
The wrapper copies the base resources, and the framework updates only
the base in place, so a long-lived wrapper kept reporting the
configuration it was created with. Android reads the application's
configuration back after every change to set the process default
locale, and AppCompat reads it for night mode and app locales.
LingoHubContextWrapper now rebuilds its copy when the base resources,
their assets or their configuration changed, and is safe to use from
any thread. The unused updateConfiguration() goes away, and
createConfigurationContext() no longer resets the parent's resources.
Tests: instrumented, with a test Application, Service and manifest
receiver set up as the README describes: strings read before
configure(), applicationContext (text, format, plural), a notification
built in a Service, a manifest receiver, createConfigurationContext(),
wrap() of a wrapped context, lookups on background threads during an
update, and a configuration change (font scale) and an app language
change (API 33+) through the wrapped application context. Both
configuration tests fail when the wrapper keeps its first copy. CI runs
the suite on API 24 and 36 emulators.
… the sample (org #2350) The README gets a "Strings outside Activities" section: the Application and Service patterns, why getResources() and not attachBaseContext(), what a wrapped context guarantees (before configure(), any thread, setLocale(), new releases, configuration changes), and what stays as packaged: layouts inflated outside Activities, RemoteViews in widgets and custom notification layouts, and Resources.getSystem(). Quick Start, Features and Troubleshooting point to it, README-STRUCTURE lists it as the Android platform extra, and CHANGELOG has the entries. The Wanderly sample wraps its Application and posts the seat-upgrade push notification from UpgradeNotificationService, asking for the notification permission on Android 13 and later. notifications_upgrade is part of the Wanderly content, so the notification shows the downloaded translation, also in languages the APK does not ship.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
hemju
left a comment
hemju
left a comment
There was a problem hiding this comment.
AI-generated review by OpenAI Codex — reviewed commit 64e06cb, all 3 commits / 32 changed files, issue lingohub/organization#2350, the discussion, and the surrounding resource, repository, update, and Activity integration code.
Two integration issues should be addressed before merging; both have independent API 36 reproductions in the inline comments:
- [P2] Previously returned Application resources remain stale after configuration changes. Retaining
application.resourcespreserves the old plural rules and display metrics even after a freshapplication.resourceshas updated. This affects normal long-lived consumers of Application resources. - [P2] The claim that manifest receivers need no additional integration is too broad. A receiver with
android:attributionTagsreceives a derived context that bypasses the Application override. Document explicit receiver wrapping and cover that case.
A third inline comment identifies the inherited, non-blocking getText(id, default) inconsistency. It already exists in ResourcesUtil, but the new public API is a useful opportunity to close or explicitly document it.
The release-scoped repository cache is a sound architectural improvement: publishing bundles and their repository cache as one volatile snapshot prevents an older repository from contaminating a newer release. Keeping ViewPump out of the public signature, initializing lookup dependencies before configure(), and retaining Android's original Application base context are also appropriate choices. The attachBaseContext() deviation from the issue is supported by Android's receiver implementation.
Additional maintainability suggestions:
- Make the concurrency tests coordinate their readers with a latch/barrier and assert that reads actually happened during the update. The instrumented test currently accepts an empty
seenset becauseseen.all { ... }is true for an empty set; it can pass without exercising the intended overlap. A deterministic regression for an old repository build completing after a refresh would provide stronger protection. - Put decision 4 from the PR description into the public documentation:
createConfigurationContext()does not select the OTA language;setLocale()/ the process default still does. That matters particularly next to theContextCompat.getContextForLanguage()example.
Validation: an independent ./gradlew build :sdk:assembleDebugAndroidTest --offline passed, including 101 unit tests, lint, and the sample build. The unchanged 9-test instrumented suite passed on a separate API 36 emulator. GitHub CI also reports successful API 24 and API 36 jobs; API 24 was not independently rerun here. Additional review-only probes reproduced the inline issues. No production fixes or commits were made as part of this review.
…Text(id, default) (org #2350) Review follow-ups: - Resources handed out earlier, such as application.resources kept by a long-lived helper, are now updated in place on a configuration change instead of being replaced, and packaged fallbacks read through the live base resources. A kept copy used to keep the old plural rules and display metrics: 5 in Russian picked "other" instead of "many", and sp dimensions ignored a new font scale. Android asks the application for its resources during every configuration change (API 24 through 36), so the application's copy is updated right away. Only new assets (overlays, split APKs) still need a new copy. - getText(id, default) serves the downloaded translation like getText(id), and returns the default for 0 or a missing resource, like Android. - A receiver with android:attributionTags gets an attribution context of its own that doesn't delegate to the Application. The wrap() KDoc now says to wrap the context onReceive() gets, and a test (API 31+) covers such a receiver. Tests: the configuration tests now check Resources kept from before the change (an sp dimension across a font scale change, and a plural whose rules differ across a language change); both fail with the previous wrapper, as does the getText(id, default) test without the override. The background-thread test requires each reader to read the old and then the new release. A new JVM test holds a repository build of the old release until the new one is live; it fails with a cache shared across releases, and so does the strengthened concurrent test, which now asserts that the readers ran during the refreshes.
…g #2350) The README listed receivers declared in the manifest as covered by the Application setup. That holds for most of them, but not for one with android:attributionTags, so the section now shows LingoHub.wrap(context) in onReceive(), and the CHANGELOG and the sample no longer list manifest receivers. It also says that downloaded strings are always in LingoHub's language, even in a context created for another language, and that application.resources kept by a helper follows configuration changes. The CHANGELOG gets the getText(id, default) fix.
|
Thanks for the review. All three inline points and both suggestions are addressed in 69953ae (SDK and tests) and bd1674a (docs), on top of a merge of Concurrency tests
Docs
Verification
|
hemju
left a comment
hemju
left a comment
There was a problem hiding this comment.
AI-generated follow-up review by OpenAI Codex — re-reviewed bd1674a, including all changes since 64e06cb, the merge of main, and the responses to the first review.
All three original inline points are addressed. No remaining blocking findings in the reviewed changes.
- Retained Application resources: the copy is now updated in place, and packaged string/plural/array fallbacks use the live base resources. Independent probes that previously failed now pass for retained-resource font scaling and Russian plural selection.
- Attributed receivers: the README, KDoc, changelog and PR description now specify explicit receiver wrapping. Both the new instrumented test and an independent probe confirm that the wrapped attribution context serves the release; the raw context remains outside that guarantee, as documented.
getText(id, default): the override now serves downloaded translations and preserves default behavior for zero/missing IDs. Independently verified.
The two additional suggestions are addressed too: the tests require reader activity and include a controlled old-repository-build/new-release race; the README explicitly distinguishes LingoHub's selected language from a derived context's packaged fallback language. The merge retains main's lazy lookup logging.
Independent validation:
./gradlew build :sdk:assembleDebugAndroidTest --offline: passed, including 104 unit tests, lint and sample compilation.- Unmodified instrumented suite on API 36: 11/11 passed.
- Additional review probes on API 36: 5/5 passed, covering retained configuration/dimensions, plural fallback, ordinary string fallback, the default-bearing text overload, and the documented attributed-receiver wrapping path.
- Latest GitHub CI: build, release-script checks, and API 24/API 36 emulator jobs passed. API 24 was not independently rerun locally.
Test reliability note: the first full suite after the manual probes timed out in font scale restored; resetting the isolated emulator's font-scale setting to 1.0 produced the clean 11/11 run above. Synchronizing the setting and the base configuration before recording the restore target would make this test less sensitive to a reused device's state. This does not reproduce either original production finding.
The review worktree has no source changes. The PR remains open; cleanup after merge remains scheduled.
Implements lingohub/organization#2350.
Why
Translations only reached Activities that use the
getAppCompatDelegatedelegate. Everything else kept the strings packaged in the APK, and no public API could change that:Application, and with itapplicationContextand WorkManager workers;Service, such as aFirebaseMessagingServicethat builds push notifications;BroadcastReceivers.Change
New public API
LingoHub.wrap(base: Context): Context, as the issue specifies. Its string lookups serve the downloaded release and fall back to the APK strings.@Keep @JvmStatic.configure()and from any thread.The documented integration overrides
getResources()instead of callingattachBaseContext(LingoHub.wrap(base))as the issue proposed (decision 1). ABroadcastReceiverwraps the contextonReceive()gets (decision 2).The wrapper follows configuration changes (decision 3).
ResourcesUtilis a copy of the baseResources, and Android only updates the base in place. Android and AppCompat both read the configuration back through the application context:LingoHubContextWrappertherefore updates its copy in place when the base configuration changes, soResourcesit handed out earlier stay current too (for exampleapplication.resourceskept by a helper). It builds a new copy only when the base object or its assets change. Packaged fallbacks read through the live base resources. The unusedupdateConfiguration()goes away, andcreateConfigurationContext()no longer resets the parent's resources.getText(id, default)serves the downloaded translation likegetText(id). It keeps Android's semantics: the default for0or a missing resource.String lookups are safe before
configure()and on any thread. The issue assumed the wrapper already was; it wasn't:LingoHubLogger.loggerandLingoHub.bundleHelperwerelateinit, so any lookup beforeconfigure()threw. The Application is asked for strings before itsonCreate()runs.LocaleProvider(one volatile field) andSnapKitHelper(aConcurrentHashMap) are now thread-safe too.README: new section "Strings outside Activities" with:
RemoteViews,Resources.getSystem()) plus layouts inflated outside Activities, whose XMLandroid:textis not replaced.Quick Start, Features and Troubleshooting point to it.
docs/README-STRUCTURE.mdlists it as the Android platform extra, andCHANGELOG.mdhas Added and Fixed entries.Sample:
UpgradeNotificationService, which posts the seat-upgrade notification and asks for the notification permission on Android 13+.notifications_upgradeis part of the Wanderly content, so the notification shows the downloaded translation, also in languages the APK doesn't ship.CI: a new
instrumented-testsjob runs the androidTest suite on API 24 and API 36 emulators. There were no instrumented tests before.Decisions worth a look
getResources(), notattachBaseContext(). A wrapped Application base context crashes the app on every broadcast to a receiver declared in the manifest, including the ones Firebase Messaging and WorkManager declare. The cause is thatActivityThread.handleReceivercastsapp.getBaseContext()toContextImpl: AOSP, Android 16, with the same cast since Android 7. Reproduced on an API 36 emulator with the test suite switched to the issue's pattern:handleStopServiceskip the cleanup of receivers the service forgot to unregister. OverridinggetResources()in both keeps the base context untouched, and it's the same two lines everywhere.ReceiverRestrictedContext(getOuterContext()), which delegates to the Application. A receiver withandroid:attributionTagsdoesn't: Android creates an attribution context first, and that context's outer context is itself (found in review). The README therefore showsLingoHub.wrap(context)inonReceive(), and a test covers an attributed receiver (API 31+).getResources()call it compares the baseResources, theirAssetManagerand a snapshot of theirConfiguration, which costs about oneConfiguration.equals. On a change it updates the copy in place withResources.updateConfiguration(), the same way Android updates the base.updateLocaleListFromAppContext, API 24 through 36), so the application's copy, and whatever kept it, is updated right away.ComponentCallbackswould come too late: Android updates the default locale before it dispatchesonConfigurationChanged.createConfigurationContext(), for example throughContextCompat.getContextForLanguage(), resolves downloaded strings in the SDK's locale (setLocale(), otherwiseLocale.getDefault()), and only its packaged fallbacks follow its configuration. This behavior is unchanged; the README now says so.Tests
JVM:
BundleHelperTest(6 tests) covers:The unit suite has 104 tests.
Instrumented: 11 tests (9 below API 31). The test APK's own
Application,Serviceand receivers are set up as the README describes. The tests cover:Application.onCreate()beforeconfigure()return the APK string, with no crash at process start;applicationContextserves the release (text, format arguments, plural), while the unwrapped context keeps the APK string;Servicecarries the release's title and plural text;applicationContextandwrap(context)all serve the release;wrap(context)serves the release there (API 31+);getText(id, default)serves the release, and returns the default for0or a missing id;createConfigurationContext()on a wrapped context serves the release, andwrap()of a wrapped context is the same object;application.resourceskept from before the change (anspdimension);Mutation runs: each regression fails on the code it guards against:
5 яблока);getText(id, default)override;attachBaseContextpattern, which crashes with theClassCastExceptionabove.Run locally:
./gradlew build(lint, unit tests, sample) andconnectedDebugAndroidTeston API 24 and API 36 emulators. CI runs both emulator jobs.Sample on an API 36 emulator: with a local Spanish release in place, tapping ES and then Send push notification posts "¡Te hemos subido de categoría, Alex! Disfruta del espacio extra.". Spanish isn't in the APK. Airplane mode was on, so the placeholder key never reached the CDN.
The branch contains the current
main, including #27 (update pacing, the staged install API) and #30 (lazy lookup logging).