android / architecture-components-samples

Samples for Android Architecture Components.
https://d.android.com/arch
Apache License 2.0
23.4k stars 8.29k forks source link

NavigationAdvancedSample incorrect handling of Fragment lifecycle events #948

Closed codefluencer closed 3 years ago

codefluencer commented 3 years ago

https://github.com/android/architecture-components-samples/blob/7466ae2015bb9bd9cf2c435a350070b29d71fd2b/NavigationAdvancedSample/app/src/main/java/com/example/android/navigationadvancedsample/NavigationExtensions.kt#L121

It looks like setting reordering to true reorders some Fragment transactions and therefore pause, start, create, destoyed events are executed in wrong order.

For example, if you click on leaderboard item in bottom sheet, onCreateView of Leaderboard is called and only afterwards onDestroyView of Title is executed. Which is wrong and can lead to some nasty bugs.

Removing reordering flag solves this problem completely.

Why is this needed?

com.example.android.navigationadvancedsample D/STACK: Title onCreateView com.example.android.navigationadvancedsample D/STACK: Leaderboard onCreateView com.example.android.navigationadvancedsample D/STACK: Title onDestroyView

@ianhanniballake

ianhanniballake commented 3 years ago

As per the documentation, you should always, always use setReorderingAllowed(true).

Think about any animation or transition, such as a cross fade. By necessity, both the outgoing fragment's view and incoming fragment's view must simultaneously exist to do a cross fade. That means that is absolutely expected that the incoming fragment has its view created, any animations are run, then the exiting fragment has its view destroyed.

You can find more information by searching the official issue tracker for setReorderingAllowed and making sure you upgrade to the latest version of Fragments (1.3.0-rc01) to ensure that you are using the new state manager and the many fixes that it brings specifically to cases like this (namely, overlap in the STARTED state that was fixed in b/161654580).