From 4f58be9d399c70e1d1cd241495280ae913afbf07 Mon Sep 17 00:00:00 2001 From: Manas <119405883+manas-yu@users.noreply.github.com> Date: Tue, 28 Jan 2025 15:52:23 +0530 Subject: [PATCH] Fix #4097, Fix #4838, Fix #4170: Resolving TODOs (#5655) ## Explanation Fixes #4097, Fix #4848 and Fix #4170 This PR resolves the TODOs left in #5627, #5614 and #5647. 1. **Integrating `getContentDescription` in `HintViewModel`:** - Replaced `fromHtml` with `getContentDescription` introduced in #4848. - Added `ConceptCardTagHandler` to `customTagHandlers` for handling `CUSTOM_CONCEPT_CARD_TAG`. - Injected `ConceptCardLinkClickListener` into the `HintsAndSolutionViewModel.Factory`. 2. **Resolving TODOs for #4097:** - Removed TODOs and text-file-exemptions(in scripts/assets) for `FractionSubject`, `MathExpressionSubject`, `RealSubject`, and `MathEquationSubject`. 3. **Resolving TODOs for #4170 :** - Removed TODO comment from `MathTagHandler`. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- .../app/hintsandsolution/HintViewModel.kt | 20 ++++++++++--- .../HintsAndSolutionViewModel.kt | 29 ++++++++++++++----- .../exploration/ExplorationActivityTest.kt | 7 ++--- .../QuestionPlayerActivityTest.kt | 2 +- scripts/assets/test_file_exemptions.textproto | 16 ---------- .../android/testing/math/FractionSubject.kt | 2 -- .../testing/math/MathEquationSubject.kt | 2 -- .../testing/math/MathExpressionSubject.kt | 2 -- .../oppia/android/testing/math/RealSubject.kt | 2 -- .../util/parser/html/ConceptCardTagHandler.kt | 22 ++++++++++++-- .../parser/html/HtmlParserEntityTypeModule.kt | 9 ++++++ .../util/parser/html/MathTagHandler.kt | 1 - .../parser/html/ConceptCardTagHandlerTest.kt | 2 +- 13 files changed, 70 insertions(+), 46 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintViewModel.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintViewModel.kt index 57b4b8a2f47..fdffc7859ba 100644 --- a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintViewModel.kt @@ -1,6 +1,9 @@ package org.oppia.android.app.hintsandsolution import androidx.databinding.ObservableBoolean +import org.oppia.android.util.logging.ConsoleLogger +import org.oppia.android.util.parser.html.CUSTOM_CONCEPT_CARD_TAG +import org.oppia.android.util.parser.html.ConceptCardTagHandler import org.oppia.android.util.parser.html.CustomHtmlContentHandler /** @@ -9,21 +12,30 @@ import org.oppia.android.util.parser.html.CustomHtmlContentHandler * @property title the title of this hint, relative to others (this is generated by the app) * @property hintSummary the core hint text (which may contain HTML) to show the user * @property isHintRevealed whether the hint is currently expanded and viewable + * @property conceptCardLinkClickListener listener for handling concept card clicks + * @property consoleLogger logger for handling any parsing errors */ class HintViewModel( val title: String, val hintSummary: String, - val isHintRevealed: ObservableBoolean + val isHintRevealed: ObservableBoolean, + private val conceptCardLinkClickListener: ConceptCardTagHandler.ConceptCardLinkClickListener, + private val consoleLogger: ConsoleLogger ) : HintsAndSolutionItemViewModel() { /** * A screenreader-friendly version of [hintSummary] that should be used for readout, in place of * the original summary. */ val hintContentDescription: String by lazy { - CustomHtmlContentHandler.fromHtml( + CustomHtmlContentHandler.getContentDescription( hintSummary, imageRetriever = null, - customTagHandlers = mapOf() - ).toString() + customTagHandlers = mapOf( + CUSTOM_CONCEPT_CARD_TAG to ConceptCardTagHandler( + conceptCardLinkClickListener, + consoleLogger + ) + ) + ) } } diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionViewModel.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionViewModel.kt index f604f7f6547..2e5c864c6e1 100644 --- a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionViewModel.kt @@ -14,6 +14,8 @@ import org.oppia.android.domain.hintsandsolution.isHintRevealed import org.oppia.android.domain.hintsandsolution.isSolutionAvailable import org.oppia.android.domain.hintsandsolution.isSolutionRevealed import org.oppia.android.domain.translation.TranslationController +import org.oppia.android.util.logging.ConsoleLogger +import org.oppia.android.util.parser.html.ConceptCardTagHandler import javax.inject.Inject /** @@ -27,7 +29,9 @@ class HintsAndSolutionViewModel private constructor( private val writtenTranslationContext: WrittenTranslationContext, private val resourceHandler: AppLanguageResourceHandler, private val translationController: TranslationController, - private val solutionViewModelFactory: SolutionViewModel.Factory + private val solutionViewModelFactory: SolutionViewModel.Factory, + private val conceptCardTagHandlerFactory: ConceptCardTagHandler.Factory, + private val consoleLogger: ConsoleLogger ) : ObservableViewModel() { private val hintList by lazy { helpIndex.dropLastUnavailable(state.interaction.hintList) } private val solution by lazy { @@ -57,7 +61,9 @@ class HintsAndSolutionViewModel private constructor( private fun createViewModels(): List { return hintList.mapIndexed { index, hint -> createHintViewModel( - index, hint, isHintRevealed = ObservableBoolean(helpIndex.isHintRevealed(index, hintList)) + index, + hint, + isHintRevealed = ObservableBoolean(helpIndex.isHintRevealed(index, hintList)) ) } + listOfNotNull(solution?.let(this::createSolutionViewModel)) + ReturnToLessonViewModel } @@ -73,16 +79,21 @@ class HintsAndSolutionViewModel private constructor( resourceHandler.toHumanReadableString(hintIndex + 1) ), hintSummary = translationController.extractString( - hint.hintContent, writtenTranslationContext + hint.hintContent, + writtenTranslationContext ), - isHintRevealed = isHintRevealed + isHintRevealed = isHintRevealed, + conceptCardLinkClickListener = + conceptCardTagHandlerFactory.createConceptCardLinkClickListener(), + consoleLogger = consoleLogger ) } private fun createSolutionViewModel(solution: Solution): SolutionViewModel { return solutionViewModelFactory.create( solutionSummary = translationController.extractString( - solution.explanation, writtenTranslationContext + solution.explanation, + writtenTranslationContext ), isSolutionRevealed = isSolutionRevealed, isSolutionExclusive = solution.answerIsExclusive, @@ -96,7 +107,9 @@ class HintsAndSolutionViewModel private constructor( class Factory @Inject constructor( private val resourceHandler: AppLanguageResourceHandler, private val translationController: TranslationController, - private val solutionViewModelFactory: SolutionViewModel.Factory + private val solutionViewModelFactory: SolutionViewModel.Factory, + private val conceptCardTagHandlerFactory: ConceptCardTagHandler.Factory, + private val consoleLogger: ConsoleLogger ) { /** * Returns a new [HintsAndSolutionViewModel] that populates a list of item view models (to be @@ -114,7 +127,9 @@ class HintsAndSolutionViewModel private constructor( writtenTranslationContext, resourceHandler, translationController, - solutionViewModelFactory + solutionViewModelFactory, + conceptCardTagHandlerFactory, + consoleLogger ) } } diff --git a/app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt b/app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt index d5adce598af..e5e87760454 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt @@ -2288,15 +2288,12 @@ class ExplorationActivityTest { openHintsAndSolutionsDialog() pressRevealHintButton(hintPosition = 0) - // TODO(#4848): Fix content description generation & update this test to verify using the - // correct text. - // Ensure the hint description is correct and doesn't contain any HTML. onView(withId(R.id.hints_and_solution_summary)) .check( matches( withContentDescription( - "Remember that two halves, when added together," + - " make one whole.\n\nClick on this .\n\n" + "Remember that two halves, when added together, make one whole." + + "\nClick on this test_skill_id_1 concept card." ) ) ) diff --git a/app/src/sharedTest/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityTest.kt b/app/src/sharedTest/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityTest.kt index 29cfd79e184..3ca1996fe85 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityTest.kt @@ -457,7 +457,7 @@ class QuestionPlayerActivityTest { matches( withContentDescription( "To write a fraction, you need to know its denominator, which is the total " + - "number of pieces in the whole. All of these pieces should be the same size.\n\n" + "number of pieces in the whole. All of these pieces should be the same size." ) ) ) diff --git a/scripts/assets/test_file_exemptions.textproto b/scripts/assets/test_file_exemptions.textproto index 0baccac8151..8529317da0b 100644 --- a/scripts/assets/test_file_exemptions.textproto +++ b/scripts/assets/test_file_exemptions.textproto @@ -4102,18 +4102,6 @@ test_file_exemption { exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/ComparableOperationSubject.kt" test_file_not_required: true } -test_file_exemption { - exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt" - test_file_not_required: true -} -test_file_exemption { - exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt" - test_file_not_required: true -} -test_file_exemption { - exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt" - test_file_not_required: true -} test_file_exemption { exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt" test_file_not_required: true @@ -4122,10 +4110,6 @@ test_file_exemption { exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/PolynomialSubject.kt" test_file_not_required: true } -test_file_exemption { - exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt" - test_file_not_required: true -} test_file_exemption { exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/TokenSubject.kt" test_file_not_required: true diff --git a/testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt b/testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt index 03c6ed4ef78..c4441098a4b 100644 --- a/testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt +++ b/testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt @@ -10,8 +10,6 @@ import com.google.common.truth.extensions.proto.LiteProtoSubject import org.oppia.android.app.model.Fraction import org.oppia.android.util.math.toDouble -// TODO(#4097): Add tests for this class. - /** * Truth subject for verifying properties of [Fraction]s. * diff --git a/testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt b/testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt index a8c456a37e3..95a527817ec 100644 --- a/testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt +++ b/testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt @@ -10,8 +10,6 @@ import org.oppia.android.testing.math.MathEquationSubject.Companion.assertThat import org.oppia.android.testing.math.MathExpressionSubject.Companion.assertThat import org.oppia.android.util.math.toRawLatex -// TODO(#4097): Add tests for this class. - /** * Truth subject for verifying properties of [MathEquation]s. * diff --git a/testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt b/testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt index e9241d67b58..14e16b98b92 100644 --- a/testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt +++ b/testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt @@ -23,8 +23,6 @@ import org.oppia.android.testing.math.RealSubject.Companion.assertThat import org.oppia.android.util.math.evaluateAsNumericExpression import org.oppia.android.util.math.toRawLatex -// TODO(#4097): Add tests for this class. - /** * Truth subject for verifying properties of [MathExpression]s. * diff --git a/testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt b/testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt index 908bc7be6e2..cc04ad30630 100644 --- a/testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt +++ b/testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt @@ -10,8 +10,6 @@ import com.google.common.truth.extensions.proto.LiteProtoSubject import org.oppia.android.app.model.Real import org.oppia.android.testing.math.FractionSubject.Companion.assertThat -// TODO(#4097): Add tests for this class. - /** * Truth subject for verifying properties of [Real]s. * diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/ConceptCardTagHandler.kt b/utility/src/main/java/org/oppia/android/util/parser/html/ConceptCardTagHandler.kt index 83625743d87..6570da71fc0 100644 --- a/utility/src/main/java/org/oppia/android/util/parser/html/ConceptCardTagHandler.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/ConceptCardTagHandler.kt @@ -7,6 +7,7 @@ import android.text.style.ClickableSpan import android.view.View import org.oppia.android.util.logging.ConsoleLogger import org.xml.sax.Attributes +import javax.inject.Inject /** The custom tag corresponding to [ConceptCardTagHandler]. */ const val CUSTOM_CONCEPT_CARD_TAG = "oppia-noninteractive-skillreview" @@ -52,10 +53,25 @@ class ConceptCardTagHandler( } override fun getContentDescription(attributes: Attributes): String? { - val skillId = attributes.getJsonStringValue(CUSTOM_CONCEPT_CARD_SKILL_ID) val text = attributes.getJsonStringValue(CUSTOM_CONCEPT_CARD_TEXT_VALUE) - return if (!skillId.isNullOrBlank() && !text.isNullOrBlank()) { - "$text concept card $skillId" + return if (!text.isNullOrBlank()) { + text } else "" } + + /** Application-injectable factory for creating [ConceptCardTagHandler]s). */ + class Factory @Inject constructor() { + /** + * Creates a new [ConceptCardLinkClickListener] for handling concept card link click events. + * + * This [ConceptCardLinkClickListener] defines behavior for handling user interactions with links + * displayed in concept cards. The `onConceptCardLinkClicked` method provides the clicked [View] + * and the associated skill ID to enable further action handling. + */ + fun createConceptCardLinkClickListener(): ConceptCardLinkClickListener { + return object : ConceptCardLinkClickListener { + override fun onConceptCardLinkClicked(view: View, skillId: String) {} + } + } + } } diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParserEntityTypeModule.kt b/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParserEntityTypeModule.kt index 42763e35749..1548a8c9348 100755 --- a/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParserEntityTypeModule.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParserEntityTypeModule.kt @@ -2,10 +2,19 @@ package org.oppia.android.util.parser.html import dagger.Module import dagger.Provides +import javax.inject.Singleton /** Provides Html parsing entity type dependencies. */ @Module class HtmlParserEntityTypeModule { + @Provides + @Singleton + fun provideConceptCardLinkClickListener( + factory: ConceptCardTagHandler.Factory + ): ConceptCardTagHandler.ConceptCardLinkClickListener { + return factory.createConceptCardLinkClickListener() + } + @Provides @ExplorationHtmlParserEntityType fun provideExplorationHtmlParserEntityType(): String { diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/MathTagHandler.kt b/utility/src/main/java/org/oppia/android/util/parser/html/MathTagHandler.kt index 8a4de9460c1..9e183440aeb 100644 --- a/utility/src/main/java/org/oppia/android/util/parser/html/MathTagHandler.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/MathTagHandler.kt @@ -44,7 +44,6 @@ class MathTagHandler( val content = MathContent.parseMathContent( attributes.getJsonObjectValue(CUSTOM_MATH_MATH_CONTENT_ATTRIBUTE) ) - // TODO(#4170): Fix vertical alignment centering for inline cached LaTeX. val useInlineRendering = when (attributes.getValue(CUSTOM_MATH_RENDER_TYPE_ATTRIBUTE)) { "inline" -> true "block" -> false diff --git a/utility/src/test/java/org/oppia/android/util/parser/html/ConceptCardTagHandlerTest.kt b/utility/src/test/java/org/oppia/android/util/parser/html/ConceptCardTagHandlerTest.kt index 53dfd421cc6..ee861e6bafc 100644 --- a/utility/src/test/java/org/oppia/android/util/parser/html/ConceptCardTagHandlerTest.kt +++ b/utility/src/test/java/org/oppia/android/util/parser/html/ConceptCardTagHandlerTest.kt @@ -125,7 +125,7 @@ class ConceptCardTagHandlerTest { imageRetriever = mockImageRetriever, customTagHandlers = tagHandlersWithConceptCardSupport ) - assertThat(contentDescription).isEqualTo("refresher lesson concept card skill_id_1") + assertThat(contentDescription).isEqualTo("refresher lesson") } @Test