From a1fbe0c98b512eec45bb94804b50095606a5e64b Mon Sep 17 00:00:00 2001 From: Vishwajith Shettigar <76042077+Vishwajith-Shettigar@users.noreply.github.com> Date: Fri, 30 Aug 2024 18:16:33 +0530 Subject: [PATCH] Fix #5395: Fixed concept card not closing when opened from hint (#5509) ## Explanation Fixes #5395. `(fragment.requireActivity() as? ConceptCardListener)?.dismissConceptCard()` in `ConceptCardFragmentPresenter` calls the `dismissConceptCard()` method in` ExplorationActivity` since `ExplorationActivity `implements `ConceptCardListener` and overrides the `dismissConceptCard()` method. This makes call `dismissAll()` in `ConceptCardFragment` through `StateFragment` there it passes its `childFragmentManager`. Even though it managed to call the `dismissAll()` method in `ConceptCardFragment`, but the wrong fragment manager was passed. We passed `StateFragment's childFragmentManager`, but `ConceptCardFragment `is nested within `HintAndSolutionFragment`, so it should be passed `HintAndSolutionFragment's childFragmentManager`. [concept_card.webm](https://github.com/user-attachments/assets/d1959296-79bf-4e4f-b2d9-04acff089c37) ## 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)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --- .../HintsAndSolutionDialogFragment.kt | 9 ++ ...HintsAndSolutionDialogFragmentPresenter.kt | 5 ++ .../player/exploration/ExplorationActivity.kt | 4 +- .../ExplorationActivityPresenter.kt | 4 - .../player/exploration/ExplorationFragment.kt | 2 - .../ExplorationFragmentPresenter.kt | 2 - .../android/app/player/state/StateFragment.kt | 2 - .../player/state/StateFragmentPresenter.kt | 5 -- .../ConceptCardFragmentTestActivity.kt | 2 +- .../QuestionPlayerActivityPresenter.kt | 4 +- .../questionplayer/QuestionPlayerFragment.kt | 2 - .../QuestionPlayerFragmentPresenter.kt | 5 -- .../exploration/ExplorationActivityTest.kt | 89 +++++++++++++++++++ 13 files changed, 110 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt index c36cafe9ecc..e81aca55b3f 100644 --- a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt +++ b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt @@ -14,6 +14,7 @@ import org.oppia.android.app.model.HintsAndSolutionDialogFragmentStateBundle import org.oppia.android.app.model.ProfileId import org.oppia.android.app.model.State import org.oppia.android.app.model.WrittenTranslationContext +import org.oppia.android.app.topic.conceptcard.ConceptCardFragment import org.oppia.android.util.extensions.getProto import org.oppia.android.util.extensions.putProto import javax.inject.Inject @@ -192,4 +193,12 @@ class HintsAndSolutionDialogFragment : isSolutionRevealed ) } + + /** + * Delegates the removal of all [ConceptCardFragment] instances + * to the [hintsAndSolutionDialogFragmentPresenter]. + */ + fun dismissConceptCard() { + hintsAndSolutionDialogFragmentPresenter.dismissConceptCard() + } } diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt index 16f9dec7b19..824c777ca78 100644 --- a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt @@ -331,4 +331,9 @@ class HintsAndSolutionDialogFragmentPresenter @Inject constructor( override fun onConceptCardLinkClicked(view: View, skillId: String) { ConceptCardFragment.bringToFrontOrCreateIfNew(skillId, profileId, fragment.childFragmentManager) } + + /** Removes all [ConceptCardFragment] in the given FragmentManager. */ + fun dismissConceptCard() { + ConceptCardFragment.dismissAll(fragment.childFragmentManager) + } } diff --git a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt index b95785bf2d0..6912d639ed9 100755 --- a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt +++ b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt @@ -190,7 +190,9 @@ class ExplorationActivity : this.writtenTranslationContext = writtenTranslationContext } - override fun dismissConceptCard() = explorationActivityPresenter.dismissConceptCard() + override fun dismissConceptCard() { + getHintsAndSolution()?.dismissConceptCard() + } override fun requestVoiceOverIconSpotlight(numberOfLogins: Int) { explorationActivityPresenter.requestVoiceOverIconSpotlight(numberOfLogins) diff --git a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt index 9d1c50ec2ea..1c493c19bfa 100644 --- a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt @@ -334,10 +334,6 @@ class ExplorationActivityPresenter @Inject constructor( showDialogFragmentBasedOnCurrentCheckpointState() } - fun dismissConceptCard() { - getExplorationFragment()?.dismissConceptCard() - } - private fun updateToolbarTitle(explorationId: String) { subscribeToExploration( explorationDataController.getExplorationById(profileId, explorationId).toLiveData() diff --git a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragment.kt b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragment.kt index fdffb73b32d..6d4cb2a6330 100755 --- a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragment.kt +++ b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragment.kt @@ -84,7 +84,5 @@ class ExplorationFragment : InjectableFragment() { explorationFragmentPresenter.viewSolution() } - fun dismissConceptCard() = explorationFragmentPresenter.dismissConceptCard() - fun getExplorationCheckpointState() = explorationFragmentPresenter.getExplorationCheckpointState() } diff --git a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragmentPresenter.kt index 151f2456f53..207f5bf9e7f 100755 --- a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragmentPresenter.kt @@ -141,8 +141,6 @@ class ExplorationFragmentPresenter @Inject constructor( getStateFragment()?.viewSolution() } - fun dismissConceptCard() = getStateFragment()?.dismissConceptCard() - fun getExplorationCheckpointState() = getStateFragment()?.getExplorationCheckpointState() private fun getStateFragment(): StateFragment? { diff --git a/app/src/main/java/org/oppia/android/app/player/state/StateFragment.kt b/app/src/main/java/org/oppia/android/app/player/state/StateFragment.kt index 50f05d60082..df5e8a07dde 100755 --- a/app/src/main/java/org/oppia/android/app/player/state/StateFragment.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/StateFragment.kt @@ -162,8 +162,6 @@ class StateFragment : stateFragmentPresenter.viewSolution() } - fun dismissConceptCard() = stateFragmentPresenter.dismissConceptCard() - fun getExplorationCheckpointState() = stateFragmentPresenter.getExplorationCheckpointState() override fun onSaveInstanceState(outState: Bundle) { diff --git a/app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt index 672595d81ef..0147fb7e82c 100755 --- a/app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt @@ -38,7 +38,6 @@ import org.oppia.android.app.player.state.listener.RouteToHintsAndSolutionListen import org.oppia.android.app.player.stopplaying.StopStatePlayingSessionWithSavedProgressListener import org.oppia.android.app.survey.SurveyWelcomeDialogFragment import org.oppia.android.app.survey.TAG_SURVEY_WELCOME_DIALOG -import org.oppia.android.app.topic.conceptcard.ConceptCardFragment import org.oppia.android.app.translation.AppLanguageResourceHandler import org.oppia.android.app.utility.SplitScreenManager import org.oppia.android.app.utility.lifecycle.LifecycleSafeTimerFactory @@ -428,10 +427,6 @@ class StateFragmentPresenter @Inject constructor( subscribeToAnswerOutcome(explorationProgressController.submitAnswer(answer).toLiveData()) } - fun dismissConceptCard() { - ConceptCardFragment.dismissAll(fragment.childFragmentManager) - } - private fun moveToNextState() { stateViewModel.setCanSubmitAnswer(canSubmitAnswer = false) explorationProgressController.moveToNextState().toLiveData().observe( diff --git a/app/src/main/java/org/oppia/android/app/testing/ConceptCardFragmentTestActivity.kt b/app/src/main/java/org/oppia/android/app/testing/ConceptCardFragmentTestActivity.kt index b87c9c8a431..c428093696f 100644 --- a/app/src/main/java/org/oppia/android/app/testing/ConceptCardFragmentTestActivity.kt +++ b/app/src/main/java/org/oppia/android/app/testing/ConceptCardFragmentTestActivity.kt @@ -29,7 +29,7 @@ class ConceptCardFragmentTestActivity : } override fun dismissConceptCard() { - getConceptCardFragment()?.dismiss() + ConceptCardFragment.dismissAll(supportFragmentManager) } private fun getConceptCardFragment(): ConceptCardFragment? { diff --git a/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter.kt b/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter.kt index 5c1f7484766..523ebf2cc4d 100644 --- a/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter.kt @@ -274,7 +274,9 @@ class QuestionPlayerActivityPresenter @Inject constructor( getHintsAndSolutionDialogFragment()?.dismiss() } - fun dismissConceptCard() = getQuestionPlayerFragment()?.dismissConceptCard() + fun dismissConceptCard() { + getHintsAndSolutionDialogFragment()?.dismissConceptCard() + } private fun getHintsAndSolutionDialogFragment(): HintsAndSolutionDialogFragment? { return activity.supportFragmentManager.findFragmentByTag( diff --git a/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragment.kt b/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragment.kt index b5e67ec1318..c825f53e5cf 100644 --- a/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragment.kt +++ b/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragment.kt @@ -106,8 +106,6 @@ class QuestionPlayerFragment : questionPlayerFragmentPresenter.revealSolution() } - fun dismissConceptCard() = questionPlayerFragmentPresenter.dismissConceptCard() - companion object { /** Arguments key for [QuestionPlayerFragment]. */ diff --git a/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragmentPresenter.kt index 7b4861580ab..6319e930125 100644 --- a/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerFragmentPresenter.kt @@ -27,7 +27,6 @@ import org.oppia.android.app.player.state.StatePlayerRecyclerViewAssembler import org.oppia.android.app.player.state.listener.RouteToHintsAndSolutionListener import org.oppia.android.app.player.stopplaying.RestartPlayingSessionListener import org.oppia.android.app.player.stopplaying.StopStatePlayingSessionListener -import org.oppia.android.app.topic.conceptcard.ConceptCardFragment import org.oppia.android.app.utility.FontScaleConfigurationUtil import org.oppia.android.app.utility.SplitScreenManager import org.oppia.android.databinding.QuestionPlayerFragmentBinding @@ -124,10 +123,6 @@ class QuestionPlayerFragmentPresenter @Inject constructor( subscribeToHintSolution(questionAssessmentProgressController.submitSolutionIsRevealed()) } - fun dismissConceptCard() { - ConceptCardFragment.dismissAll(fragment.childFragmentManager) - } - private fun retrieveArguments(): QuestionPlayerFragmentArguments { return fragment.requireArguments().getProto( QuestionPlayerFragment.ARGUMENTS_KEY, QuestionPlayerFragmentArguments.getDefaultInstance() 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 ec53953d954..32d29315737 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 @@ -3,7 +3,9 @@ package org.oppia.android.app.player.exploration import android.app.Application import android.content.Context import android.content.Intent +import android.text.Spannable import android.text.TextUtils +import android.text.style.ClickableSpan import android.view.View import android.widget.TextView import androidx.appcompat.app.AppCompatActivity @@ -48,6 +50,7 @@ import org.hamcrest.CoreMatchers.containsString import org.hamcrest.Description import org.hamcrest.Matcher import org.hamcrest.Matchers.not +import org.hamcrest.TypeSafeMatcher import org.junit.After import org.junit.Before import org.junit.Ignore @@ -2559,6 +2562,92 @@ class ExplorationActivityTest { } } + @Test + @RunOn(TestPlatform.ROBOLECTRIC) // TODO(#3858): Enable for Espresso. + fun testExpActivity_openConceptCard_selectNavigationUp_conceptCardCloses() { + markAllSpotlightsSeen() + launch( + createExplorationActivityIntent( + internalProfileId, + TEST_CLASSROOM_ID_0, + TEST_TOPIC_ID_0, + TEST_STORY_ID_0, + TEST_EXPLORATION_ID_2, + shouldSavePartialProgress = false + ) + ).use { + explorationDataController.startPlayingNewExploration( + internalProfileId, + TEST_CLASSROOM_ID_0, + TEST_TOPIC_ID_0, + TEST_STORY_ID_0, + TEST_EXPLORATION_ID_2 + ) + testCoroutineDispatchers.runCurrent() + clickContinueButton() + // Submit two incorrect answers. + submitFractionAnswer(answerText = "1/3") + submitFractionAnswer(answerText = "1/4") + + // Reveal the hint. + openHintsAndSolutionsDialog() + pressRevealHintButton(hintPosition = 0) + + onView(withId(R.id.hints_and_solution_summary)) + .inRoot(isDialog()) + .perform(openClickableSpan("test_skill_id_1 concept card")) + + testCoroutineDispatchers.runCurrent() + + onView(withText("Concept Card")).inRoot(isDialog()).check(matches(isDisplayed())) + onView(withText("Another important skill")).inRoot(isDialog()).check(matches(isDisplayed())) + onView(withId(R.id.concept_card_toolbar)).check(matches(isDisplayed())) + + onView(withContentDescription(R.string.navigate_up)).perform(click()) + + testCoroutineDispatchers.runCurrent() + onView(withId(R.id.concept_card_toolbar)).check(doesNotExist()) + } + explorationDataController.stopPlayingExploration(isCompletion = false) + } + + private fun openClickableSpan(text: String): ViewAction { + return object : ViewAction { + override fun getDescription(): String = "openClickableSpan" + + override fun getConstraints(): Matcher = hasClickableSpanWithText(text) + + override fun perform(uiController: UiController?, view: View?) { + // The view shouldn't be null if the constraints are being met. + (view as? TextView)?.getClickableSpans()?.findMatchingTextOrNull(text)?.onClick(view) + } + } + } + + private fun List>.findMatchingTextOrNull(text: String) = + find { text in it.first }?.second + + private fun TextView.getClickableSpans(): List> { + val viewText = text + return (viewText as Spannable).getSpans( + /* start= */ 0, /* end= */ text.length, ClickableSpan::class.java + ).map { + viewText.subSequence(viewText.getSpanStart(it), viewText.getSpanEnd(it)).toString() to it + } + } + + private fun hasClickableSpanWithText(text: String): Matcher { + return object : TypeSafeMatcher(TextView::class.java) { + override fun describeTo(description: Description?) { + description?.appendText("has ClickableSpan with text")?.appendValue(text) + } + + override fun matchesSafely(item: View?): Boolean { + return (item as? TextView)?.getClickableSpans()?.findMatchingTextOrNull(text) != null + } + } + } + private fun markSpotlightSeen(feature: Spotlight.FeatureCase) { val profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() spotlightStateController.markSpotlightViewed(profileId, feature)