Skip to content

Commit

Permalink
Fix #5395: Fixed concept card not closing when opened from hint (#5509)
Browse files Browse the repository at this point in the history
<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## 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)


<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
<!-- Delete these section if this PR does not include UI-related
changes. -->
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
  • Loading branch information
Vishwajith-Shettigar authored Aug 30, 2024
1 parent 0ef7a46 commit a1fbe0c
Show file tree
Hide file tree
Showing 13 changed files with 110 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -192,4 +193,12 @@ class HintsAndSolutionDialogFragment :
isSolutionRevealed
)
}

/**
* Delegates the removal of all [ConceptCardFragment] instances
* to the [hintsAndSolutionDialogFragmentPresenter].
*/
fun dismissConceptCard() {
hintsAndSolutionDialogFragmentPresenter.dismissConceptCard()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,6 @@ class ExplorationActivityPresenter @Inject constructor(
showDialogFragmentBasedOnCurrentCheckpointState()
}

fun dismissConceptCard() {
getExplorationFragment()?.dismissConceptCard()
}

private fun updateToolbarTitle(explorationId: String) {
subscribeToExploration(
explorationDataController.getExplorationById(profileId, explorationId).toLiveData()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,5 @@ class ExplorationFragment : InjectableFragment() {
explorationFragmentPresenter.viewSolution()
}

fun dismissConceptCard() = explorationFragmentPresenter.dismissConceptCard()

fun getExplorationCheckpointState() = explorationFragmentPresenter.getExplorationCheckpointState()
}
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,6 @@ class ExplorationFragmentPresenter @Inject constructor(
getStateFragment()?.viewSolution()
}

fun dismissConceptCard() = getStateFragment()?.dismissConceptCard()

fun getExplorationCheckpointState() = getStateFragment()?.getExplorationCheckpointState()

private fun getStateFragment(): StateFragment? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,6 @@ class StateFragment :
stateFragmentPresenter.viewSolution()
}

fun dismissConceptCard() = stateFragmentPresenter.dismissConceptCard()

fun getExplorationCheckpointState() = stateFragmentPresenter.getExplorationCheckpointState()

override fun onSaveInstanceState(outState: Bundle) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class ConceptCardFragmentTestActivity :
}

override fun dismissConceptCard() {
getConceptCardFragment()?.dismiss()
ConceptCardFragment.dismissAll(supportFragmentManager)
}

private fun getConceptCardFragment(): ConceptCardFragment? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,6 @@ class QuestionPlayerFragment :
questionPlayerFragmentPresenter.revealSolution()
}

fun dismissConceptCard() = questionPlayerFragmentPresenter.dismissConceptCard()

companion object {

/** Arguments key for [QuestionPlayerFragment]. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2559,6 +2562,92 @@ class ExplorationActivityTest {
}
}

@Test
@RunOn(TestPlatform.ROBOLECTRIC) // TODO(#3858): Enable for Espresso.
fun testExpActivity_openConceptCard_selectNavigationUp_conceptCardCloses() {
markAllSpotlightsSeen()
launch<ExplorationActivity>(
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<View> = 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<Pair<String, ClickableSpan>>.findMatchingTextOrNull(text: String) =
find { text in it.first }?.second

private fun TextView.getClickableSpans(): List<Pair<String, ClickableSpan>> {
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<View> {
return object : TypeSafeMatcher<View>(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)
Expand Down

0 comments on commit a1fbe0c

Please sign in to comment.