Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #5395: Fixed concept card not closing when opened from hint #5444

Closed
wants to merge 1 commit into from
Closed

Fix #5395: Fixed concept card not closing when opened from hint #5444

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions app/src/main/java/org/oppia/android/app/topic/conceptcard/ConceptCardFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class ConceptCardFragment : InjectableDialogFragment() {
companion object {

const val CONCEPT_CARD_FRAGMENT_ARGUMENTS_KEY = "ConceptCardFragment.arguments"
private var conceptCardFragment: ConceptCardFragment? = null

/** The fragment tag corresponding to the concept card dialog fragment. */
private const val CONCEPT_CARD_DIALOG_FRAGMENT_TAG = "CONCEPT_CARD_FRAGMENT"
Expand Down Expand Up @@ -82,11 +83,15 @@ class ConceptCardFragment : InjectableDialogFragment() {
fun dismissAll(fragmentManager: FragmentManager) {
val toDismiss = fragmentManager.fragments.filterIsInstance<ConceptCardFragment>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really helpful to better understand why ConceptCardFragment is missing from the fragments that the fragment manager is tracking.

I'm a bit concerned about tracking the fragment itself in this way because shared static state can be really tricky to test and and verify that it's always the correct value at the correct time, so it would be much preferable to try and get the original solution working if we can.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option might be to try and fetch all fragments by tag, but I'm not sure if that'll handle the concept card stacking situation correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion in meeting, it seems likely that the fragment manager might not be correct for the hints case since it's a dialog fragment (and the fragment manager is handled differently).

if (toDismiss.isNotEmpty()) {
val transaction = fragmentManager.beginTransaction()
for (fragment in toDismiss) {
transaction.remove(fragment)
fragmentManager.beginTransaction().apply {
for (fragment in toDismiss) {
remove(fragment)
}
commitNow()
}
transaction.commitNow()
} else {
conceptCardFragment?.dismiss()
conceptCardFragment = null
}
}

Expand All @@ -97,6 +102,7 @@ class ConceptCardFragment : InjectableDialogFragment() {
): ConceptCardFragment {
val conceptCardFragment = newInstance(skillId, profileId)
conceptCardFragment.showNow(fragmentManager, CONCEPT_CARD_DIALOG_FRAGMENT_TAG)
this.conceptCardFragment = conceptCardFragment
return conceptCardFragment
}
}
Expand Down
Loading