Skip to content

Commit

Permalink
Fix #159: Introduce question player UI structure (#500)
Browse files Browse the repository at this point in the history
* Initial check in for the question data controller

* Removed progress controller test

* Address the renames suggested in #217.

* Introduce thoroughly stubbed QuestionAssessmentProgressController +
empty test suite.

* Review changes

* Review changes

* Pass data providers to the assessment controller, and set caps in the training controller

* Removed unnecessary imports/variables

* Add fake question data for stubbed interfaces

* Remove duplicate questions while fetching in training controller

* Comment explaining the filter function

* Improve duplicate checking - check it while filtering instead of after filtering

* Add linked skill id values

* Review changes

* add a new module for questions constants

* Review changes

* add a test to verify questions were fetched properly

* reformatted code

* Re-add QuestionTrainingController removed in merge.

* Finalize question progress controller interface.

* Implement QuestionAssessmentProgressController.

This includes some basic refactoring of internal structures used by the
exploration progress controller to share common functionality between the
two progress controllers. There's still some duplication, but this seems
like a reasonable split since there's likely to be further differences in
the progress controllers in the future.

The question assessment progress controller tests pass, but no new ones
have yet been added to thoroughly test the implementation.

* Add initial phase of tests for the progress controller based on
ExplorationProgressController. They haven't yet been verified as correct.

* Fix typo in QuestionTrainingController.

* Post-merge fixes and adjustments.

* Fix broken ProfileManagementControllerTest.

* Post-merge fixes.

* Update tests to build, but not pass.

* Started UI

* setup ViewModel

* added binding adapter

* Handles basic functionality

* Moved code around

* Added replay button

* Make questions partially work.

This introduces back button behavior to ensure training sessions can be
reset.

This also attempts to introduce proper asset loading, but it doesn't
seem to be working currently.

* Temporarily remove check in StateDeck that breaks questions.

* Ensure all state navigation buttons are on the same line and the same
size.

This required reworking how nav button binding works in the state
fragment such that multiple models are used depending on which type of
navigation scenario the fragment is currently in.

* Fix dp adjustment that caused previous button to disappear.

Fix previous button showing up twice for continue interaction.

* Undo .idea/misc.xml change that snuck in.

* Update & fix StateFragmentTest (at least for Espresso). See #495 description
for details on the changes necessary to make these tests work.

* Post-merge fixes with some temporary stop-gap solutions.

* Fix questions image rendering and consolidate GCS resource bucket names.

* Fix question session not properly being reinitialized for new sessions.

Also, ensure that notifications do not regenerate the training session.

* Fix index accounting for questions. Before, the index incremented upon
correct answer submission rather than navigation.

* Remove support for backward navigation in questions since it isn't
allowed.

* Set title of question activity to 'Train Mode'.

* Add support for a synthetic ephemeral question to represent the end of a
training session.

* Update question progress tracking to be translatable and to ensure that
the synthetic terminal state shows the correct progress: 'completed'.

* Move most state player functional support into a new configurable
assembler class to enable sharing between the exploration and question
players.

* Hook up StatePlayerRecyclerViewAssembler to the question player fragment
presenter. This also enables the congratulations text.

* Introduce proper terminal page and disable overscroll. Note that the end
session page introduced here has not been mocked yet, so it's mostly a
placeholder until a mock is available.

* Add support & button for training session replay.

* Update/add TODOs to correspond to GitHub tracked issues.

* Fix broken import.

* Finish tests for Question{Training,AssessmentProgress}Controllers.

Note that the nature of the assessment progress controller is such that
a new, dynamic DataProvider is needed since the data provider for the
current assessment question is actually a transformation of different
data providers depending on which questions are passed to the
controller.

* Initial introduction of test coroutine dispatchers to replace Kotlin's
test coroutine dispatcher.

This includes introducing a test-only module to contain testing
dependencies.

* Introduce a new LiveData mechanism to bridge coroutines from
DataProviders in a way that doesn't have the same limitations as the
previous MutableLiveData-based bridge.

* Introduce new nested data provider that effectively allows a
DataProvider to be set up like transformAsync() but with the ability to
change the root DataProvider.

* question player progress-bar hifi

* nit

* revert

* revert

* Early work at introducing FakeSystemClock tests (not yet complete).

* Post-merge fixes.

* Fix the submit button disabled states: with the new refactor, the submit
button is now properly disabled when an invalid answer is pending.

* Revert accidental changes to .idea/misc.xml.

* Address reviewer comments.

* Remove unnecessary meta file for Mockito.

* Minor import cleanup.

* Post-merge fixes.

* Fix audio playback & generalize it to facilitate enabling it for
questions in the future.

* Address reviewer comments & fix broken tests.

* Fix hints & solutions functionality after simplification (some bugs &
regressions were introduced due to the simplification; these have been
fixed).

* Revert the assembler delays to match the intended durations rather than
the test-only values used for development.

* Address reviewer comments.

* Remove infeasible testing structures, add documentation, and clean up
implementation to prepare for code review.

* Add notice that the dispatchers utility is temporary.

* Cleanup new LiveData bridge, add tests for it, and migrate other
DataProviders tests to using TestCoroutineDispatchers utility.

* Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments.

* Use ktlint to reformat TestCoroutineDispatchers per reviewer comment
thread.

* Reformat files failing linter check.

* Reformat new DataProviders code.

* Add new tests for the NestedTransformedDataProvider.

* Address reviewer comment.

* Address reviewer comments.

* Address reviewer comments & fix lint issue.

* Update hints & solutions handling to more closely follow the intended
behavior graph (and added an actual visual representation of this graph
in code to simplify maintaining the solution in the future). Tests will
be added as part of solving #1273.

Co-authored-by: vinitamurthi <[email protected]>
Co-authored-by: James Xu <[email protected]>
Co-authored-by: marysoloman <[email protected]>
  • Loading branch information
4 people authored Jun 10, 2020
1 parent 7834ff9 commit 07c4ee8
Show file tree
Hide file tree
Showing 63 changed files with 1,994 additions and 753 deletions.
3 changes: 2 additions & 1 deletion app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@
android:theme="@style/OppiaThemeWithoutActionBar" />
<activity
android:name=".topic.questionplayer.QuestionPlayerActivity"
android:screenOrientation="portrait" />
android:screenOrientation="portrait"
android:label="@string/question_player_title" />
<activity
android:name=".topic.revisioncard.RevisionCardActivity"
android:theme="@style/OppiaThemeWithoutActionBar" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import org.oppia.domain.classify.rules.multiplechoiceinput.MultipleChoiceInputMo
import org.oppia.domain.classify.rules.numberwithunits.NumberWithUnitsRuleModule
import org.oppia.domain.classify.rules.numericinput.NumericInputRuleModule
import org.oppia.domain.classify.rules.textinput.TextInputRuleModule
import org.oppia.domain.question.QuestionModule
import org.oppia.util.caching.CachingModule
import org.oppia.util.gcsresource.GcsResourceModule
import org.oppia.util.logging.LoggerModule
Expand All @@ -28,10 +29,11 @@ import javax.inject.Singleton
@Singleton
@Component(modules = [
ApplicationModule::class, DispatcherModule::class, NetworkModule::class, LoggerModule::class,
ContinueModule::class, FractionInputModule::class, ItemSelectionInputModule::class, MultipleChoiceInputModule::class,
NumberWithUnitsRuleModule::class, NumericInputRuleModule::class, TextInputRuleModule::class,
DragDropSortInputModule::class, InteractionsModule::class, GcsResourceModule::class, GlideImageLoaderModule::class,
ImageParsingModule::class, HtmlParserEntityTypeModule::class, CachingModule::class
ContinueModule::class, FractionInputModule::class, ItemSelectionInputModule::class,
MultipleChoiceInputModule::class, NumberWithUnitsRuleModule::class, NumericInputRuleModule::class,
TextInputRuleModule::class, DragDropSortInputModule::class, InteractionsModule::class,
GcsResourceModule::class, GlideImageLoaderModule::class, ImageParsingModule::class,
HtmlParserEntityTypeModule::class, CachingModule::class, QuestionModule::class
])
interface ApplicationComponent {
@Component.Builder
Expand Down
13 changes: 10 additions & 3 deletions app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ class AudioFragment : InjectableFragment(), LanguageInterface, AudioUiManager,
audioFragmentPresenter.handleOnDestroy()
}

override fun enableAudioPlayback(contentId: String?) {
audioFragmentPresenter.handleAudioClick(
shouldEnableAudioPlayback = true, feedbackId = contentId
)
}

override fun disableAudioPlayback() {
audioFragmentPresenter.handleAudioClick(shouldEnableAudioPlayback = false, feedbackId = null)
}

override fun setStateAndExplorationId(newState: State, explorationId: String) =
audioFragmentPresenter.setStateAndExplorationId(newState, explorationId)

Expand All @@ -86,7 +96,4 @@ class AudioFragment : InjectableFragment(), LanguageInterface, AudioUiManager,

/** Used in data binding to know position of user's touch */
fun getUserPosition() = audioFragmentPresenter.userProgress

fun handleAudioClick(isShowing: Boolean, feedbackId: String?) =
audioFragmentPresenter.handleAudioClick(isShowing, feedbackId)
}
Original file line number Diff line number Diff line change
Expand Up @@ -196,11 +196,9 @@ class AudioFragmentPresenter @Inject constructor(
}
}

fun handleAudioClick(isShowing: Boolean, feedbackId: String?) {
fun handleAudioClick(shouldEnableAudioPlayback: Boolean, feedbackId: String?) {
this.feedbackId = feedbackId
if (isShowing) {
setAudioFragmentVisible(false)
} else {
if (shouldEnableAudioPlayback) {
when (networkConnectionUtil.getCurrentConnectionStatus()) {
NetworkConnectionUtil.ConnectionStatus.LOCAL -> setAudioFragmentVisible(true)
NetworkConnectionUtil.ConnectionStatus.CELLULAR -> {
Expand All @@ -220,6 +218,8 @@ class AudioFragmentPresenter @Inject constructor(
setAudioFragmentVisible(false)
}
}
} else {
setAudioFragmentVisible(false)
}
}

Expand Down
10 changes: 10 additions & 0 deletions app/src/main/java/org/oppia/app/player/audio/AudioUiManager.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ import org.oppia.app.model.State

/** Manager for updating audio state within the state player. */
interface AudioUiManager {
/**
* Enables audio playback with the specified content ID as the initial content to play. Note that
* this corresponds to a response to the UI which means the action may fail with a dialog
* notifying the user why they can't enable audio playback right now.
*/
fun enableAudioPlayback(contentId: String?)

/** Disables audio playback, stopping any currently playing tracks. */
fun disableAudioPlayback();

/**
* Used to set the state and explorationId for the audio player
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import org.oppia.domain.audio.AudioPlayerController
import org.oppia.domain.audio.AudioPlayerController.PlayProgress
import org.oppia.domain.audio.AudioPlayerController.PlayStatus
import org.oppia.util.data.AsyncResult
import org.oppia.util.gcsresource.DefaultResource
import org.oppia.util.gcsresource.DefaultResourceBucketName
import java.util.Locale
import javax.inject.Inject

Expand All @@ -22,7 +22,7 @@ import javax.inject.Inject
class AudioViewModel @Inject constructor(
private val audioPlayerController: AudioPlayerController,
private val fragment: Fragment,
@DefaultResource private val gcsResource: String
@DefaultResourceBucketName private val gcsResource: String
) : ViewModel() {

private lateinit var state: State
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ import org.oppia.app.player.state.hintsandsolution.RevealHintListener
import org.oppia.app.player.state.hintsandsolution.RevealSolutionInterface
import org.oppia.app.player.state.listener.RouteToHintsAndSolutionListener
import org.oppia.app.player.state.listener.StateKeyboardButtonListener
import org.oppia.app.player.stopexploration.StopExplorationDialogFragment
import org.oppia.app.player.stopexploration.StopExplorationInterface
import org.oppia.app.player.stopplaying.StopExplorationDialogFragment
import org.oppia.app.player.stopplaying.StopStatePlayingSessionListener
import javax.inject.Inject

private const val TAG_STOP_EXPLORATION_DIALOG = "STOP_EXPLORATION_DIALOG"
const val TAG_HINTS_AND_SOLUTION_DIALOG = "HINTS_AND_SOLUTION_DIALOG"

/** The starting point for exploration. */
class ExplorationActivity : InjectableAppCompatActivity(), StopExplorationInterface, StateKeyboardButtonListener,
class ExplorationActivity : InjectableAppCompatActivity(), StopStatePlayingSessionListener, StateKeyboardButtonListener,
AudioButtonListener, HintsAndSolutionListener, RouteToHintsAndSolutionListener, RevealHintListener,
RevealSolutionInterface {

Expand Down Expand Up @@ -83,7 +83,7 @@ class ExplorationActivity : InjectableAppCompatActivity(), StopExplorationInterf
dialogFragment.showNow(supportFragmentManager, TAG_STOP_EXPLORATION_DIALOG)
}

override fun stopExploration() {
override fun stopSession() {
explorationActivityPresenter.stopExploration()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,15 @@ class ExplorationActivityPresenter @Inject constructor(
})
}

fun onKeyboardAction(actionCode: Int) {
if (actionCode == EditorInfo.IME_ACTION_DONE) {
val explorationFragment = activity.supportFragmentManager.findFragmentByTag(
TAG_EXPLORATION_FRAGMENT
) as? ExplorationFragment
explorationFragment?.onKeyboardAction()
}
}

private fun updateToolbarTitle(explorationId: String) {
subscribeToExploration(explorationDataController.getExplorationById(explorationId))
}
Expand Down Expand Up @@ -152,14 +161,6 @@ class ExplorationActivityPresenter @Inject constructor(
}
}

fun onKeyboardAction(actionCode: Int) {
if (actionCode == EditorInfo.IME_ACTION_DONE) {
val explorationFragment =
activity.supportFragmentManager.findFragmentByTag(TAG_EXPLORATION_FRAGMENT) as ExplorationFragment
explorationFragment.onKeyboardAction()
}
}

fun revealHint(saveUserChoice: Boolean, hintIndex: Int) {
val explorationFragment =
activity.supportFragmentManager.findFragmentByTag(TAG_EXPLORATION_FRAGMENT) as ExplorationFragment
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ class ExplorationFragmentPresenter @Inject constructor(

fun scrollToTop() = getStateFragment()?.scrollToTop()

private fun getStateFragment(): StateFragment? {
return fragment.childFragmentManager.findFragmentById(R.id.state_fragment_placeholder) as StateFragment?
}

fun onKeyboardAction() {
getStateFragment()?.handleKeyboardAction()
}
Expand All @@ -57,4 +53,8 @@ class ExplorationFragmentPresenter @Inject constructor(
fun revealSolution(saveUserChoice: Boolean){
getStateFragment()?.revealSolution(saveUserChoice)
}

private fun getStateFragment(): StateFragment? {
return fragment.childFragmentManager.findFragmentById(R.id.state_fragment_placeholder) as StateFragment?
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import org.oppia.app.databinding.MultipleChoiceInteractionItemsBinding
import org.oppia.app.fragment.InjectableFragment
import org.oppia.app.player.state.itemviewmodel.SelectionInteractionContentViewModel
import org.oppia.app.recyclerview.BindableAdapter
import org.oppia.util.gcsresource.DefaultResourceBucketName
import org.oppia.util.parser.ExplorationHtmlParserEntityType
import org.oppia.util.parser.HtmlParser
import javax.inject.Inject
Expand All @@ -37,7 +38,11 @@ class SelectionInteractionView @JvmOverloads constructor(
@Inject
@field:ExplorationHtmlParserEntityType
lateinit var entityType: String
private lateinit var explorationId: String
@Inject
@field:DefaultResourceBucketName
lateinit var resourceBucketName: String

private lateinit var entityId: String

override fun onAttachedToWindow() {
super.onAttachedToWindow()
Expand All @@ -55,8 +60,8 @@ class SelectionInteractionView @JvmOverloads constructor(

// TODO(#264): Clean up HTML parser such that it can be handled completely through a binding adapter, allowing
// TextViews that require custom Oppia HTML parsing to be fully automatically bound through data-binding.
fun setExplorationId(explorationId: String) {
this.explorationId = explorationId
fun setEntityId(entityId: String) {
this.entityId = entityId
}

private fun createAdapter(): BindableAdapter<SelectionInteractionContentViewModel> {
Expand All @@ -72,7 +77,9 @@ class SelectionInteractionView @JvmOverloads constructor(
bindView = { view, viewModel ->
val binding = DataBindingUtil.findBinding<ItemSelectionInteractionItemsBinding>(view)!!
binding.htmlContent =
htmlParserFactory.create(entityType, explorationId, /* imageCenterAlign= */ false).parseOppiaHtml(
htmlParserFactory.create(
resourceBucketName, entityType, entityId, /* imageCenterAlign= */ false
).parseOppiaHtml(
viewModel.htmlContent, binding.itemSelectionContentsTextView
)
binding.viewModel = viewModel
Expand All @@ -90,7 +97,9 @@ class SelectionInteractionView @JvmOverloads constructor(
bindView = { view, viewModel ->
val binding = DataBindingUtil.findBinding<MultipleChoiceInteractionItemsBinding>(view)!!
binding.htmlContent =
htmlParserFactory.create(entityType, explorationId, /* imageCenterAlign= */ false).parseOppiaHtml(
htmlParserFactory.create(
resourceBucketName, entityType, entityId, /* imageCenterAlign= */ false
).parseOppiaHtml(
viewModel.htmlContent, binding.multipleChoiceContentTextView
)
binding.viewModel = viewModel
Expand All @@ -108,7 +117,7 @@ fun setAllOptionsItemInputType(
) = selectionInteractionView.setAllOptionsItemInputType(selectionItemInputType)

/** Sets the exploration ID for a specific [SelectionInteractionView] via data-binding. */
@BindingAdapter("explorationId")
fun setExplorationId(
selectionInteractionView: SelectionInteractionView, explorationId: String
) = selectionInteractionView.setExplorationId(explorationId)
@BindingAdapter("entityId")
fun setEntityId(
selectionInteractionView: SelectionInteractionView, entityId: String
) = selectionInteractionView.setEntityId(entityId)
10 changes: 9 additions & 1 deletion app/src/main/java/org/oppia/app/player/state/StateFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,25 @@ import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import org.oppia.app.fragment.InjectableFragment
import org.oppia.app.model.HelpIndex
import org.oppia.app.model.UserAnswer
import org.oppia.app.player.state.answerhandling.InteractionAnswerErrorReceiver
import org.oppia.app.player.state.answerhandling.InteractionAnswerHandler
import org.oppia.app.player.state.answerhandling.InteractionAnswerReceiver
import org.oppia.app.player.state.listener.ContinueNavigationButtonListener
import org.oppia.app.player.state.listener.NextNavigationButtonListener
import org.oppia.app.player.state.listener.PreviousNavigationButtonListener
import org.oppia.app.player.state.listener.PreviousResponsesHeaderClickListener
import org.oppia.app.player.state.listener.ReturnToTopicNavigationButtonListener
import org.oppia.app.player.state.listener.ShowHintAvailabilityListener
import org.oppia.app.player.state.listener.SubmitNavigationButtonListener
import javax.inject.Inject

/** Fragment that represents the current state of an exploration. */
class StateFragment : InjectableFragment(), InteractionAnswerReceiver, InteractionAnswerHandler,
InteractionAnswerErrorReceiver, ContinueNavigationButtonListener, NextNavigationButtonListener,
PreviousNavigationButtonListener, ReturnToTopicNavigationButtonListener, SubmitNavigationButtonListener {
PreviousNavigationButtonListener, ReturnToTopicNavigationButtonListener, SubmitNavigationButtonListener,
PreviousResponsesHeaderClickListener, ShowHintAvailabilityListener {
companion object {
/**
* Creates a new instance of a StateFragment.
Expand Down Expand Up @@ -78,6 +82,10 @@ class StateFragment : InjectableFragment(), InteractionAnswerReceiver, Interacti

override fun onSubmitButtonClicked() = stateFragmentPresenter.onSubmitButtonClicked()

override fun onResponsesHeaderClicked() = stateFragmentPresenter.onResponsesHeaderClicked()

override fun onHintAvailable(helpIndex: HelpIndex) = stateFragmentPresenter.onHintAvailable(helpIndex)

fun handlePlayAudio() = stateFragmentPresenter.handleAudioClick()

fun handleKeyboardAction() = stateFragmentPresenter.handleKeyboardAction()
Expand Down
Loading

0 comments on commit 07c4ee8

Please sign in to comment.