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

Mark that the user opened the app in early demo #70

Closed
BenHenning opened this issue Aug 23, 2019 · 2 comments
Closed

Mark that the user opened the app in early demo #70

BenHenning opened this issue Aug 23, 2019 · 2 comments
Assignees
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

The WIP Android app shows a label indicating whether the user opened the app yet, but this is not persisted to disk to survive across multiple instances of the app. It should be to demonstrate the persistence system.

@BenHenning BenHenning added Type: Improvement Priority: Essential This work item must be completed for its milestone. labels Aug 23, 2019
@BenHenning BenHenning added this to the Proof of concept milestone Aug 23, 2019
@BenHenning BenHenning self-assigned this Aug 23, 2019
@BenHenning
Copy link
Member Author

NB: This issue also includes actually persisting the state to disk as part of demonstrating working data sources per #4.

rt4914 pushed a commit that referenced this issue Sep 3, 2019
* Initial introduction of the data source module.

High-level notes:
- Data sources are now being referred to as 'DataProviders'
- This commit also fixes the build which was broken in a previous PR
  ('compile' needs to be used for the proto library)
- The data source module is referred to as just 'data'
- This introduces a few style changes to prohibit wildcard imports
- This also normalizes the JDK versions needed across modules, potentially
  fixing some of the repeated reversions the IDE does for a Java SDK
  version setting

Functional differences: the app now properly shows the correct string upon
initial open ("Welcome to Oppia"), and reopening thereafter correctly
shows the updated string ("Welcome back to Oppia"). Rotating the device is
currently broken without Dagger integration.

Overall, this moves all general data provider functionality into the new
module, and introduces a PersistentCacheStore to store proto messages
on-disk, or load them into memory if they are available. Overall, this
commit is introducing a potentially reasonable medium-term solution for
data providers and on-disk proto storage. A long-term design should still
be determined, but this seems like a good place to start.

Tests are currently broken or missing. New TODOs will also be resolved in
a later commit.

* Introduce one possible integration with Dagger 2.

This is loosely based on both
https://github.com/tfcporciuncula/dagger-simple-way and
https://dagger.dev/android.html, except this approach is meant to keep
activities and fragments as thin as possible and push as much UI
presentation and service business logic into Dagger-provided objects as
possible. This maximizes Dagger use throughout the codebase, and opens the
potential for future code generation for these thin adapter fragments &
services.

This fixes the rotation bug in the app: rotating upon the initial app open
retains the 'Welcome to Oppia' label since the same user app history
controller is used in both instances of HomeFragment due to it being
singleton scoped and not needing to be recreated.

No tests were updated with these changes.

* Move data singletons and blocking/background dispatchers to Dagger
modules & Dagger graph. Update HomeScreenActivityTest to include a new
test for the second-app-launch test case.

Note that a significant amount of the work going into this commit was
around properly setting up tests to switch out their coroutine dispatchers
with dispatchers that could be controlled in a test context. Although the
affected test suites are passing in this commit, the work isn't quite done
yet. Espresso and Robolectric require slightly different mechanisms to
block on dispatchers, and with Gradle there isn't an easy way to set up
the build graph such that Espresso can do this reliably. A temporary
sleep-based workaround wad added for Espresso. Long-term, all tests should
use Espresso's idling resource and facilitate proper background thread
usage (which also simplifies the Robolectric side--no need to force
sequential execution or switch the main thread dispatcher).

Otherwise, this commit essentially "Dagger-ifies" the rest of the app.
This will become more beneficial over time, especially after the codebase
moves to Bazel since we'll be able to more easily swap out modules to
change dependency implementations in different contexts.

* Switch TODOs with references to issues.

* Fix typo.

* Introduce tests for InMemoryBlockingCache, DataProviders, and
PersistentCacheStore.

This fixes a few issues in PersistentCacheStore and the
DataProvider->LiveData conversion (mostly to ensure that duplicate data
notifications don't actually result in propagations to the UI). This also
includes correct equals, hash code, and toString behavior for AsyncResult.

The PersistentCacheStore tests aren't perfect, but they do expose some
issues with the current implementation and seem to provide a reasonable
breadth in core coverage. It may be worthwhile to eventually rewrite this
class into something both cleaner and easier to maintain.
BenHenning added a commit that referenced this issue Sep 3, 2019
@BenHenning
Copy link
Member Author

This was fixed as part of #85.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

No branches or pull requests

1 participant