Skip to content

Commit

Permalink
Introduce load exploration part 1 (#213)
Browse files Browse the repository at this point in the history
* Introduce first pass interface for ExplorationProgressController.

* Fill in the stubbed logic for ExplorationProgressController. Still no
tests to verify correctness.

Also, added a method to facilitate notifying of DataProvider changes on
the UI thread.

* Fix lateinit issue in ExplorationProgressController due to wrongly ordered
initialization.

* Fix a variaty of issues in the exp progress controller, properly hook it up to
the data controller, and start adding tests.

* Created a separate ExplorationRetriever, hooked up
AnswerClassificationController, and attempted to make
ExplorationProgressController thread-safe.

The thread-safety led to significant interface changes in the progress
controller, and led to discovering some issues with the mediator live data
approach to interop coroutines and LiveData. This locking mechanism will
need to change since the optimal solution requires resolving #90.

* Change the locking mechanism for ExplorationProgressController to work
with the current MediatorLiveData implementation (see #90 for more
context). Fix existing progress controller tests and add a few more. All
current progress controller tests are passing.

* Finish tests for ExplorationProgressController and add test classification
support for the second test exploration (about_oppia).

* ObservableViewModel

* Load exploration and open ExplorationActivity

* Test case for load exploration

* Connection with StateFragment

* Nit changes self review

* Refactor home

* Resolve merge conflicts

* Test case updated
  • Loading branch information
rt4914 authored Oct 9, 2019
1 parent 3bb093e commit 3db1aae
Show file tree
Hide file tree
Showing 9 changed files with 148 additions and 34 deletions.
7 changes: 6 additions & 1 deletion app/src/main/java/org/oppia/app/home/HomeActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,20 @@ package org.oppia.app.home

import android.os.Bundle
import org.oppia.app.activity.InjectableAppCompatActivity
import org.oppia.app.player.exploration.ExplorationActivity
import javax.inject.Inject

/** The central activity for all users entering the app. */
class HomeActivity : InjectableAppCompatActivity() {
class HomeActivity : InjectableAppCompatActivity(), RouteToExplorationListener {
@Inject lateinit var homeActivityPresenter: HomeActivityPresenter

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
activityComponent.inject(this)
homeActivityPresenter.handleOnCreate()
}

override fun routeToExploration(explorationId: String) {
startActivity(ExplorationActivity.createExplorationActivityIntent(this, explorationId))
}
}
32 changes: 31 additions & 1 deletion app/src/main/java/org/oppia/app/home/HomeFragmentPresenter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,41 @@ package org.oppia.app.home
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.appcompat.app.AppCompatActivity
import androidx.fragment.app.Fragment
import androidx.lifecycle.Observer
import org.oppia.app.databinding.HomeFragmentBinding
import org.oppia.app.fragment.FragmentScope
import org.oppia.app.viewmodel.ViewModelProvider
import org.oppia.domain.UserAppHistoryController
import org.oppia.domain.exploration.ExplorationDataController
import org.oppia.domain.exploration.TEST_EXPLORATION_ID_5
import org.oppia.util.data.AsyncResult
import org.oppia.util.logging.Logger
import javax.inject.Inject

private const val EXPLORATION_ID = TEST_EXPLORATION_ID_5

/** The controller for [HomeFragment]. */
@FragmentScope
class HomeFragmentPresenter @Inject constructor(
activity: AppCompatActivity,
private val fragment: Fragment,
private val viewModelProvider: ViewModelProvider<UserAppHistoryViewModel>,
private val userAppHistoryController: UserAppHistoryController
private val userAppHistoryController: UserAppHistoryController,
private val explorationDataController: ExplorationDataController,
private val logger: Logger
) {

private val routeToExplorationListener = activity as RouteToExplorationListener

fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
val binding = HomeFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
// 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 = getUserAppHistoryViewModel()
it.presenter = this
it.lifecycleOwner = fragment
}

Expand All @@ -34,4 +49,19 @@ class HomeFragmentPresenter @Inject constructor(
private fun getUserAppHistoryViewModel(): UserAppHistoryViewModel {
return viewModelProvider.getForFragment(fragment, UserAppHistoryViewModel::class.java)
}

fun playExplorationButton(v: View) {
explorationDataController.startPlayingExploration(
EXPLORATION_ID
).observe(fragment, Observer<AsyncResult<Any?>> { result ->
when {
result.isPending() -> logger.d("HomeFragment", "Loading exploration")
result.isFailure() -> logger.e("HomeFragment", "Failed to load exploration", result.getErrorOrNull()!!)
else -> {
logger.d("HomeFragment", "Successfully loaded exploration")
routeToExplorationListener.routeToExploration(EXPLORATION_ID)
}
}
})
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package org.oppia.app.home

/** Listener for when an activity should route to a exploration. */
interface RouteToExplorationListener {
fun routeToExploration(explorationId: String)
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,30 @@
package org.oppia.app.player.exploration

import android.content.Context
import android.content.Intent
import android.os.Bundle
import org.oppia.app.activity.InjectableAppCompatActivity
import javax.inject.Inject

const val EXPLORATION_ACTIVITY_TOPIC_ID_ARGUMENT_KEY = "ExplorationActivity.exploration_id"

/** The starting point for exploration. */
class ExplorationActivity : InjectableAppCompatActivity() {
@Inject lateinit var explorationActivityPresenter: ExplorationActivityPresenter
@Inject
lateinit var explorationActivityPresenter: ExplorationActivityPresenter

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
activityComponent.inject(this)
explorationActivityPresenter.handleOnCreate()
}

companion object {
/** Returns a new [Intent] to route to [ExplorationActivity] for a specified topic ID. */
fun createExplorationActivityIntent(context: Context, explorationId: String): Intent {
val intent = Intent(context, ExplorationActivity::class.java)
intent.putExtra(EXPLORATION_ACTIVITY_TOPIC_ID_ARGUMENT_KEY, explorationId)
return intent
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@ import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.fragment.app.Fragment
import androidx.lifecycle.LiveData
import androidx.lifecycle.Observer
import androidx.lifecycle.Transformations
import org.oppia.app.databinding.StateFragmentBinding
import org.oppia.app.fragment.FragmentScope
import org.oppia.app.model.CellularDataPreference
import org.oppia.app.model.EphemeralState
import org.oppia.app.player.audio.CellularDataDialogFragment
import org.oppia.app.viewmodel.ViewModelProvider
import org.oppia.domain.audio.CellularDialogController
import org.oppia.domain.exploration.ExplorationProgressController
import org.oppia.util.data.AsyncResult
import org.oppia.util.logging.Logger
import javax.inject.Inject

private const val TAG_CELLULAR_DATA_DIALOG = "CELLULAR_DATA_DIALOG"
Expand All @@ -21,15 +26,17 @@ private const val TAG_CELLULAR_DATA_DIALOG = "CELLULAR_DATA_DIALOG"
class StateFragmentPresenter @Inject constructor(
private val fragment: Fragment,
private val cellularDialogController: CellularDialogController,
private val viewModelProvider: ViewModelProvider<StateViewModel>
private val viewModelProvider: ViewModelProvider<StateViewModel>,
private val explorationProgressController: ExplorationProgressController,
private val logger: Logger
) {

private var showCellularDataDialog = true
private var useCellularData = false

fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
cellularDialogController.getCellularDataPreference()
.observe(fragment, Observer<AsyncResult<CellularDataPreference>>{
.observe(fragment, Observer<AsyncResult<CellularDataPreference>> {
if (it.isSuccess()) {
val prefs = it.getOrDefault(CellularDataPreference.getDefaultInstance())
showCellularDataDialog = !(prefs.hideDialog)
Expand All @@ -42,6 +49,9 @@ class StateFragmentPresenter @Inject constructor(
it.stateFragment = fragment as StateFragment
it.viewModel = getStateViewModel()
}

subscribeToCurrentState()

return binding.root
}

Expand Down Expand Up @@ -81,4 +91,25 @@ class StateFragmentPresenter @Inject constructor(
fun setAudioFragmentVisible(isVisible: Boolean) {
getStateViewModel().setAudioFragmentVisible(isVisible)
}

private fun subscribeToCurrentState() {
ephemeralStateLiveData.observe(fragment, Observer<EphemeralState> { result ->
logger.d("StateFragment", "getCurrentState: ${result.state.name}")
})
}

private val ephemeralStateLiveData: LiveData<EphemeralState> by lazy {
getEphemeralState()
}

private fun getEphemeralState(): LiveData<EphemeralState> {
return Transformations.map(explorationProgressController.getCurrentState(), ::processCurrentState)
}

private fun processCurrentState(ephemeralStateResult: AsyncResult<EphemeralState>): EphemeralState {
if (ephemeralStateResult.isFailure()) {
logger.e("StateFragment", "Failed to retrieve ephemeral state", ephemeralStateResult.getErrorOrNull()!!)
}
return ephemeralStateResult.getOrDefault(EphemeralState.getDefaultInstance())
}
}
9 changes: 5 additions & 4 deletions app/src/main/res/layout/exploration_fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
android:layout_width="match_parent"
android:layout_height="match_parent">

<TextView
android:id="@+id/dummy_text_view"
android:layout_width="wrap_content"
<fragment
android:id="@+id/state_fragment"
class="org.oppia.app.player.state.StateFragment"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="This is dummy TextView for testing"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent" />

</androidx.constraintlayout.widget.ConstraintLayout>
</layout>
12 changes: 6 additions & 6 deletions app/src/main/res/layout/home_activity.xml
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<FrameLayout
xmlns:android="http://schemas.android.com/apk/res/android"
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=".home.HomeActivity" />
xmlns:android="http://schemas.android.com/apk/res/android"
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=".home.HomeActivity" />
17 changes: 13 additions & 4 deletions app/src/main/res/layout/home_fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,18 @@
<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.home.UserAppHistoryViewModel"/>
<variable
name="presenter"
type="org.oppia.app.home.HomeFragmentPresenter"/>
</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"
Expand All @@ -23,6 +23,15 @@
app:layout_constraintLeft_toLeftOf="parent"
app:layout_constraintRight_toRightOf="parent"
app:layout_constraintTop_toTopOf="parent"/>

<Button
android:id="@+id/play_exploration_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="16dp"
android:onClick="@{presenter::playExplorationButton}"
android:text="Play exploration"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/welcome_text_view"/>
</androidx.constraintlayout.widget.ConstraintLayout>
</layout>
48 changes: 33 additions & 15 deletions app/src/sharedTest/java/org/oppia/app/home/HomeActivityTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import android.content.Context
import android.os.Handler
import android.os.Looper
import android.view.View
import androidx.test.espresso.matcher.ViewMatchers.withId
import androidx.test.espresso.matcher.ViewMatchers.withText
import androidx.test.core.app.ActivityScenario.launch
import androidx.test.core.app.ApplicationProvider
import androidx.test.espresso.Espresso.onIdle
Expand All @@ -14,14 +16,17 @@ import androidx.test.espresso.PerformException
import androidx.test.espresso.UiController
import androidx.test.espresso.ViewAction
import androidx.test.espresso.ViewInteraction
import androidx.test.espresso.action.ViewActions.click
import androidx.test.espresso.assertion.ViewAssertions.matches
import androidx.test.espresso.idling.CountingIdlingResource
import androidx.test.espresso.intent.Intents
import androidx.test.espresso.intent.Intents.intended
import androidx.test.espresso.intent.matcher.IntentMatchers.hasComponent
import androidx.test.espresso.matcher.ViewMatchers.isRoot
import androidx.test.espresso.matcher.ViewMatchers.withId
import androidx.test.espresso.matcher.ViewMatchers.withText
import androidx.test.espresso.util.HumanReadables
import androidx.test.espresso.util.TreeIterables
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.rule.ActivityTestRule
import dagger.BindsInstance
import dagger.Component
import dagger.Module
Expand All @@ -31,9 +36,9 @@ import kotlinx.coroutines.asCoroutineDispatcher
import org.hamcrest.Matcher
import org.junit.After
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.oppia.app.R
import org.oppia.domain.UserAppHistoryController
import org.oppia.util.logging.EnableConsoleLog
import org.oppia.util.logging.EnableFileLog
Expand All @@ -45,12 +50,16 @@ import java.util.concurrent.AbstractExecutorService
import java.util.concurrent.TimeUnit
import java.util.concurrent.TimeoutException
import javax.inject.Singleton
import org.oppia.app.R
import org.oppia.app.player.exploration.ExplorationActivity

/** Tests for [HomeActivity]. */
@RunWith(AndroidJUnit4::class)
class HomeActivityTest {

@Before
fun setUp() {
Intents.init()
IdlingRegistry.getInstance().register(MainThreadExecutor.countingResource)
simulateNewAppInstance()
}
Expand Down Expand Up @@ -78,6 +87,14 @@ class HomeActivityTest {
}
}

@Test
fun testHomeActivity_playExplorationButtonClicked_opensExplorationActivity() {
launch(HomeActivity::class.java).use {
onView(withId(R.id.play_exploration_button)).perform(click())
intended(hasComponent(ExplorationActivity::class.java.name))
}
}

private fun simulateNewAppInstance() {
// Simulate a fresh app install by clearing any potential on-disk caches using an isolated app history controller.
createTestRootComponent().getUserAppHistoryController().clearUserAppHistory()
Expand All @@ -101,12 +118,12 @@ class HomeActivityTest {
return onView(isRoot()).perform(waitForMatch(viewMatcher, 30000L))
}

// TODO(#59): Remove these waits once we can ensure that the production executors are not depended on in tests.
// Sleeping is really bad practice in Espresso tests, and can lead to test flakiness. It shouldn't be necessary if we
// use a test executor service with a counting idle resource, but right now Gradle mixes dependencies such that both
// the test and production blocking executors are being used. The latter cannot be updated to notify Espresso of any
// active coroutines, so the test attempts to assert state before it's ready. This artificial delay in the Espresso
// thread helps to counter that.
// TODO(#59): Remove these waits once we can ensure that the production executors are not depended on in tests.
// Sleeping is really bad practice in Espresso tests, and can lead to test flakiness. It shouldn't be necessary if we
// use a test executor service with a counting idle resource, but right now Gradle mixes dependencies such that both
// the test and production blocking executors are being used. The latter cannot be updated to notify Espresso of any
// active coroutines, so the test attempts to assert state before it's ready. This artificial delay in the Espresso
// thread helps to counter that.
/**
* Perform action of waiting for a specific matcher to finish. Adapted from:
* https://stackoverflow.com/a/22563297/3689782.
Expand Down Expand Up @@ -192,22 +209,23 @@ class HomeActivityTest {
interface Builder {
@BindsInstance
fun setApplication(application: Application): Builder

fun build(): TestApplicationComponent
}

fun getUserAppHistoryController(): UserAppHistoryController
}

// TODO(#59): Move this to a general-purpose testing library that replaces all CoroutineExecutors with an
// Espresso-enabled executor service. This service should also allow for background threads to run in both Espresso
// and Robolectric to help catch potential race conditions, rather than forcing parallel execution to be sequential
// and immediate.
// NB: This also blocks on #59 to be able to actually create a test-only library.
// TODO(#59): Move this to a general-purpose testing library that replaces all CoroutineExecutors with an
// Espresso-enabled executor service. This service should also allow for background threads to run in both Espresso
// and Robolectric to help catch potential race conditions, rather than forcing parallel execution to be sequential
// and immediate.
// NB: This also blocks on #59 to be able to actually create a test-only library.
/**
* An executor service that schedules all [Runnable]s to run asynchronously on the main thread. This is based on:
* https://android.googlesource.com/platform/packages/apps/TV/+/android-live-tv/src/com/android/tv/util/MainThreadExecutor.java.
*/
private object MainThreadExecutor: AbstractExecutorService() {
private object MainThreadExecutor : AbstractExecutorService() {
override fun isTerminated(): Boolean = false

private val handler = Handler(Looper.getMainLooper())
Expand Down

0 comments on commit 3db1aae

Please sign in to comment.