Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Come up with long-term strategy for managing view data management #1051

Open
BenHenning opened this issue May 10, 2020 · 3 comments
Open

Come up with long-term strategy for managing view data management #1051

BenHenning opened this issue May 10, 2020 · 3 comments
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

BenHenning commented May 10, 2020

The current solution for view-specific data management has a few issues:

  • View models are currently fragment-bound, which means they easily leak the fragment state OR aren't true view models (e.g. are recreated when fragments are recreated)
  • Passing data between fragments involves a lot of additional boilerplate by funneling through common ancestors

#147 may help solve the second point, but the first one needs additional thinking. In general, we should:

  1. Come up with a holistic strategy for how to cleanly store & pass view-specific data through the UI, taking into consideration configuration changes, activity destruction & bundling, data-binding observers (see Convert all ViewModels to ObservableViewModel #234), and processing live data observers
  2. Determine an implementation plan & migration strategy for the current codebase
  3. Break up the migration into smaller pieces that are filed as separate issues, then drive this migration effort (this is a good project to parallelize among contributors)

This issue will be the top-level issue tracking the above.

@BenHenning
Copy link
Member Author

BenHenning commented May 10, 2020

NB: part of this may also involve simplifying our current interface set up for the state/question fragments since there are quite a few listeners now, and some seem closely related (I think there are actually some situations where synchronous events are moving up and down the UI hierarchy a couple times for simple operations). While there's not a serious performance concern here (and likely not a concern with leaks either), it does introduce a bit of complexity which makes tracking down handlers & origins of events harder.

@BenHenning
Copy link
Member Author

Also maybe a better way to be thinking about this might be to consider both how data should be stored, and how it should be passed in the UI level. For each of these I think there are a few categories.

Places to store data:

  • Presenter: for data that does not need to survive across configuration changes or state loss
  • View model: for data that should survive configuration changes but doesn't need to survive state loss
  • Bundles: for UI-specific data that should survive state loss
  • Domain controllers: for application state that should survive state loss (including persisted data)

Ways to transfer data:

  • Intents & argument bundles: for parameters used to initialize an activity/fragment
  • Events/listeners: for transient data that needs to be passed across UI component boundaries (generally up the UI hierarchy--all data to components at the same level or below another component must go through abstract listeners via a common ancestor that directly calls down without storing fragment/view references)
  • Via domain controllers: this should only be done for data that falls under the type of data stored here per the above list

We should think through the above & see if any other cases should be added, or if we should handle data in a different way than the above. Otherwise, if that looks good we can optimize our approaches to simplify the above. We should also come up with typical use cases, examples, and best practices around each of the above to make things a bit more structured.

@BenHenning BenHenning assigned BenHenning and unassigned BenHenning May 10, 2020
@BenHenning BenHenning added this to the Beta milestone Jun 23, 2020
@Broppia Broppia added issue_type_infrastructure Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 14, 2022
BenHenning added a commit that referenced this issue 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.
@BenHenning BenHenning added Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged. Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. labels Sep 16, 2022
@BenHenning BenHenning removed this from the Beta milestone Sep 16, 2022
@BenHenning
Copy link
Member Author

One idea I just had: we could automatically generate a true view model by annotating compatible fields with something like @RetainAcrossConfigChanges. We could also similarly have a @RetainAcrossAppDeath, but perhaps we should just do both right away? I'm not sure since the former could be any non-lifecycle object, but latter has to be serializable (so it must be a proto).

This may go well with existing plans to generate fragment, activity, and view classes with corresponding presenters. Given the existing complexity around using injectable classes in view models, and the conflation between Oppia and Jetpack view models, it may be the best just to move off of view models entirely (but still leverage their benefits via code generation). However, this is not going to work well with databinding since our entire approach is utilizing view models to interact with views, so we should figure out a replacement for databinding OR figure out how this would work with Compose.

This also seems like the best option since:

  • It's currently awkward to test view models directly (they're tested through their corresponding fragments currently).
  • Activities mostly can't have view models.
  • Using view models correctly such that they work in both Dagger and across configuration changes is pretty difficult, and avoiding it seems cleaner.
  • Generally it makes more sense to keep all logic in one place (presenters) since view models are tightly coupled with presenters, anyway.

@seanlip seanlip changed the title Come up with long-term strategy for managing view data mangement Come up with long-term strategy for managing view data management Mar 28, 2023
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
BenHenning added a commit that referenced this issue Jun 3, 2024
…5402)

## Explanation
Fixes part of #4120
Fixes part of #1051

Similar to #5400, this brings forward changes that would otherwise go in
#4937 to simplify the transition to Kotlin 1.6.

Part of #4937 is introducing warnings-as-errors for both Kotlin and Java
in order to reduce developer error, simplify the codebase, and minimize
warnings when building (which can result in developer habits of ignoring
warnings that might have real consequences to end users of the app). In
order to keep the main migration PR smaller, this PR fixes all existing
warnings and any new ones detected with the Kotlin 1.6 compiler that are
not tied to Kotlin 1.5/1.6 API changes (those are part of #4937,
instead). Fortunately, most of the changes could be brought forward into
this PR.

Specific things to note:
- A few new issues were filed for SDK 33 deprecations caused, but not
noted by, #5222): #5404, #5405, and #5406 and corresponding TODOs added.
This PR opts for TODOs over actual fixes to minimize the amount of
manual verification needed, and to try and keep the PR more focused on
non-functional refactor changes (to reduce the risk as reverting this PR
may be difficult if an issue is introduced).
- A lot of the fixes were removing redundant casts or null checks.
- The old mechanism we used for view models is deprecated, and had a lot
of problems (partially documented in #1051). This PR moves the codebase
over to directly injecting view models instead of using the view model
provider (thus getting rid of true Jetpack view models entirely in the
codebase).
- We never used the Jetpack functionality, and we were leaking a lot of
context objects that could theoretically result in memory leaks.
- The migration of view models in this way has already been ongoing in
the codebase; this PR just finishes moving the rest of them over to
remove the deprecated JetPack view model reference.
- Note that this doesn't actually change the scope of the view models,
and in fact they should largely behave as they always have.
- ``ObservableViewModel`` was subsequently updated, and may be something
we could remove in the future now that it's no longer a Jetpack view
model.
- The old view model binding code was removed, along with its test file
exemptions. It's no longer used now that the view models have been
finished being migrated over to direct injection.
- Some of the binding adapters didn't correctly correspond to their
namespaced properties. I _think_ that the databinding compiler was still
hooking them up correctly, but they produced build warnings that have
now been addressed (specifically, 'app' is implied). Some other
properties were using unusual namespaces, so these were replaced with
'app' versions for consistency & correctness.
- Some cases where SAM interfaces could be converted to lambdas were
also addressed (mainly for ``Observer`` callbacks in UI code).
- ``DrawerLayout.setDrawerListener`` was replaced with calls to
``DrawerLayout.addDrawerListener`` since the former [is
deprecated](https://developer.android.com/reference/androidx/drawerlayout/widget/DrawerLayout#setDrawerListener(androidx.drawerlayout.widget.DrawerLayout.DrawerListener)).
This isn't expected to have a functional difference.
- Some other minor control flow warnings were addressed (such as dead
code paths).
- ``when`` cases were updated to be comprehensive (as this is enforced
starting in newer versions of Kotlin even for non-result based ``when``
statements).
- Some unused variables were removed and/or replaced with ``_`` per
Kotlin convention.
- Some parameter names needed to be updated to match their override
signatures.
- One change in ``ExitSurveyConfirmationDialogFragment`` involved
removing parsing a profile ID. Technically this is a semantic change
since now a crash isn't going to happen if the profile ID is missing or
incorrect, but that seems fine since the fragment doesn't even need a
profile ID to be passed.
- Some of the test activities were updated to bind a ``Runnable``
callback rather than binding to a method (just to avoid passing the
unused ``View`` parameter and to keep things a bit simple binding-wise).
- Some cases were fixed where variables were being shadowed due to
reused names in deeper scopes.
- There were some typing issues going on in tests with custom test
application components. This has been fixed by explicitly declaring the
application component types rather than them being implicit within the
generated Dagger code.
- ``getDrawable`` calls were updated to pass in a ``Theme`` since the
non-theme version is deprecated.
- Some Java property references were updated, too (i.e. using property
syntax instead of Java getters when referencing Java code in Kotlin).
- In some cases, deprecated APIs were suppressed since they're needed
for testing purposes.
- Mockito's ``verifyZeroInteractions`` has been deprecated in favor of
``verifyNoMoreInteractions``, so updates were made in tests accordingly.
- ``ExperimentalCoroutinesApi`` and ``InternalCoroutinesApi`` have been
deprecated in favor of a newer ``OptIn`` method (which can actually be
done via kotlinc arguments, but not in this PR). Thus, they've been
outright removed in cases where not needed, and otherwise migrated to
the ``OptIn`` approach where they do need to be declared.
- In some cases, Kotlin recommends using a ``toSet()`` conversion for
iterable operations when it's more performant, so some of those cases
(where noticed) have been addressed.
- Some unused parameter cases needed to be suppressed for situations
when Robolectric is using reflection to access them.
- In some cases Android Studio would recommend transformation chain
simplifications; these were adopted where obvious.
- There are a few new TODOs added on #3616 as well, to clean up
deprecated references that have been suppressed in this PR.
- ``BundleExtensions`` was updated to implement its own version of the
type-based ``getSerializable`` until such time as ``BundleCompat`` can
be used, instead (per #5405).
- A **lot** of nullability improvements needed to happen throughout the
JSON asset loading process since there was a lot of loose typing
happening there.
- Some Kotlin & OkHttp deprecated API references were also updated to
use their non-deprecated replacements.
- ``NetworkLoggingInterceptorTest`` was majorly reworked to ensure that
the assertions would actually run (``runBlockingTest`` was being used
which is deprecated, and something I try to avoid since it's very
difficult to write tests that use it correctly). My investigations
showed that the assertions weren't being called, so these tests would
never fail. The new versions will always run the assertions or fail
before reaching them, and fortunately the code under test passes the
assertions correctly. Ditto for ``ConsoleLoggerTest``.
- Some parts of ``SurveyProgressController`` were reworked to have
better typing management and to reduce the need for nullability
management.
- Some generic typing trickiness needed to be fixed ahead of the Kotlin
version upgrade in ``UrlImageParser``. See file comments & links in
those comments for more context.
- ``BundleExtensionsTest`` had to be changed since
``getSerializableExtra`` is now deprecated. We also can't update the
test to run SDK 33 since that requires upgrading Robolectric, and
Robolectric can't be upgraded without upgrading other dependencies that
eventually lead to needing to upgrade both Kotlin and Bazel (so it's a
non-starter; this is a workaround until we can actually move to a newer
version of Robolectric).
- There was some minor code-deduplication & cleanup done in
``ClickableAreasImage``.
- Some incorrect comments were removed in tests (to the effect of "using
not-allowed-listed variables should result in a failure."). These seemed
to have been copied from an earlier test, but the later tests weren't
actually verifying that behavior so the comment wasn't correct.
- An unused method was removed from ``ConceptCardRetriever``
(``createWrittenTranslationFromJson``) and some other small
cleanup/consolidation work happened in that class.
- Some stylistic changes were done in ``TopicController`` for JSON
loading to better manage nullable situations, and to try and make the
JSON loading code slightly more Kotlin idiomatic.

Note that overall the PR has relied **heavily** on tooling to detect
warnings to fix, and automated tests to verify that the changes have no
side effects.

Note also that this PR does not actually enable warnings-as-errors; that
will happen in a downstream PR.

## 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
N/A -- While this changes UI code, it should change very few UI
behaviors and only failure cases for those it does affect. It's largely
infrastructural-only and falls mainly under refactoring/cleanup work.

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Co-authored-by: Sean Lip <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

No branches or pull requests

4 participants