From 382d57ee3446db468c667db84e9c4610654d422a Mon Sep 17 00:00:00 2001 From: "mert.yuksel" Date: Tue, 25 Jun 2024 15:34:55 +0200 Subject: [PATCH 1/2] Create stage fragment holder to hold fragment references by their tags until they're destroyed. FragmentManagerController use this class to retrieve fragment instances if it can't find those instances in FragmentManager. --- gradle/libs.toml | 2 + medusalib/build.gradle | 8 ++ .../trendyol/medusalib/TestChildFragment.kt | 36 +++++++ .../trendyol/medusalib/TestParentFragment.kt | 19 ++++ .../navigator/ConcurrentTransactionTest.kt | 83 ++++++++++++++++ .../navigator/MultipleStackNavigator.kt | 96 ++++++++++++++----- .../controller/FragmentManagerController.kt | 21 +++- .../controller/StagedFragmentHolder.kt | 26 +++++ .../controller/StagedFragmentHolderTest.kt | 69 +++++++++++++ 9 files changed, 329 insertions(+), 31 deletions(-) create mode 100644 medusalib/src/androidTest/java/com/trendyol/medusalib/TestChildFragment.kt create mode 100644 medusalib/src/androidTest/java/com/trendyol/medusalib/TestParentFragment.kt create mode 100644 medusalib/src/androidTest/java/com/trendyol/medusalib/navigator/ConcurrentTransactionTest.kt create mode 100644 medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolder.kt create mode 100644 medusalib/src/test/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolderTest.kt diff --git a/gradle/libs.toml b/gradle/libs.toml index 893ad22..bba23ba 100644 --- a/gradle/libs.toml +++ b/gradle/libs.toml @@ -13,9 +13,11 @@ androidXArchCoreTesting = "androidx.arch.core:core-testing:2.2.0" androidXFragmentTesting = "androidx.fragment:fragment-testing:1.8.0" androidXTestCoreKtx = "androidx.test:core-ktx:1.4.0" androidXTestExtJunit = "androidx.test.ext:junit:1.1.3" +androidXTestRunner = "androidx.test:runner:1.5.2" junit = "junit:junit:4.13.2" robolectric = "org.robolectric:robolectric:4.12.2" truth = "com.google.truth:truth:1.4.2" +espressoTest = "androidx.test.espresso:espresso-core:3.6.0" [versions] kotlin = "2.0.0" diff --git a/medusalib/build.gradle b/medusalib/build.gradle index e529714..9be47f3 100755 --- a/medusalib/build.gradle +++ b/medusalib/build.gradle @@ -57,6 +57,14 @@ dependencies { testImplementation libs.junit testImplementation libs.robolectric testImplementation libs.truth + + androidTestImplementation libs.androidXTestExtJunit + androidTestImplementation libs.androidXTestRunner + debugImplementation libs.androidXFragmentTesting + androidTestImplementation libs.truth + androidTestImplementation libs.junit + androidTestImplementation libs.espressoTest + } repositories { diff --git a/medusalib/src/androidTest/java/com/trendyol/medusalib/TestChildFragment.kt b/medusalib/src/androidTest/java/com/trendyol/medusalib/TestChildFragment.kt new file mode 100644 index 0000000..535844b --- /dev/null +++ b/medusalib/src/androidTest/java/com/trendyol/medusalib/TestChildFragment.kt @@ -0,0 +1,36 @@ +package com.trendyol.medusalib + +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import android.widget.TextView +import androidx.fragment.app.Fragment + +private const val KEY_TITLE = "title" + +class TestChildFragment : Fragment() { + + var onFragmentVisibleAgain: (() -> Unit)? = null + + override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? { + return TextView(requireContext()).apply { + text = requireArguments().getString(KEY_TITLE) + } + } + + override fun onHiddenChanged(hidden: Boolean) { + super.onHiddenChanged(hidden) + if (hidden.not() && view != null) { + onFragmentVisibleAgain?.invoke() + } + } + + companion object { + fun newInstance(title: String): TestChildFragment { + return TestChildFragment().apply { + arguments = Bundle().apply { putString(KEY_TITLE, title) } + } + } + } +} diff --git a/medusalib/src/androidTest/java/com/trendyol/medusalib/TestParentFragment.kt b/medusalib/src/androidTest/java/com/trendyol/medusalib/TestParentFragment.kt new file mode 100644 index 0000000..a60b3ed --- /dev/null +++ b/medusalib/src/androidTest/java/com/trendyol/medusalib/TestParentFragment.kt @@ -0,0 +1,19 @@ +package com.trendyol.medusalib + +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import android.widget.FrameLayout +import androidx.fragment.app.Fragment + +class TestParentFragment : Fragment() { + + override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? { + return FrameLayout(requireContext()).apply { id = CONTAINER_ID } + } + + companion object { + const val CONTAINER_ID = 1_000 + } +} diff --git a/medusalib/src/androidTest/java/com/trendyol/medusalib/navigator/ConcurrentTransactionTest.kt b/medusalib/src/androidTest/java/com/trendyol/medusalib/navigator/ConcurrentTransactionTest.kt new file mode 100644 index 0000000..22fdf8d --- /dev/null +++ b/medusalib/src/androidTest/java/com/trendyol/medusalib/navigator/ConcurrentTransactionTest.kt @@ -0,0 +1,83 @@ +package com.trendyol.medusalib.navigator + +import androidx.fragment.app.testing.FragmentScenario +import androidx.fragment.app.testing.launchFragmentInContainer +import androidx.fragment.app.testing.withFragment +import androidx.lifecycle.Lifecycle +import androidx.test.espresso.Espresso.onView +import androidx.test.espresso.assertion.ViewAssertions.matches +import androidx.test.espresso.matcher.ViewMatchers.isDisplayed +import androidx.test.espresso.matcher.ViewMatchers.withText +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.trendyol.medusalib.TestChildFragment +import com.trendyol.medusalib.TestParentFragment +import com.trendyol.medusalib.navigator.transaction.NavigatorTransaction +import com.trendyol.medusalib.navigator.transaction.TransactionType +import org.junit.Test +import org.junit.runner.RunWith +import java.util.concurrent.CountDownLatch + + +@RunWith(AndroidJUnit4::class) +class ConcurrentTransactionTest { + + @Test + fun givenNavigatorWithShowAndHideWhenFragmentResetsTheCurrentTabAndStartsAnotherFragmentThenItMustNotThrowAnyExceptions() { + // Given + val transaction = CountDownLatch(1) + var caughtException: Throwable? = null + var navigator: Navigator? = null + val scenario = launchFragmentInContainer( + initialState = Lifecycle.State.INITIALIZED + ) + scenario.moveToState(Lifecycle.State.RESUMED) + val rootFragment = TestChildFragment.newInstance("Root") + val expectedFragment = TestChildFragment.newInstance("ExpectedFragment") + + scenario.withFragment { navigator = createNavigator(rootFragment = rootFragment) } + scenario.moveToState(Lifecycle.State.RESUMED) + + // When + rootFragment.onFragmentVisibleAgain = { + resetTabAndStartFragment(navigator!!, expectedFragment) + .onSuccess { transaction.countDown() } + .onFailure { + caughtException = it + transaction.countDown() + } + } + scenario.startAndDismissAFragment(navigator!!) + + transaction.await() + caughtException?.let { throw it } + scenario.moveToState(Lifecycle.State.RESUMED) + onView(withText("ExpectedFragment")).check(matches(isDisplayed())) + } + + private fun FragmentScenario.startAndDismissAFragment(navigator: Navigator) { + onFragment { navigator.start(TestChildFragment.newInstance("SecondFragment")) } + moveToState(Lifecycle.State.RESUMED) + onFragment { navigator.goBack() } + } + + private fun resetTabAndStartFragment( + navigator: Navigator, + expectedFragment: TestChildFragment + ): Result { + return runCatching { + navigator.resetCurrentTab(true) + navigator.start(expectedFragment) + } + } + + private fun TestParentFragment.createNavigator(rootFragment: TestChildFragment): MultipleStackNavigator { + return MultipleStackNavigator( + fragmentManager = this.childFragmentManager, + containerId = TestParentFragment.CONTAINER_ID, + rootFragmentProvider = listOf({ rootFragment }), + navigatorConfiguration = NavigatorConfiguration( + defaultNavigatorTransaction = NavigatorTransaction(TransactionType.SHOW_HIDE) + ) + ).apply { this.initialize(null) } + } +} \ No newline at end of file diff --git a/medusalib/src/main/java/com/trendyol/medusalib/navigator/MultipleStackNavigator.kt b/medusalib/src/main/java/com/trendyol/medusalib/navigator/MultipleStackNavigator.kt index 3c122e6..62df4cd 100755 --- a/medusalib/src/main/java/com/trendyol/medusalib/navigator/MultipleStackNavigator.kt +++ b/medusalib/src/main/java/com/trendyol/medusalib/navigator/MultipleStackNavigator.kt @@ -7,6 +7,7 @@ import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.MutableLiveData import com.trendyol.medusalib.navigator.controller.FragmentManagerController +import com.trendyol.medusalib.navigator.controller.StagedFragmentHolder import com.trendyol.medusalib.navigator.data.FragmentData import com.trendyol.medusalib.navigator.data.StackItem import com.trendyol.medusalib.navigator.tag.TagCreator @@ -28,7 +29,8 @@ open class MultipleStackNavigator( private val fragmentManagerController = FragmentManagerController( fragmentManager, containerId, - navigatorConfiguration.defaultNavigatorTransaction + navigatorConfiguration.defaultNavigatorTransaction, + StagedFragmentHolder(mutableMapOf()) ) private val fragmentStackStateMapper = FragmentStackStateMapper() @@ -77,10 +79,13 @@ open class MultipleStackNavigator( fragmentStackState.notifyStackItemAddToCurrentTab( StackItem( fragmentTag = createdTag, - groupName = fragmentGroupName - ) + groupName = fragmentGroupName, + ), + ) + fragment.observeFragmentLifecycle( + ::onFragmentViewCreated, + ::onFragmentDestroy ) - notifyFragmentDestinationChange(fragment) } override fun goBack() { @@ -144,9 +149,16 @@ open class MultipleStackNavigator( val createdTag = tagCreator.create(rootFragment) val rootFragmentData = FragmentData(rootFragment, createdTag) fragmentStackState.switchTab(currentTabIndex) - fragmentStackState.notifyStackItemAdd(currentTabIndex, StackItem(fragmentTag = createdTag)) + fragmentStackState.notifyStackItemAdd( + currentTabIndex, + StackItem(fragmentTag = createdTag), + ) fragmentManagerController.addFragment(rootFragmentData) - notifyFragmentDestinationChange(rootFragment) + + rootFragment.observeFragmentLifecycle( + onViewCreated = ::onFragmentViewCreated, + onFragmentDestroy = ::onFragmentDestroy + ) } else { val upperFragmentTag: String = getCurrentFragmentTag() val upperFragment: Fragment? = fragmentManagerController.getFragment(upperFragmentTag) @@ -154,7 +166,10 @@ open class MultipleStackNavigator( val newDestination: Fragment = upperFragment ?: getRootFragment(currentTabIndex) val newDestinationTag: String = tagCreator.create(newDestination) - notifyFragmentDestinationChange(newDestination) + newDestination.observeFragmentLifecycle( + onViewCreated = ::onFragmentViewCreated, + onFragmentDestroy = ::onFragmentDestroy + ) fragmentManagerController.enableFragment(newDestinationTag) } } @@ -163,6 +178,7 @@ open class MultipleStackNavigator( clearAllFragments() fragmentStackState.clear() initializeStackState() + } override fun resetWithFragmentProvider(rootFragmentProvider: List<() -> Fragment>) { @@ -237,14 +253,20 @@ open class MultipleStackNavigator( val stackItem = StackItem(fragmentTag = createdTag) fragmentStackState.setStackCount(rootFragmentProvider.size) - fragmentStackState.notifyStackItemAdd(tabIndex = initialTabIndex, stackItem = stackItem) + fragmentStackState.notifyStackItemAdd( + tabIndex = initialTabIndex, + stackItem = stackItem, + ) fragmentStackState.switchTab(initialTabIndex) val rootFragmentTag = fragmentStackState.peekItem(initialTabIndex).fragmentTag val rootFragmentData = FragmentData(rootFragment, rootFragmentTag) fragmentManagerController.addFragment(rootFragmentData) navigatorListener?.onTabChanged(navigatorConfiguration.initialTabIndex) - notifyFragmentDestinationChange(rootFragment) + rootFragment.observeFragmentLifecycle( + ::onFragmentViewCreated, + ::onFragmentDestroy + ) } private fun loadStackStateFromSavedState(savedState: Bundle) { @@ -271,13 +293,29 @@ open class MultipleStackNavigator( val rootFragmentData = FragmentData(rootFragment, createdTag) fragmentStackState.notifyStackItemAdd( fragmentStackState.getSelectedTabIndex(), - StackItem(createdTag) + StackItem(createdTag), ) fragmentManagerController.addFragment(rootFragmentData) - notifyFragmentDestinationChange(rootFragment) + rootFragment.observeFragmentLifecycle( + onViewCreated = ::onFragmentViewCreated, + onFragmentDestroy = ::onFragmentDestroy + ) } else { fragmentManagerController.enableFragment(upperFragmentTag) - notifyFragmentDestinationChange(upperFragment) + upperFragment.observeFragmentLifecycle( + onViewCreated = ::onFragmentViewCreated, + onFragmentDestroy = ::onFragmentDestroy + ) + } + } + + private fun onFragmentViewCreated(fragment: Fragment) { + destinationChangeLiveData.value = fragment + } + + private fun onFragmentDestroy(fragment: Fragment) { + if (destinationChangeLiveData.value == fragment) { + destinationChangeLiveData.value = null } } @@ -321,28 +359,34 @@ open class MultipleStackNavigator( return true } - private fun notifyFragmentDestinationChange(fragment: Fragment) { - fragment.lifecycle.addObserver(object: DefaultLifecycleObserver { + private fun Fragment.observeFragmentLifecycle( + onViewCreated: (Fragment) -> Unit, + onFragmentDestroy: (Fragment) -> Unit + ) { + this.lifecycle.addObserver(object : DefaultLifecycleObserver { override fun onStart(owner: LifecycleOwner) { + super.onStart(owner) owner.lifecycle.removeObserver(this) - fragment.viewLifecycleOwner.lifecycle.addObserver( - object : DefaultLifecycleObserver { - override fun onCreate(owner: LifecycleOwner) { - destinationChangeLiveData.value = fragment - } - - override fun onDestroy(owner: LifecycleOwner) { - if (destinationChangeLiveData.value == fragment) { - destinationChangeLiveData.value = null + val fragment = this@observeFragmentLifecycle + fragment + .viewLifecycleOwner + .lifecycle + .addObserver( + object : DefaultLifecycleObserver { + override fun onCreate(owner: LifecycleOwner) { + onViewCreated(fragment) + } + override fun onDestroy(owner: LifecycleOwner) { + onFragmentDestroy(fragment) + owner.lifecycle.removeObserver(this) } - owner.lifecycle.removeObserver(this) } - } - ) + ) } }) } + override fun onSaveInstanceState(outState: Bundle) { outState.putBundle(MEDUSA_STACK_STATE_KEY, fragmentStackStateMapper.toBundle(fragmentStackState)) } diff --git a/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/FragmentManagerController.kt b/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/FragmentManagerController.kt index f7a5219..2f2e6b6 100644 --- a/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/FragmentManagerController.kt +++ b/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/FragmentManagerController.kt @@ -17,9 +17,12 @@ import com.trendyol.medusalib.navigator.transaction.NavigatorTransaction import com.trendyol.medusalib.navigator.transaction.TransactionType import com.trendyol.medusalib.navigator.transitionanimation.TransitionAnimationType -class FragmentManagerController(private val fragmentManager: FragmentManager, - private val containerId: Int, - private val navigatorTransaction: NavigatorTransaction) { +internal class FragmentManagerController( + private val fragmentManager: FragmentManager, + private val containerId: Int, + private val navigatorTransaction: NavigatorTransaction, + private val stagedFragmentHolder: StagedFragmentHolder, +) { private var currentTransaction: FragmentTransaction? = null @@ -70,7 +73,7 @@ class FragmentManagerController(private val fragmentManager: FragmentManager, fun addFragment(fragmentData: FragmentData) { checkAndCreateTransaction() - + stageFragment(fragmentData) currentTransaction?.add(containerId, fragmentData.fragment, fragmentData.fragmentTag) commitAllowingStateLoss() } @@ -90,6 +93,7 @@ class FragmentManagerController(private val fragmentManager: FragmentManager, TransitionAnimationType.FADE_IN_OUT -> setCustomAnimations(R.anim.fade_in, R.anim.empty_animation) null -> { /* no-op */ } } + stageFragment(fragmentData) currentTransaction?.add(containerId, fragmentData.fragment, fragmentData.fragmentTag) } @@ -107,7 +111,7 @@ class FragmentManagerController(private val fragmentManager: FragmentManager, } private fun getFragmentWithExecutingPendingTransactionsIfNeeded(fragmentTag: String): Fragment? { - var fragment = getFragment(fragmentTag) + var fragment = getFragment(fragmentTag) ?: stagedFragmentHolder.getStagedFragment(fragmentTag) if (fragment == null && fragmentManager.executePendingTransactions()) { fragment = getFragment(fragmentTag) } @@ -174,4 +178,11 @@ class FragmentManagerController(private val fragmentManager: FragmentManager, currentTransaction = fragmentManager.beginTransaction() } } + + private fun stageFragment(fragmentData: FragmentData) { + stagedFragmentHolder.stageFragmentForCommit( + fragmentData.fragmentTag, + fragmentData.fragment, + ) + } } diff --git a/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolder.kt b/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolder.kt new file mode 100644 index 0000000..4fbb241 --- /dev/null +++ b/medusalib/src/main/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolder.kt @@ -0,0 +1,26 @@ +package com.trendyol.medusalib.navigator.controller + +import androidx.fragment.app.Fragment +import androidx.lifecycle.DefaultLifecycleObserver +import androidx.lifecycle.LifecycleOwner + +internal class StagedFragmentHolder constructor( + private val fragmentsByTags: MutableMap +) { + + fun stageFragmentForCommit(tag: String, fragment: Fragment) { + fragmentsByTags.put(tag, fragment) + fragment.lifecycle.addObserver(object : DefaultLifecycleObserver { + + override fun onDestroy(owner: LifecycleOwner) { + fragmentsByTags.remove(tag) + fragment.lifecycle.removeObserver(this) + super.onDestroy(owner) + } + }) + } + + fun getStagedFragment(tag: String): Fragment? { + return fragmentsByTags.get(tag) + } +} \ No newline at end of file diff --git a/medusalib/src/test/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolderTest.kt b/medusalib/src/test/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolderTest.kt new file mode 100644 index 0000000..7492253 --- /dev/null +++ b/medusalib/src/test/java/com/trendyol/medusalib/navigator/controller/StagedFragmentHolderTest.kt @@ -0,0 +1,69 @@ +package com.trendyol.medusalib.navigator.controller + +import androidx.fragment.app.testing.launchFragmentInContainer +import androidx.fragment.app.testing.withFragment +import com.google.common.truth.Truth +import com.trendyol.medusalib.TestChildFragment +import com.trendyol.medusalib.TestParentFragment +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class StagedFragmentHolderTest { + + @Test + fun `given a fragment for a tag doesnt exist, when getStagedFragment is called, then it must return null`() { + val sut = StagedFragmentHolder(mutableMapOf()) + + val actualFragment = sut.getStagedFragment("missing-fragment") + + Truth.assertThat(actualFragment).isNull() + } + + @Test + fun `given a fragment for a tag exists, when getStagedFragment is called, then it must return that fragment`() { + val sut = StagedFragmentHolder(mutableMapOf()) + + + launchFragmentInContainer { TestParentFragment() }.withFragment { + val fragmentToBeStaged = TestChildFragment.newInstance("title") + childFragmentManager + .beginTransaction() + .add(fragmentToBeStaged, "staged-child") + .commitNow() + + sut.stageFragmentForCommit("staged-child", fragmentToBeStaged) + + val actualFragment = sut.getStagedFragment("staged-child") + + Truth.assertThat(actualFragment).isEqualTo(fragmentToBeStaged) + } + } + + @Test + fun `given a fragment for a tag exists, when it is removed, then it must return null for that tag`() { + val sut = StagedFragmentHolder(mutableMapOf()) + + + launchFragmentInContainer { TestParentFragment() }.withFragment { + val fragmentToBeStaged = TestChildFragment.newInstance("title") + childFragmentManager + .beginTransaction() + .add(fragmentToBeStaged, "staged-child") + .commitNow() + sut.stageFragmentForCommit("staged-child", fragmentToBeStaged) + + childFragmentManager + .beginTransaction() + .remove(fragmentToBeStaged) + .commitNow() + + + val fragment = sut.getStagedFragment("staged-child") + + Truth.assertThat(fragment).isNull() + } + } + +} \ No newline at end of file From 7b253f34c3ac7209d75503965c3a1d77408e0cad Mon Sep 17 00:00:00 2001 From: "mert.yuksel" Date: Tue, 25 Jun 2024 16:07:08 +0200 Subject: [PATCH 2/2] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 839e1a7..cebe5a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - This changelog file +- Fix a bug where a fragment that we want to hide is not added to the fragment manager, +causing an IllegalStateException due to multiple fragment transactions occurring simultaneously +because of the executePendingTransactions() method call ### Changed