Skip to content

Commit a6c23ca

Browse files
BenHenningBenHenning
authored
Fix #4026, #4027, #4040, #4076, #4287, #4319, #4329: Fix miscellaneous bugs found while testing alpha MR4 (#4259)
* Fix for #4040. Doesn't include test changes yet. * Fixes for #4027. No tests or updated documentation. * Revert development-only caching flags. * Lint fixes. * Post-merge fix + 2 other fixes. The first fix addresses #4076 in full. The second fix addresses what seems to be a long-standing issue with image region selection to ensure that the region views are always clickable when the interaction loads. * Add follow-up fixes & tests. * Fix new test on Gradle builds. This test corresponds to Bazel-locked functionality, so it can't pass on Gradle in the same way. * Fix #4319 and #4329. * Post-merge lint fixes. * Lint fixes, + doc & TODO udpate.
1 parent 5f195af commit a6c23ca

File tree

32 files changed

+594
-226
lines changed

32 files changed

+594
-226
lines changed

‎WORKSPACE‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,9 @@ git_repository(
125125
# to correctly size in-line SVGs (such as those needed for LaTeX-based math expressions).
126126
git_repository(
127127
name = "androidsvg",
128-
commit = "6bd15f69caee3e6857fcfcd123023716b4adec1d",
128+
commit = "4bc1d26412f0fb9fd4ef263fa93f6a64f4d4dbcf",
129129
remote = "https://github.com/oppia/androidsvg",
130+
shallow_since = "1647295507 -0700",
130131
)
131132

132133
# A custom fork of KotliTeX that removes resources artifacts that break the build, and updates the

‎app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ class StatePlayerRecyclerViewAssembler private constructor(
373373
answerAndResponse.userAnswer,
374374
gcsEntityId,
375375
/* isAnswerCorrect= */ false
376-
).let { viewModel ->
376+
)?.let { viewModel ->
377377
if (showPreviousAnswers) {
378378
pendingItemList += viewModel
379379
}
@@ -397,17 +397,17 @@ class StatePlayerRecyclerViewAssembler private constructor(
397397
answersAndResponses.lastOrNull()?.let { answerAndResponse ->
398398
if (playerFeatureSet.pastAnswerSupport) {
399399
if (isLastAnswerCorrect && isSplitView.get()!!) {
400-
rightPendingItemList += createSubmittedAnswer(
400+
createSubmittedAnswer(
401401
answerAndResponse.userAnswer,
402402
gcsEntityId,
403403
isAnswerCorrect = true
404-
)
404+
)?.let(rightPendingItemList::add)
405405
} else {
406-
pendingItemList += createSubmittedAnswer(
406+
createSubmittedAnswer(
407407
answerAndResponse.userAnswer,
408408
gcsEntityId,
409409
isLastAnswerCorrect || answerAndResponse.isCorrectAnswer
410-
)
410+
)?.let(pendingItemList::add)
411411
}
412412
}
413413
if (playerFeatureSet.feedbackSupport) {
@@ -551,19 +551,20 @@ class StatePlayerRecyclerViewAssembler private constructor(
551551
userAnswer: UserAnswer,
552552
gcsEntityId: String,
553553
isAnswerCorrect: Boolean
554-
): SubmittedAnswerViewModel {
555-
val submittedAnswerViewModel =
554+
): SubmittedAnswerViewModel? {
555+
return userAnswer.takeIf { it.hasAnswerToDisplayToUser() }?.let {
556556
SubmittedAnswerViewModel(
557557
userAnswer,
558558
gcsEntityId,
559559
hasConversationView,
560560
isSplitView.get()!!,
561561
playerFeatureSet.conceptCardSupport,
562562
resourceHandler
563-
)
564-
submittedAnswerViewModel.setIsCorrectAnswer(isAnswerCorrect)
565-
submittedAnswerViewModel.isExtraInteractionAnswerCorrect.set(isAnswerCorrect)
566-
return submittedAnswerViewModel
563+
).also { submittedAnswerViewModel ->
564+
submittedAnswerViewModel.setIsCorrectAnswer(isAnswerCorrect)
565+
submittedAnswerViewModel.isExtraInteractionAnswerCorrect.set(isAnswerCorrect)
566+
}
567+
}
567568
}
568569

569570
private fun createFeedbackItem(
@@ -1454,4 +1455,16 @@ class StatePlayerRecyclerViewAssembler private constructor(
14541455
)
14551456
}
14561457
}
1458+
1459+
private companion object {
1460+
private fun UserAnswer.hasAnswerToDisplayToUser(): Boolean {
1461+
return when (textualAnswerCase) {
1462+
UserAnswer.TextualAnswerCase.HTML_ANSWER -> htmlAnswer.isNotEmpty()
1463+
UserAnswer.TextualAnswerCase.PLAIN_ANSWER -> plainAnswer.isNotEmpty()
1464+
UserAnswer.TextualAnswerCase.LIST_OF_HTML_ANSWERS ->
1465+
listOfHtmlAnswers.setOfHtmlStringsOrBuilderList.isNotEmpty()
1466+
UserAnswer.TextualAnswerCase.TEXTUALANSWER_NOT_SET, null -> false
1467+
}
1468+
}
1469+
}
14571470
}

‎app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/ContinueInteractionViewModel.kt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ class ContinueInteractionViewModel private constructor(
3838
answer = InteractionObject.newBuilder().apply {
3939
normalizedString = DEFAULT_CONTINUE_INTERACTION_TEXT_ANSWER
4040
}.build()
41-
plainAnswer = DEFAULT_CONTINUE_INTERACTION_TEXT_ANSWER
4241
this.writtenTranslationContext = this@ContinueInteractionViewModel.writtenTranslationContext
4342
}.build()
4443

‎app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import android.view.MotionEvent
55
import android.view.View
66
import android.widget.FrameLayout
77
import androidx.core.view.ViewCompat
8+
import androidx.core.view.children
89
import androidx.core.view.forEachIndexed
910
import androidx.core.view.isVisible
1011
import org.oppia.android.R
@@ -136,7 +137,21 @@ class ClickableAreasImage(
136137
}
137138
}
138139
it.addView(newView)
139-
newView.requestLayout()
140+
}
141+
142+
// Ensure that the children views are properly computed. The specific flow below is
143+
// recommended by https://stackoverflow.com/a/42430695/3689782 where it's also explained in
144+
// great detail. The 'post' seems necessary since, from observation, requesting layout &
145+
// invalidation doesn't always work (perhaps since this method can be called during a layout
146+
// step), so posting ensures that the views are eventually computed. It's not obvious why
147+
// Android sometimes doesn't compute the region view dimensions, but it results in the
148+
// interaction being non-interactive (though it's recoverable with back & forward navigation
149+
// or rotation, this isn't likely to be obvious to learners and it's a generally poor user
150+
// experience).
151+
it.post {
152+
it.children.forEach(View::forceLayout)
153+
it.invalidate()
154+
it.requestLayout()
140155
}
141156
}
142157
}

‎app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3580,6 +3580,37 @@ class StateFragmentTest {
35803580
// TODO(#3171): Implement image region selection tests for English/Arabic to demonstrate that
35813581
// answers submit normally & with no special behaviors.
35823582

3583+
@Test
3584+
fun testStateFragment_clickContinue_returnToState_doesNotHaveFeedbackBox() {
3585+
launchForExploration(TEST_EXPLORATION_ID_2, shouldSavePartialProgress = false).use {
3586+
startPlayingExploration()
3587+
playThroughPrototypeState1()
3588+
3589+
clickPreviousNavigationButton()
3590+
3591+
// The continue interaction should not show feedback.
3592+
scrollToViewType(CONTENT)
3593+
onView(withId(R.id.submitted_answer_text_view)).check(doesNotExist())
3594+
}
3595+
}
3596+
3597+
@Test
3598+
fun testStateFragment_clickContinue_finishNextState_returnToContinue_doesNotHaveFeedbackBox() {
3599+
launchForExploration(TEST_EXPLORATION_ID_2, shouldSavePartialProgress = false).use {
3600+
startPlayingExploration()
3601+
playThroughPrototypeState1()
3602+
3603+
// Finish the current state, then return back to the previous one.
3604+
typeFractionText("1/2")
3605+
clickSubmitAnswerButton()
3606+
clickPreviousNavigationButton()
3607+
3608+
// The continue interaction should not show feedback.
3609+
scrollToViewType(CONTENT)
3610+
onView(withId(R.id.submitted_answer_text_view)).check(doesNotExist())
3611+
}
3612+
}
3613+
35833614
private fun addShadowMediaPlayerException(dataSource: Any, exception: Exception) {
35843615
val classLoader = StateFragmentTest::class.java.classLoader!!
35853616
val shadowMediaPlayerClass = classLoader.loadClass("org.robolectric.shadows.ShadowMediaPlayer")

‎app/src/test/java/org/oppia/android/app/player/state/StateFragmentLocalTest.kt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1924,6 +1924,7 @@ class StateFragmentLocalTest {
19241924
}
19251925

19261926
private fun clickNextStateNavigationButton() {
1927+
onView(withId(R.id.state_recycler_view)).perform(scrollToViewType(NEXT_NAVIGATION_BUTTON))
19271928
onView(withId(R.id.next_state_navigation_button)).perform(click())
19281929
testCoroutineDispatchers.runCurrent()
19291930
}

‎domain/src/main/java/org/oppia/android/domain/classify/rules/algebraicexpressioninput/AlgebraicExpressionInputIsEquivalentToRuleClassifierProvider.kt‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,14 @@ import org.oppia.android.domain.classify.RuleClassifier
77
import org.oppia.android.domain.classify.rules.GenericRuleClassifier
88
import org.oppia.android.domain.classify.rules.RuleClassifierProvider
99
import org.oppia.android.util.logging.ConsoleLogger
10+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode
11+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.ALL_ERRORS
12+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.REQUIRED_ONLY
1013
import org.oppia.android.util.math.MathExpressionParser.Companion.MathParsingResult
11-
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicExpression
1214
import org.oppia.android.util.math.isApproximatelyEqualTo
1315
import org.oppia.android.util.math.toPolynomial
1416
import javax.inject.Inject
17+
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicExpression as parseExpression
1518

1619
/**
1720
* Provider for a classifier that determines whether an algebraic expression is mathematically
@@ -39,13 +42,17 @@ class AlgebraicExpressionInputIsEquivalentToRuleClassifierProvider @Inject const
3942
classificationContext: ClassificationContext
4043
): Boolean {
4144
val allowedVariables = classificationContext.extractAllowedVariables()
42-
val answerExpression = parsePolynomial(answer, allowedVariables) ?: return false
43-
val inputExpression = parsePolynomial(input, allowedVariables) ?: return false
45+
val answerExpression = parsePolynomial(answer, allowedVariables, ALL_ERRORS) ?: return false
46+
val inputExpression = parsePolynomial(input, allowedVariables, REQUIRED_ONLY) ?: return false
4447
return answerExpression.isApproximatelyEqualTo(inputExpression)
4548
}
4649

47-
private fun parsePolynomial(rawExpression: String, allowedVariables: List<String>): Polynomial? {
48-
return when (val expResult = parseAlgebraicExpression(rawExpression, allowedVariables)) {
50+
private fun parsePolynomial(
51+
rawExpression: String,
52+
allowedVariables: List<String>,
53+
checkingMode: ErrorCheckingMode
54+
): Polynomial? {
55+
return when (val expResult = parseExpression(rawExpression, allowedVariables, checkingMode)) {
4956
is MathParsingResult.Success -> {
5057
expResult.result.toPolynomial().also {
5158
if (it == null) {

‎domain/src/main/java/org/oppia/android/domain/classify/rules/algebraicexpressioninput/AlgebraicExpressionInputMatchesExactlyWithRuleClassifierProvider.kt‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,14 @@ import org.oppia.android.domain.classify.RuleClassifier
77
import org.oppia.android.domain.classify.rules.GenericRuleClassifier
88
import org.oppia.android.domain.classify.rules.RuleClassifierProvider
99
import org.oppia.android.util.logging.ConsoleLogger
10+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode
11+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.ALL_ERRORS
12+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.REQUIRED_ONLY
1013
import org.oppia.android.util.math.MathExpressionParser.Companion.MathParsingResult
11-
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicExpression
1214
import org.oppia.android.util.math.isApproximatelyEqualTo
15+
import org.oppia.android.util.math.stripRedundantGroups
1316
import javax.inject.Inject
17+
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicExpression as parseExpression
1418

1519
/**
1620
* Provider for a classifier that determines whether an algebraic expression is exactly equal to the
@@ -37,16 +41,19 @@ class AlgebraicExpressionInputMatchesExactlyWithRuleClassifierProvider @Inject c
3741
classificationContext: ClassificationContext
3842
): Boolean {
3943
val allowedVariables = classificationContext.extractAllowedVariables()
40-
val answerExpression = parseExpression(answer, allowedVariables) ?: return false
41-
val inputExpression = parseExpression(input, allowedVariables) ?: return false
42-
return answerExpression.isApproximatelyEqualTo(inputExpression)
44+
val answerExpression =
45+
parseAlgebraicExpression(answer, allowedVariables, ALL_ERRORS) ?: return false
46+
val inputExpression =
47+
parseAlgebraicExpression(input, allowedVariables, REQUIRED_ONLY) ?: return false
48+
return answerExpression.isApproximatelyEqualTo(inputExpression.stripRedundantGroups())
4349
}
4450

45-
private fun parseExpression(
51+
private fun parseAlgebraicExpression(
4652
rawExpression: String,
47-
allowedVariables: List<String>
53+
allowedVariables: List<String>,
54+
checkingMode: ErrorCheckingMode
4855
): MathExpression? {
49-
return when (val expResult = parseAlgebraicExpression(rawExpression, allowedVariables)) {
56+
return when (val expResult = parseExpression(rawExpression, allowedVariables, checkingMode)) {
5057
is MathParsingResult.Success -> expResult.result
5158
is MathParsingResult.Failure -> {
5259
consoleLogger.e(

‎domain/src/main/java/org/oppia/android/domain/classify/rules/algebraicexpressioninput/AlgebraicExpressionInputMatchesUpToTrivialManipulationsRuleClassifierProvider.kt‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,14 @@ import org.oppia.android.domain.classify.RuleClassifier
77
import org.oppia.android.domain.classify.rules.GenericRuleClassifier
88
import org.oppia.android.domain.classify.rules.RuleClassifierProvider
99
import org.oppia.android.util.logging.ConsoleLogger
10+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode
11+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.ALL_ERRORS
12+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.REQUIRED_ONLY
1013
import org.oppia.android.util.math.MathExpressionParser.Companion.MathParsingResult
11-
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicExpression
1214
import org.oppia.android.util.math.isApproximatelyEqualTo
1315
import org.oppia.android.util.math.toComparableOperation
1416
import javax.inject.Inject
17+
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicExpression as parseExpression
1518

1619
/**
1720
* Provider for a classifier that determines whether an algebraic expression is equal to the
@@ -41,16 +44,19 @@ class AlgebraicExpressionInputMatchesUpToTrivialManipulationsRuleClassifierProvi
4144
classificationContext: ClassificationContext
4245
): Boolean {
4346
val allowedVariables = classificationContext.extractAllowedVariables()
44-
val answerExpression = parseComparableOperation(answer, allowedVariables) ?: return false
45-
val inputExpression = parseComparableOperation(input, allowedVariables) ?: return false
47+
val answerExpression =
48+
parseComparableOperation(answer, allowedVariables, ALL_ERRORS) ?: return false
49+
val inputExpression =
50+
parseComparableOperation(input, allowedVariables, REQUIRED_ONLY) ?: return false
4651
return answerExpression.isApproximatelyEqualTo(inputExpression)
4752
}
4853

4954
private fun parseComparableOperation(
5055
rawExpression: String,
51-
allowedVariables: List<String>
56+
allowedVariables: List<String>,
57+
checkingMode: ErrorCheckingMode
5258
): ComparableOperation? {
53-
return when (val expResult = parseAlgebraicExpression(rawExpression, allowedVariables)) {
59+
return when (val expResult = parseExpression(rawExpression, allowedVariables, checkingMode)) {
5460
is MathParsingResult.Success -> expResult.result.toComparableOperation()
5561
is MathParsingResult.Failure -> {
5662
consoleLogger.e(

‎domain/src/main/java/org/oppia/android/domain/classify/rules/mathequationinput/MathEquationInputIsEquivalentToRuleClassifierProvider.kt‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,17 @@ import org.oppia.android.domain.classify.RuleClassifier
77
import org.oppia.android.domain.classify.rules.GenericRuleClassifier
88
import org.oppia.android.domain.classify.rules.RuleClassifierProvider
99
import org.oppia.android.util.logging.ConsoleLogger
10+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode
11+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.ALL_ERRORS
12+
import org.oppia.android.util.math.MathExpressionParser.Companion.ErrorCheckingMode.REQUIRED_ONLY
1013
import org.oppia.android.util.math.MathExpressionParser.Companion.MathParsingResult
11-
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicEquation
1214
import org.oppia.android.util.math.isApproximatelyEqualTo
1315
import org.oppia.android.util.math.minus
1416
import org.oppia.android.util.math.sort
1517
import org.oppia.android.util.math.toPolynomial
1618
import org.oppia.android.util.math.unaryMinus
1719
import javax.inject.Inject
20+
import org.oppia.android.util.math.MathExpressionParser.Companion.parseAlgebraicEquation as parseEquation
1821

1922
/**
2023
* Provider for a classifier that determines whether a math equation expression is mathematically
@@ -44,8 +47,10 @@ class MathEquationInputIsEquivalentToRuleClassifierProvider @Inject constructor(
4447
classificationContext: ClassificationContext
4548
): Boolean {
4649
val allowedVariables = classificationContext.extractAllowedVariables()
47-
val (answerLhs, answerRhs) = parsePolynomials(answer, allowedVariables) ?: return false
48-
val (inputLhs, inputRhs) = parsePolynomials(input, allowedVariables) ?: return false
50+
val (answerLhs, answerRhs) =
51+
parsePolynomials(answer, allowedVariables, ALL_ERRORS) ?: return false
52+
val (inputLhs, inputRhs) =
53+
parsePolynomials(input, allowedVariables, REQUIRED_ONLY) ?: return false
4954

5055
val newAnswerLhs = (answerLhs - answerRhs).sort()
5156
val newInputLhs = (inputLhs - inputRhs).sort()
@@ -65,9 +70,10 @@ class MathEquationInputIsEquivalentToRuleClassifierProvider @Inject constructor(
6570

6671
private fun parsePolynomials(
6772
rawEquation: String,
68-
allowedVariables: List<String>
73+
allowedVariables: List<String>,
74+
checkingMode: ErrorCheckingMode
6975
): Pair<Polynomial, Polynomial>? {
70-
return when (val eqResult = parseAlgebraicEquation(rawEquation, allowedVariables)) {
76+
return when (val eqResult = parseEquation(rawEquation, allowedVariables, checkingMode)) {
7177
is MathParsingResult.Success -> {
7278
val lhsExp = eqResult.result.leftSide.toPolynomial()
7379
val rhsExp = eqResult.result.rightSide.toPolynomial()

0 commit comments

Comments
 (0)