Fix #4564: Introduce data provider dynamic transforms #4484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To ensure that the new dynamic DataProvider can't result in a soft lock if in-memory cache-backed providers (such as PersistentCacheStore) circularly reference each other (since they depend on a single thread), InMemoryBlockingCache was updated to use the standard background in conjunction with an actor channel. This ensures the lockless synchronization mechanism can continue without cross-cache contention occurring due to using a shared test. This commit also subsequently updates all blocking caches to use the same actor model, instead, and outright removes the blocking dispatcher to ensure these situations can never occur in the future.
…ic-transforms Conflicts: scripts/assets/test_file_exemptions.textproto testing/src/main/java/org/oppia/android/testing/threading/TestCoroutineDispatchersEspressoImpl.kt testing/src/main/java/org/oppia/android/testing/threading/TestCoroutineDispatchersRobolectricImpl.kt testing/src/main/java/org/oppia/android/testing/threading/TestDispatcherModule.kt utility/src/main/java/org/oppia/android/util/threading/DispatcherModule.kt
Does not include new tests that need to be added, or lint fixes.
This also includes fixing some core issues in AsyncDataSubscriptionManager and InMemoryBlockingCache that went previously unnoticed. More tests still need to be added yet.
(This is an in-progress commit that's needed so that I can transfer changes to a different machine).
|
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
BenHenning
added a commit
that referenced
this pull request
Sep 6, 2022
This undoes the work of PRs #4484 and others not yet created in order to isolate the expanded translations support directly off of the develop branch to expedite getting it finished for the upcoming beta release.
BenHenning
changed the title
BenHenning
changed the title
6 tasks
BenHenning
added a commit
that referenced
this pull request
Sep 8, 2022
…rt to titles & descriptions (#4555) ## Explanation Fixes #4224 Fixes #4306 Fixes part of #1051 This PR introduces expanded support for translating content strings. In particular, previous support for written content translations only extended as far as exploration state content and interactions. Now, all topics, stories, chapters, exploration, and revision cards titles and descriptions now also support translations. When considered in conjunction with app string translations, this now means that nearly the entire app is translatable. ### Technical approach At a high-level, this PR expands the existing pattern for content translations for titles and descriptions for structures: explorations, topics, revision cards (subtopics), stories, and chapters (story nodes). This required changing existing structures (and corresponding test assets) to use ``SubtitledHtml`` instead of raw strings for their titles and descriptions, and to now include written translation tables. New ephemeral structures have been introduced to compute the translations that can be consumed by the UI to ensure the correct string is displayed. Note that the test JSON assets have not been updated since it was a bit easier to just translate the title/description strings into ``SubtitledHtml``s during parsing time (which seems like a reasonable stopgap since the JSON loading pipeline is temporary and only developer-facing). Due to new ephemeral structures needing to be computed in a bunch of places, profile ID now needs to be passed in more places. This has led to some minor refactoring to push more UI components over to passing around the ``ProfileId`` structure rather than the internal integer ID. This PR also fixes part of #1051 by migrating more fragment presenters over to injecting their view models rather than retrieving them from Jetpack's view model provider (which means each of these changed presenters no longer expose potential memory leaks in the corresponding view model). ### Caveats This PR doesn't technically directly address #4224 and #4306, but it does indirectly. The new structures are linked to the assets download script which has been subsequently updated to cross-reference the translations for topics, stories, chapters, and subtopics from Oppia web's repository (which has temporary hardcoded translations for many of the titles and descriptions). This means that: - Not everything will be translated despite the app now supporting it (example: revision card content isn't available to cross-reference). - Many translation languages are missing (the team is currently focusing on Brazilian Portuguese ahead of the upcoming Beta MR1 app launch). - Longer term, Oppia web will provide the translations directly with the structures like it does for explorations (this is an ongoing backend project). Another caveat is that this PR further reveals the specific need to be able to compute a ``DataProvider`` based on the result of another. This is a limitation that the current ``DataProviders`` utility methods cannot handle, and the workaround is to call ``retrieveData()`` on the computed ``DataProvider`` directly which has updating inconsistency issues. Some of the providers modified as part of this PR should be updated to leverage a new dynamic transform method (see #4564 for the tracking issue). This was unfortunately dropped from the project due to limited time, but #4484 is the WIP PR. ### Test changes (or lack thereof) Due to this PR being especially high priority, tests have been omitted from this PR in favor of merging it faster. Instead, tests will be added in a follow-up PR (#4567 is tracking to make sure that this happens). The changes have been manually tested, and will be tested more prior to many users interacting with the new functionality. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only While this PR is highly end user-facing, for the sake of time screenshots and video demonstrations have been omitted from the PR. They will be added at a later date (after the PR is merged), and #4567 is tracking the work item to make sure that this happens. Commits: * Create dedicated alpha application component. This simplifies application component management significantly and allows individual build flavors to have their own unique module lists. * Add beta & GA update notices. This also introduces dedicated beta & GA build flavors which is a necessary prerequisite. It also introduces an extra beta, alpha, and dev mode labels for the splash screen (the latter 2 were extra) with 2 second minimum wait timers for beta and alpha to ensure they are seen. A 5-second safety timer was added to ensure the splash screen can always be passed even if something goes wrong at the domain level (since there are now quite a few moving pieces to determine the user's current onboarding state). * Add build tests for the new beta & GA flavors. * Initial support for dynamic data provider xforms. * Stabilization improvements. To ensure that the new dynamic DataProvider can't result in a soft lock if in-memory cache-backed providers (such as PersistentCacheStore) circularly reference each other (since they depend on a single thread), InMemoryBlockingCache was updated to use the standard background in conjunction with an actor channel. This ensures the lockless synchronization mechanism can continue without cross-cache contention occurring due to using a shared test. This commit also subsequently updates all blocking caches to use the same actor model, instead, and outright removes the blocking dispatcher to ensure these situations can never occur in the future. * Update codebase to Kotlin 1.6. * Add support for build-specific asset handling. * Fix build & Kotlin warnings (that are now errors). One test needed to be ignored due to the Kotlin coroutines update, so follow-up work will be needed to address this. Note that this commit also restricts access to the Kotlin reflection library (since it shouldn't be used broadly). This commit also updates Gradle to use Kotlin 1.6 (though only the app build has been verified). Proguard building support hasn't yet been verified. * Fix broken test per earlier changes. * Fix general broken tests & builds. Tests broken due to changes to the app startup experience haven't yet been fixed. * Lint fixes. * Introduce coordinated executors for tests. This commit replaces coroutine dispatchers with lower-level executors that perform the actual state management and coordination for all multi-threaded task handling in the app. This is actually a major simplification as it allows for: - Proper threading resource sharing between the app and its libraries (such as OkHttp) - Proper thread synchronization between executors and coroutines in tests without leveraging the extremely complicated CoroutineExecutorService - Less custom coroutine infrastructure which can be difficult to maintain * Fix broken tests & lint issues. * Post-merge & static check fixes. Does not include new tests that need to be added, or lint fixes. * Lots of test fixes. This also includes fixing some core issues in AsyncDataSubscriptionManager and InMemoryBlockingCache that went previously unnoticed. More tests still need to be added yet. * First part of new tests. (This is an in-progress commit that's needed so that I can transfer changes to a different machine). * First part of adding tests for GA notices. There's a bunch left to do here, this is mainly needed so that I can transfer changes to a different machine. * Update TransformAndroidManifestTest.kt Correct typos. * Fix tests & static checks. This also removes temporary debug code and TODOs, and finishes the tests for SplashActivity. * Post-merge fixes. * Test fixes. * Fix Gradle test. * Initial commit for expanded xlations support. This commit introduces the base changes needed to introduce translations support for all other content strings in the app outside core lessons. * Undo executors, Kotlin upgrade, and dynamic xform. This undoes the work of PRs #4484 and others not yet created in order to isolate the expanded translations support directly off of the develop branch to expedite getting it finished for the upcoming beta release. * Part 1 of post-"rebase" fixes. * Some post-merge fixes. * Undo changes from #4436. Isolating domain assets shouldn't be necessary for the expanded translations support, and it will be easier to manage this PR with fewer extra things being added to it. * Fix various tests & update test assets. Also, remove temporary TODOs. * Static fixes. * Add some string resource checks/tools. Also, fixes major performance issue with all file-based CI checks. * Ensure newline consistency in translated strings. Also, fix reporting in new validation check script. * Add tests & fix static checks. * Fix broken tests. * Update version codes & pt-BR strings. The strings were manually pulled Translatewiki. * Follow-up adjustments after self-review. * Undo changes from #4565. This commit allows the PR to be "rebased" back onto develop so that it can be prepped for merging.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
TODO: Finish.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: