-
Notifications
You must be signed in to change notification settings - Fork 527
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
* Migrate ListenableFuture to a Kotlin coroutine, and split HomeActivity into both an activity and a fragment. * Introduce the domain and testsupport modules. The domain module includes a user app history controller that provides instances of a new AsyncResult interface that's meant to be a potential bridge between Kotlin coroutines and LiveData. The exact design of how this should work needs to be determined as part of #6. This also includes a new testsupport module that's required due to robolectric/robolectric#4736. This is supporting a new test for the app history controller that leverages an AndroidX activity scenario to test the live data. Note that the test does not yet work since there's a race condition between the LiveData's coroutines completing and the test continuing to verify the state of the activity. This needs to be resolved, likely by waiting for test visual elements to change based on the LiveData result. Additional tests need to be added for other new components, and some slight cleaning up may be necessary. * Fix binary build: testsupport should only be depended on for tests, not for production configurations. * Further attempts to try and test the asynchronous behavior of the new LiveData + kotlin coroutine system. This introduces a custom LiveData to mediate between Kotlin coroutines in a way that allows tests to actually ensure the coroutines resolve quickly, and ensure lifecycle safety through standard LiveData mechanisms. However, this doesn't actually fix the underlying tests (which have been changed in a number of ways in this commit in an effort to try and get them to pass). The kotlinx coroutine deps were downgraded to work around a NoSuchMethodError (see Kotlin/kotlinx.coroutines#1331). After introducing a runBlockingTest structure and updating the user app histoyr controller to use the test context, the operations seem to work correctly. However, one job is hanging which is causing the test to still fail (it appears to be deadlocked--no known amount of time allows it to resolve). This new NotifiableAsyncLiveData also appears to break the production behavior, as well. The LiveData result is no longer being observed by the data binding transformation function, so it seems the coroutines aren't being executed, aren't completing, or something is deadlocking somewhere. Further investigation is needed. * Fix the app not properly binding to the live data by ensuring the data binder had the view model set properly. Fix resource binding issues in Robolectric tests by ensuring binary resources are provided to Robolectric. Fix the controller tests not working by introducing a temporary, custom CoroutineLiveData that ensures no long-living jobs continue running after the live data is completed. * Clean up tests (we no longer need a testsupport module for this module introduction). * Add test for AsyncResult. Set code style for Groovy files & clean up all build gradle files. Add missing newlines at end of various files. * Reformat top-level build.gradle file, as well. * Address review comments: replace name-based TODOs with issues. * Address review comments: add EOF new lines.
- Loading branch information
1 parent
fa05e2c
commit 6aa44f5
Showing
22 changed files
with
808 additions
and
178 deletions.
There are no files selected for viewing
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,65 +1,71 @@ | ||
apply plugin: 'com.android.application' | ||
|
||
apply plugin: 'kotlin-android' | ||
|
||
apply plugin: 'kotlin-android-extensions' | ||
|
||
android { | ||
compileSdkVersion 29 | ||
buildToolsVersion "29.0.1" | ||
defaultConfig { | ||
applicationId "org.oppia.app" | ||
minSdkVersion 16 | ||
targetSdkVersion 28 | ||
versionCode 1 | ||
versionName "1.0" | ||
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" | ||
compileSdkVersion 29 | ||
buildToolsVersion "29.0.1" | ||
defaultConfig { | ||
applicationId "org.oppia.app" | ||
minSdkVersion 16 | ||
targetSdkVersion 28 | ||
versionCode 1 | ||
versionName "1.0" | ||
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" | ||
} | ||
buildTypes { | ||
release { | ||
minifyEnabled false | ||
proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' | ||
} | ||
buildTypes { | ||
release { | ||
minifyEnabled false | ||
proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' | ||
} | ||
} | ||
testOptions { | ||
unitTests { | ||
includeAndroidResources = true | ||
} | ||
} | ||
dataBinding { | ||
enabled = true | ||
} | ||
testOptions { | ||
unitTests { | ||
includeAndroidResources = true | ||
} | ||
} | ||
|
||
// https://proandroiddev.com/isolated-fragments-unit-tests-that-run-both-instrumented-and-on-the-jvm-with-the-same-source-code-283db2e9be5d | ||
sourceSets { | ||
androidTest { | ||
java.srcDirs += "src/sharedTest/java" | ||
kotlin.srcDirs += "src/sharedTest/java" | ||
} | ||
test { | ||
java.srcDirs += "src/sharedTest/java" | ||
kotlin.srcDirs += "src/sharedTest/java" | ||
} | ||
// https://proandroiddev.com/isolated-fragments-unit-tests-that-run-both-instrumented-and-on-the-jvm-with-the-same-source-code-283db2e9be5d | ||
sourceSets { | ||
androidTest { | ||
java.srcDirs += "src/sharedTest/java" | ||
kotlin.srcDirs += "src/sharedTest/java" | ||
} | ||
test { | ||
java.srcDirs += "src/sharedTest/java" | ||
kotlin.srcDirs += "src/sharedTest/java" | ||
} | ||
} | ||
} | ||
|
||
dependencies { | ||
implementation fileTree(dir: 'libs', include: ['*.jar']) | ||
implementation"org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version" | ||
implementation 'androidx.appcompat:appcompat:1.0.2' | ||
implementation 'androidx.core:core-ktx:1.0.2' | ||
implementation 'androidx.constraintlayout:constraintlayout:1.1.3' | ||
api 'com.google.guava:guava:28.0-android' | ||
|
||
testImplementation 'androidx.test:core:1.2.0' | ||
testImplementation 'androidx.test.ext:junit:1.1.1' | ||
testImplementation 'org.robolectric:robolectric:4.3' | ||
testImplementation "com.google.truth:truth:0.43" | ||
testImplementation 'androidx.test.espresso:espresso-core:3.2.0' | ||
|
||
androidTestImplementation 'androidx.test:core:1.2.0' | ||
androidTestImplementation 'androidx.test.ext:junit:1.1.1' | ||
androidTestImplementation 'androidx.test:runner:1.2.0' | ||
androidTestImplementation "com.google.truth:truth:0.43" | ||
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0' | ||
|
||
implementation project(":model") | ||
implementation project(":utility") | ||
implementation fileTree(dir: 'libs', include: ['*.jar']) | ||
implementation( | ||
'android.arch.lifecycle:extensions:1.1.1', | ||
'androidx.appcompat:appcompat:1.0.2', | ||
'androidx.constraintlayout:constraintlayout:1.1.3', | ||
'androidx.core:core-ktx:1.0.2', | ||
'androidx.lifecycle:lifecycle-runtime-ktx:2.2.0-alpha03', | ||
"org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version", | ||
) | ||
testImplementation( | ||
'androidx.test:core:1.2.0', | ||
'androidx.test.espresso:espresso-core:3.2.0', | ||
'androidx.test.ext:junit:1.1.1', | ||
'com.google.truth:truth:0.43', | ||
'org.robolectric:robolectric:4.3', | ||
) | ||
androidTestImplementation( | ||
'androidx.test:core:1.2.0', | ||
'androidx.test.espresso:espresso-core:3.2.0', | ||
'androidx.test.ext:junit:1.1.1', | ||
'androidx.test:runner:1.2.0', | ||
'com.google.truth:truth:0.43', | ||
) | ||
implementation project(":model") | ||
implementation project(":domain") | ||
implementation project(":utility") | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,52 +1,14 @@ | ||
package org.oppia.app | ||
|
||
import androidx.appcompat.app.AppCompatActivity | ||
import android.os.Bundle | ||
import android.util.Log | ||
import android.widget.TextView | ||
import com.google.common.util.concurrent.FutureCallback | ||
import com.google.common.util.concurrent.Futures | ||
import com.google.common.util.concurrent.ListenableFuture | ||
import com.google.common.util.concurrent.MoreExecutors.directExecutor | ||
import org.oppia.app.model.UserAppHistory | ||
import org.oppia.util.data.AsyncDataSource | ||
import androidx.appcompat.app.AppCompatActivity | ||
|
||
/** The central activity for all users entering the app. */ | ||
class HomeActivity : AppCompatActivity() { | ||
|
||
override fun onCreate(savedInstanceState: Bundle?) { | ||
super.onCreate(savedInstanceState) | ||
setContentView(R.layout.home_activity) | ||
} | ||
|
||
override fun onStart() { | ||
super.onStart() | ||
// TODO(BenHenning): Convert this to LiveData rather than risk an async operation completing outside of the | ||
// activity's lifecycle. Also, a directExecutor() in this context could be a really bad idea if it isn't the main | ||
// thread. | ||
Futures.addCallback(getUserAppHistory().pendingOperation, object: FutureCallback<UserAppHistory> { | ||
override fun onSuccess(result: UserAppHistory?) { | ||
if (result != null && result.alreadyOpenedApp) { | ||
getWelcomeTextView().setText(R.string.welcome_back_text) | ||
} | ||
} | ||
|
||
override fun onFailure(t: Throwable) { | ||
// TODO(BenHenning): Replace this log statement with a clearer logging API. | ||
Log.e("HomeActivity", "Failed to retrieve user app history", t) | ||
} | ||
}, directExecutor()) | ||
} | ||
|
||
private fun getWelcomeTextView(): TextView { | ||
return findViewById(R.id.welcome_text_view) | ||
} | ||
|
||
private fun getUserAppHistory(): AsyncDataSource<UserAppHistory> { | ||
// TODO(BenHenning): Retrieve this from a domain provider. | ||
return object: AsyncDataSource<UserAppHistory> { | ||
override val pendingOperation: ListenableFuture<UserAppHistory> | ||
get() = Futures.immediateFuture(UserAppHistory.newBuilder().setAlreadyOpenedApp(false).build()) | ||
} | ||
supportFragmentManager.beginTransaction().add(R.id.home_fragment_placeholder, HomeFragment()).commitNow() | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package org.oppia.app | ||
|
||
import android.os.Bundle | ||
import android.util.Log | ||
import android.view.LayoutInflater | ||
import android.view.View | ||
import android.view.ViewGroup | ||
import androidx.fragment.app.Fragment | ||
import androidx.lifecycle.LiveData | ||
import androidx.lifecycle.Transformations | ||
import androidx.lifecycle.ViewModelProviders | ||
import org.oppia.app.databinding.HomeFragmentBinding | ||
import org.oppia.app.model.UserAppHistory | ||
import org.oppia.domain.UserAppHistoryController | ||
import org.oppia.util.data.AsyncResult | ||
|
||
/** Fragment that contains an introduction to the app. */ | ||
class HomeFragment : Fragment() { | ||
private val userAppHistoryController = UserAppHistoryController() | ||
|
||
override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? { | ||
val binding = HomeFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false) | ||
val viewModel = getUserAppHistoryViewModel() | ||
val appUserHistory = getUserAppHistory() | ||
viewModel.userAppHistoryLiveData = appUserHistory | ||
// NB: Both the view model and lifecycle owner must be set in order to correctly bind LiveData elements to | ||
// data-bound view models. | ||
binding.let { | ||
it.viewModel = viewModel | ||
it.lifecycleOwner = this | ||
} | ||
|
||
// TODO(#70): Mark that the user opened the app once it's persisted to disk. | ||
|
||
return binding.root | ||
} | ||
|
||
private fun getUserAppHistoryViewModel(): UserAppHistoryViewModel { | ||
return ViewModelProviders.of(this).get(UserAppHistoryViewModel::class.java) | ||
} | ||
|
||
private fun getUserAppHistory(): LiveData<UserAppHistory> { | ||
// If there's an error loading the data, assume the default. | ||
return Transformations.map( | ||
userAppHistoryController.getUserAppHistory() | ||
) { result: AsyncResult<UserAppHistory> -> processUserAppHistoryResult(result) } | ||
} | ||
|
||
private fun processUserAppHistoryResult(appHistoryResult: AsyncResult<UserAppHistory>): UserAppHistory { | ||
if (appHistoryResult.isFailure()) { | ||
Log.e("HomeFragment", "Failed to retrieve user app history", appHistoryResult.error) | ||
} | ||
return appHistoryResult.getOrDefault(UserAppHistory.getDefaultInstance()) | ||
} | ||
} |
10 changes: 10 additions & 0 deletions
10
app/src/main/java/org/oppia/app/UserAppHistoryViewModel.kt
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package org.oppia.app | ||
|
||
import androidx.lifecycle.LiveData | ||
import androidx.lifecycle.ViewModel | ||
import org.oppia.app.model.UserAppHistory | ||
|
||
/** [ViewModel] for user app usage history. */ | ||
class UserAppHistoryViewModel: ViewModel() { | ||
var userAppHistoryLiveData: LiveData<UserAppHistory>? = null | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,8 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<androidx.constraintlayout.widget.ConstraintLayout | ||
<FrameLayout | ||
xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:app="http://schemas.android.com/apk/res-auto" | ||
xmlns:tools="http://schemas.android.com/tools" | ||
android:id="@+id/home_fragment_placeholder" | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
tools:context=".HomeActivity"> | ||
|
||
<TextView | ||
android:id="@+id/welcome_text_view" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:text="@string/welcome_text" | ||
app:layout_constraintBottom_toBottomOf="parent" | ||
app:layout_constraintLeft_toLeftOf="parent" | ||
app:layout_constraintRight_toRightOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" /> | ||
|
||
</androidx.constraintlayout.widget.ConstraintLayout> | ||
tools:context=".HomeActivity" /> |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<layout | ||
xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:app="http://schemas.android.com/apk/res-auto"> | ||
|
||
<data> | ||
<import type="org.oppia.app.R"/> | ||
<variable | ||
name="viewModel" | ||
type="org.oppia.app.UserAppHistoryViewModel"/> | ||
</data> | ||
|
||
<androidx.constraintlayout.widget.ConstraintLayout | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent"> | ||
|
||
<TextView | ||
android:id="@+id/welcome_text_view" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:text="@{viewModel.userAppHistoryLiveData.getAlreadyOpenedApp() ? R.string.welcome_back_text : R.string.welcome_text}" | ||
app:layout_constraintBottom_toBottomOf="parent" | ||
app:layout_constraintLeft_toLeftOf="parent" | ||
app:layout_constraintRight_toRightOf="parent" | ||
app:layout_constraintTop_toTopOf="parent"/> | ||
|
||
</androidx.constraintlayout.widget.ConstraintLayout> | ||
</layout> |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,27 @@ | ||
// Top-level build file where you can add configuration options common to all sub-projects/modules. | ||
|
||
buildscript { | ||
ext.kotlin_version = '1.3.41' | ||
repositories { | ||
google() | ||
jcenter() | ||
} | ||
dependencies { | ||
classpath 'com.android.tools.build:gradle:3.4.2' | ||
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" | ||
classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10' | ||
// NOTE: Do not place your application dependencies here; they belong | ||
// in the individual module build.gradle files | ||
} | ||
ext.kotlin_version = '1.3.41' | ||
repositories { | ||
google() | ||
jcenter() | ||
} | ||
dependencies { | ||
classpath 'com.android.tools.build:gradle:3.4.2' | ||
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" | ||
classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10' | ||
// NOTE: Do not place your application dependencies here; they belong | ||
// in the individual module build.gradle files | ||
} | ||
} | ||
|
||
allprojects { | ||
repositories { | ||
google() | ||
jcenter() | ||
} | ||
repositories { | ||
google() | ||
jcenter() | ||
} | ||
} | ||
|
||
task clean(type: Delete) { | ||
delete rootProject.buildDir | ||
delete rootProject.buildDir | ||
} |
Oops, something went wrong.