Skip to content

Commit

Permalink
Fix #259 and part of #163: Finish state fragment overall UI structure (
Browse files Browse the repository at this point in the history
…#270)

* Attempt to rebase content-card onto develop.

* Manually apply rejections and remove rej files.

* Add missing pieces implied in merge-fix that are needed for this branch.

* Maunally apply multiple-single-input-interaction changes to develop
branch (with patch rejections).

* Manually apply rejected diffs.

* Fix broken build after merges.

* Add missing changes from merge-fix that are needed to enable state
content binding.

* Resolve unresolved conflicts.

* Make an exploration with all prototype interactions work end-to-end.

This includes:
- Introducing an exploration with all prototype interactions
- Introducing support for binding all interaction types
- Fixing some parsing issues with fractions
- Rewiring the interaction object creation for views to be generic
- Removing temporary answer handling everywhere
- Other random fixes

This includes no tests, and there are a few other fixes still necessary
for the core learning experience to fully work.

* Add support for displaying feedback items.

* Add support for reshowing submitted answers.

* Make 'Continue' button a separate interaction (but keep the existing
general 'Continue' navigation button). Rework how navigation buttons
behave. Introduce automatic answer submission for some interactions
(Continue, ItemSelection if it shows radio buttons, and MultipleChoice).

* Move audio functionality to a custom toolbar, remove the dummy audio
button, and remove the transient state name at the top of the state
fragment.

* Simplify the presenter by updating ExplorationProgressController to feed
information about the next and current state to allow the state presenter
to only focus on rendering the current state.

* First batch of work corresponding to moving the entirety of state
fragment's recycler view (and child views/recycler view) to be bound via
data-binding.

* Finish updating item selection recycler view to use data binding. This
required introducing a ViewComponent that inherits from FragmentComponent
in the Dagger dependency graph. This also involved updating the app to use
a newer AndroidX fragment library that's still pre-release in order to
gain access to the necessary functionality to associate views with their
fragments.

The two custom adapters have been removed in this change.

* Some minor fixes to polish up the experience a bit and reduce jank.

* Fix broken voiceover audio button.

* Address one TODO to generalize adding interactions by leveraging a Dagger
module.

* Fix broken test.

* Fix broken domain tests by allowing feedback to be either a string or an
object, and loading about_oppia instead of oppia_exploration since the
former is expected by ExplorationProgressControllerTest.

* Fix broken app test builds (tests still fail, but at least run now).

* Add TODO to fix feedback parsing hack.

* Remove TODOs in StringToFractionParser.

* Resolve open TODOs for the state portion of the player, or associate
them with issues.

* Associate 2 more TODOs with issues.

* Add tests for new next state behavior in ExplorationProgressController,
and resolve last unattributed TODO.

* Remove number with units support.

* Post-merge fixes and clean-ups.

* Post-merge fix.

* Remove number with units tests.

* First round of addressing reviewer comments.

* Remove view scope & component.

* Second round of addressing reviewer comments. Includes a project-wide
optimization of all imports.

* Round 3 of addressing reviewer changes: removed UrlImageParserTest since
it was pulled in with earlier, unfinalized changes and we have since
realized it's not possible to test the image parser based on the current
project setup.

* Round 4 of addressing reviewer comments: reformat all layout XML files.

* Round 5 of addressing reviewer comments.

* Revert "Remove view scope & component."

This reverts commit 99783d5.

* Post-clean up fixes and other minor adjustments.

* Ensure ExplorationActivityTest passes even though it doesn't have
interesting tests yet.

* Temporary workaround to make the prototype exploration accessible with
recent topic & home fragment changes.

Also, hide the topic button in the home fragment since the topic is
accessible directly via the home fragment tiles.

* Round 6 of addressing review comments.

* Update prototype exploration to provide feedback after the multi-item
item selection state for the correct answer so that the player doesn't
seem broken.
  • Loading branch information
BenHenning authored Nov 7, 2019
1 parent 2944a67 commit a23de65
Show file tree
Hide file tree
Showing 195 changed files with 2,570 additions and 1,527 deletions.
6 changes: 5 additions & 1 deletion app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ dependencies {
'androidx.appcompat:appcompat:1.0.2',
'androidx.constraintlayout:constraintlayout:1.1.3',
'androidx.core:core-ktx:1.0.2',
"androidx.fragment:fragment:$fragment_version",
'androidx.lifecycle:lifecycle-runtime-ktx:2.2.0-alpha03',
'androidx.multidex:multidex:2.0.1',
'androidx.recyclerview:recyclerview:1.0.0',
Expand All @@ -72,6 +73,7 @@ dependencies {
"org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version",
'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.2.1',
'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.2.1',
'org.mockito:mockito-core:2.7.22',
)
testImplementation(
'androidx.test:core:1.2.0',
Expand All @@ -82,6 +84,7 @@ dependencies {
'com.google.truth:truth:0.43',
'org.robolectric:robolectric:4.3',
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
'org.mockito:mockito-core:2.7.22',
)
androidTestImplementation(
'androidx.test:core:1.2.0',
Expand All @@ -91,7 +94,8 @@ dependencies {
'androidx.test.ext:junit:1.1.1',
'androidx.test:runner:1.2.0',
'com.google.truth:truth:0.43',
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2'
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
'org.mockito:mockito-android:2.7.22',
)
androidTestUtil(
'androidx.test:orchestrator:1.2.0',
Expand Down
4 changes: 3 additions & 1 deletion app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
<activity android:name=".testing.ContinuePlayingFragmentTestActivity" />
<activity android:name=".home.HomeActivity" />
<activity android:name=".player.audio.testing.AudioFragmentTestActivity" />
<activity android:name=".player.exploration.ExplorationActivity" />
<activity
android:name=".player.exploration.ExplorationActivity"
android:theme="@style/OppiaThemeWithoutActionBar" />
<activity android:name=".player.state.testing.StateFragmentTestActivity" />
<activity android:name=".profile.ProfileActivity" />
<activity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import androidx.appcompat.app.AppCompatActivity
import dagger.BindsInstance
import dagger.Subcomponent
import org.oppia.app.fragment.FragmentComponent
import org.oppia.app.home.continueplaying.ContinuePlayingActivity
import org.oppia.app.home.HomeActivity
import org.oppia.app.home.continueplaying.ContinuePlayingActivity
import org.oppia.app.player.audio.testing.AudioFragmentTestActivity
import org.oppia.app.player.exploration.ExplorationActivity
import org.oppia.app.player.state.testing.StateFragmentTestActivity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ import javax.inject.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, InteractionsModule::class,
GcsResourceModule::class, ImageParsingModule::class, GlideImageLoaderModule::class,
NumberWithUnitsRuleModule::class, NumericInputRuleModule::class, TextInputRuleModule::class,
InteractionsModule::class, GcsResourceModule::class, GlideImageLoaderModule::class, ImageParsingModule::class,
HtmlParserEntityTypeModule::class
])
interface ApplicationComponent {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package org.oppia.app.customview.interaction
import android.content.Context
import android.util.AttributeSet
import android.widget.EditText
import org.oppia.app.model.InteractionObject
import org.oppia.app.parser.StringToFractionParser

// TODO(#249): These are the attributes which should be defined in XML, that are required for this interaction view to work correctly
// digits="0123456789/-"
Expand All @@ -18,13 +16,4 @@ class FractionInputInteractionView @JvmOverloads constructor(
context: Context,
attrs: AttributeSet? = null,
defStyle: Int = android.R.attr.editTextStyle
) : EditText(context, attrs, defStyle), InteractionAnswerRetriever {

override fun getPendingAnswer(): InteractionObject {
val interactionObjectBuilder = InteractionObject.newBuilder()
if (!text.isNullOrEmpty()) {
interactionObjectBuilder.fraction = StringToFractionParser().getFractionFromString(text = text.toString())
}
return interactionObjectBuilder.build()
}
}
) : EditText(context, attrs, defStyle)

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package org.oppia.app.customview.interaction
import android.content.Context
import android.util.AttributeSet
import android.widget.EditText
import org.oppia.app.model.InteractionObject

// TODO(#249): These are the attributes which should be defined in XML, that are required for this interaction view to work correctly
// digits="0123456789."
Expand All @@ -17,13 +16,4 @@ class NumericInputInteractionView @JvmOverloads constructor(
context: Context,
attrs: AttributeSet? = null,
defStyle: Int = android.R.attr.editTextStyle
) : EditText(context, attrs, defStyle), InteractionAnswerRetriever {

override fun getPendingAnswer(): InteractionObject {
val interactionObjectBuilder = InteractionObject.newBuilder()
if (!text.isNullOrEmpty()) {
interactionObjectBuilder.real = text.toString().toDouble()
}
return interactionObjectBuilder.build()
}
}
) : EditText(context, attrs, defStyle)
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package org.oppia.app.customview.interaction
import android.content.Context
import android.util.AttributeSet
import android.widget.EditText
import org.oppia.app.model.InteractionObject

// TODO(#249): These are the attributes which should be defined in XML, that are required for this interaction view to work correctly
// hint="Write here."
Expand All @@ -16,13 +15,4 @@ class TextInputInteractionView @JvmOverloads constructor(
context: Context,
attrs: AttributeSet? = null,
defStyle: Int = android.R.attr.editTextStyle
) : EditText(context, attrs, defStyle), InteractionAnswerRetriever {

override fun getPendingAnswer(): InteractionObject {
val interactionObjectBuilder = InteractionObject.newBuilder()
if (!text.isNullOrEmpty()) {
interactionObjectBuilder.normalizedString = text.toString()
}
return interactionObjectBuilder.build()
}
}
) : EditText(context, attrs, defStyle)
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ package org.oppia.app.databinding
import android.widget.ImageView
import androidx.annotation.DrawableRes
import androidx.databinding.BindingAdapter
import com.bumptech.glide.request.RequestOptions
import com.bumptech.glide.Glide
import com.bumptech.glide.request.RequestOptions
import org.oppia.app.R
import org.oppia.app.model.LessonThumbnailGraphic

Expand Down
9 changes: 7 additions & 2 deletions app/src/main/java/org/oppia/app/fragment/FragmentComponent.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ package org.oppia.app.fragment
import androidx.fragment.app.Fragment
import dagger.BindsInstance
import dagger.Subcomponent
import org.oppia.app.home.continueplaying.ContinuePlayingFragment
import org.oppia.app.home.HomeFragment
import org.oppia.app.home.continueplaying.ContinuePlayingFragment
import org.oppia.app.player.audio.AudioFragment
import org.oppia.app.player.exploration.ExplorationFragment
import org.oppia.app.player.state.StateFragment
import org.oppia.app.player.state.itemviewmodel.InteractionViewModelModule
import org.oppia.app.profile.AddProfileFragment
import org.oppia.app.profile.AdminAuthFragment
import org.oppia.app.profile.ProfileChooserFragment
Expand All @@ -20,9 +21,11 @@ import org.oppia.app.topic.play.TopicPlayFragment
import org.oppia.app.topic.questionplayer.QuestionPlayerFragment
import org.oppia.app.topic.review.TopicReviewFragment
import org.oppia.app.topic.train.TopicTrainFragment
import org.oppia.app.view.ViewComponent
import javax.inject.Provider

/** Root subcomponent for all fragments. */
@Subcomponent
@Subcomponent(modules = [FragmentModule::class, InteractionViewModelModule::class])
@FragmentScope
interface FragmentComponent {
@Subcomponent.Builder
Expand All @@ -33,6 +36,8 @@ interface FragmentComponent {
fun build(): FragmentComponent
}

fun getViewComponentBuilderProvider(): Provider<ViewComponent.Builder>

fun inject(addProfileFragment: AddProfileFragment)
fun inject(adminAuthFragment: AdminAuthFragment)
fun inject(audioFragment: AudioFragment)
Expand Down
7 changes: 7 additions & 0 deletions app/src/main/java/org/oppia/app/fragment/FragmentModule.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package org.oppia.app.fragment

import dagger.Module
import org.oppia.app.view.ViewComponent

/** Root fragment module. */
@Module(subcomponents = [ViewComponent::class]) class FragmentModule
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ abstract class InjectableDialogFragment: DialogFragment() {
*/
lateinit var fragmentComponent: FragmentComponent

override fun onAttach(context: Context?) {
override fun onAttach(context: Context) {
super.onAttach(context)
fragmentComponent = (requireActivity() as InjectableAppCompatActivity).createFragmentComponent(this)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package org.oppia.app.fragment

import android.content.Context
import android.view.View
import androidx.fragment.app.Fragment
import org.oppia.app.activity.InjectableAppCompatActivity
import org.oppia.app.view.ViewComponent

/**
* A fragment that facilitates field injection to children. This fragment can only be used with
Expand All @@ -16,8 +18,12 @@ abstract class InjectableFragment: Fragment() {
*/
lateinit var fragmentComponent: FragmentComponent

override fun onAttach(context: Context?) {
override fun onAttach(context: Context) {
super.onAttach(context)
fragmentComponent = (requireActivity() as InjectableAppCompatActivity).createFragmentComponent(this)
}

fun createViewComponent(view: View): ViewComponent {
return fragmentComponent.getViewComponentBuilderProvider().get().setView(view).build()
}
}
2 changes: 1 addition & 1 deletion app/src/main/java/org/oppia/app/home/HomeFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import javax.inject.Inject
class HomeFragment : InjectableFragment(), TopicSummaryClickListener {
@Inject lateinit var homeFragmentPresenter: HomeFragmentPresenter

override fun onAttach(context: Context?) {
override fun onAttach(context: Context) {
super.onAttach(context)
fragmentComponent.inject(this)
}
Expand Down
5 changes: 3 additions & 2 deletions app/src/main/java/org/oppia/app/home/HomeFragmentPresenter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ import org.oppia.app.model.TopicSummary
import org.oppia.app.model.UserAppHistory
import org.oppia.domain.UserAppHistoryController
import org.oppia.domain.exploration.ExplorationDataController
import org.oppia.domain.exploration.TEST_EXPLORATION_ID_5
import org.oppia.domain.exploration.TEST_EXPLORATION_ID_30
import org.oppia.domain.topic.TEST_TOPIC_ID_0
import org.oppia.domain.topic.TopicListController
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
private const val EXPLORATION_ID = TEST_EXPLORATION_ID_30
private const val TAG_HOME_FRAGMENT = "HomeFragment"

/** The presenter for [HomeFragment]. */
Expand Down Expand Up @@ -82,6 +82,7 @@ class HomeFragmentPresenter @Inject constructor(
}

fun playExplorationButton(v: View) {
explorationDataController.stopPlayingExploration()
explorationDataController.startPlayingExploration(
EXPLORATION_ID
).observe(fragment, Observer<AsyncResult<Any?>> { result ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import javax.inject.Inject
class ContinuePlayingFragment : InjectableFragment(), OngoingStoryClickListener {
@Inject lateinit var continuePlayingFragmentPresenter: ContinuePlayingFragmentPresenter

override fun onAttach(context: Context?) {
override fun onAttach(context: Context) {
super.onAttach(context)
fragmentComponent.inject(this)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import androidx.appcompat.app.AppCompatActivity
import androidx.databinding.ObservableField
import androidx.lifecycle.LiveData
import androidx.lifecycle.ViewModel
import org.oppia.app.home.continueplaying.ContinuePlayingActivity
import org.oppia.app.home.HomeItemViewModel
import org.oppia.app.home.RouteToContinuePlayingListener
import org.oppia.app.home.RouteToTopicPlayStoryListener
import org.oppia.app.home.continueplaying.ContinuePlayingActivity
import org.oppia.app.model.PromotedStory
import org.oppia.app.topic.TopicActivity

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package org.oppia.app.home.topiclist
import android.graphics.Color
import android.view.View
import androidx.annotation.ColorInt
import androidx.lifecycle.ViewModel
import org.oppia.app.home.HomeItemViewModel
import org.oppia.app.model.TopicSummary

Expand Down
72 changes: 47 additions & 25 deletions app/src/main/java/org/oppia/app/parser/StringToFractionParser.kt
Original file line number Diff line number Diff line change
@@ -1,34 +1,56 @@
package org.oppia.app.parser

import org.oppia.app.model.Fraction
import org.oppia.domain.util.normalizeWhitespace

/** This class contains method that helps to parse string to fraction. */
class StringToFractionParser {
private val wholeNumberOnlyRegex = """^-? ?(\d+)$""".toRegex()
private val fractionOnlyRegex = """^-? ?(\d+) ?/ ?(\d)+$""".toRegex()
private val mixedNumberRegex = """^-? ?(\d)+ ?(\d+) ?/ ?(\d)+$""".toRegex()

fun getFractionFromString(text: String): Fraction {
var inputText: String = text
var isNegative = false
var numerator = "0"
var denominator = "0"
var wholeNumber = "0"
val fractionObjectBuilder = Fraction.newBuilder()
if (inputText.startsWith("-"))
isNegative = true
inputText = inputText.replace("-", "").trim()
wholeNumber = if (inputText.contains("/") && inputText.contains(" ")) {
inputText.substringBefore(" ")
} else if (inputText.contains("/")) {
wholeNumber
} else {
inputText
}
inputText =
if (inputText.contains(" ")) inputText.substringAfter(" ").replace(" ", "") else inputText.replace(" ", "")
if (inputText.contains("/")) {
numerator = inputText.substringBefore("/")
denominator = inputText.substringAfter("/")
}
fractionObjectBuilder.setIsNegative(isNegative).setNumerator(numerator.toInt())
.setDenominator(denominator.toInt()).wholeNumber = wholeNumber.toInt()
return fractionObjectBuilder.build()
// Normalize whitespace to ensure that answer follows a simpler subset of possible patterns.
val inputText: String = text.normalizeWhitespace()
return parseMixedNumber(inputText)
?: parseFraction(inputText)
?: parseWholeNumber(inputText)
?: throw IllegalArgumentException("Incorrectly formatted fraction: $text")
}

private fun parseMixedNumber(inputText: String): Fraction? {
val mixedNumberMatch = mixedNumberRegex.matchEntire(inputText) ?: return null
val (_, wholeNumberText, numeratorText, denominatorText) = mixedNumberMatch.groupValues
return Fraction.newBuilder()
.setIsNegative(isInputNegative(inputText))
.setWholeNumber(wholeNumberText.toInt())
.setNumerator(numeratorText.toInt())
.setDenominator(denominatorText.toInt())
.build()
}

private fun parseFraction(inputText: String): Fraction? {
val fractionOnlyMatch = fractionOnlyRegex.matchEntire(inputText) ?: return null
val (_, numeratorText, denominatorText) = fractionOnlyMatch.groupValues
// Fraction-only numbers imply no whole number.
return Fraction.newBuilder()
.setIsNegative(isInputNegative(inputText))
.setNumerator(numeratorText.toInt())
.setDenominator(denominatorText.toInt())
.build()
}

private fun parseWholeNumber(inputText: String): Fraction? {
val wholeNumberMatch = wholeNumberOnlyRegex.matchEntire(inputText) ?: return null
val (_, wholeNumberText) = wholeNumberMatch.groupValues
// Whole number fractions imply '0/1' fractional parts.
return Fraction.newBuilder()
.setIsNegative(isInputNegative(inputText))
.setWholeNumber(wholeNumberText.toInt())
.setNumerator(0)
.setDenominator(1)
.build()
}

private fun isInputNegative(inputText: String): Boolean = inputText.startsWith("-")
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class AudioFragment : InjectableFragment(), LanguageInterface {
@Inject
lateinit var audioFragmentPresenter: AudioFragmentPresenter

override fun onAttach(context: Context?) {
override fun onAttach(context: Context) {
super.onAttach(context)
fragmentComponent.inject(this)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import org.oppia.domain.exploration.ExplorationDataController
import org.oppia.util.data.AsyncResult
import org.oppia.util.logging.Logger
import javax.inject.Inject
import kotlin.collections.ArrayList

private const val TAG_LANGUAGE_DIALOG = "LANGUAGE_DIALOG"
private const val KEY_SELECTED_LANGUAGE = "SELECTED_LANGUAGE"
Expand Down
Loading

0 comments on commit a23de65

Please sign in to comment.