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

Introduce load exploration part 1 #213

Merged
merged 21 commits into from
Oct 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e19af2b
Introduce first pass interface for ExplorationProgressController.
BenHenning Sep 25, 2019
43766b0
Fill in the stubbed logic for ExplorationProgressController. Still no
BenHenning Sep 26, 2019
c8eaa66
Fix lateinit issue in ExplorationProgressController due to wrongly or…
BenHenning Sep 27, 2019
a0eb3ce
Fix a variaty of issues in the exp progress controller, properly hook…
BenHenning Sep 29, 2019
1ea9d01
Merge branch 'develop' into introduce-exploration-progress-controller
BenHenning Sep 29, 2019
41141b6
Created a separate ExplorationRetriever, hooked up
BenHenning Oct 1, 2019
ccbac0e
Merge branch 'develop' into introduce-exploration-progress-controller
BenHenning Oct 1, 2019
5f326fc
Change the locking mechanism for ExplorationProgressController to work
BenHenning Oct 3, 2019
b5b7485
Finish tests for ExplorationProgressController and add test classific…
BenHenning Oct 4, 2019
a3c4667
Merge branch 'develop' into introduce-exploration-progress-controller
BenHenning Oct 4, 2019
4a9aad1
ObservableViewModel
Oct 7, 2019
38c0275
Merge remote-tracking branch 'upstream/introduce-exploration-progress…
Oct 7, 2019
0705586
Load exploration and open ExplorationActivity
Oct 7, 2019
d38bbe1
Test case for load exploration
Oct 7, 2019
0fb1729
Connection with StateFragment
Oct 7, 2019
309f3ba
Resolve merge conflicts
Oct 9, 2019
7045d7e
Nit changes self review
Oct 9, 2019
b683638
Refactor home
Oct 9, 2019
83afead
Resolve merge conflicts
Oct 9, 2019
155b83c
Test case updated
Oct 9, 2019
8e28b94
Resolve merge conflicts
Oct 9, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()!!)
rt4914 marked this conversation as resolved.
Show resolved Hide resolved
}
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