Show fragments correctly in bottom navigation #11838
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
planarvoid
changed the title
There was a problem hiding this comment.
Thanks @planarvoid! I'm curious what you think about using the fragmentmanager instead of caching the fragments locally - see my comment. I haven't tested the app yet.
| @@ -281,31 +288,45 @@ class WPMainNavigationView @JvmOverloads constructor( | |||
| } | |||
|
|
|||
| private inner class NavAdapter { | |||
| private val mFragments = SparseArray<Fragment>(numPages()) | |||
| private val mFragments = mutableMapOf<PageType, Fragment>() | |||
There was a problem hiding this comment.
Do we really need to manually keep references to the fragments? I'm not sure it's safe - what if we unintentionally re-use a destroyed fragment. It won't go though it's regular restoration process and we might end up in an undefined state. I'd personally rather rely on the FragmentManager implementation for the fragment caching, wdyt?
There was a problem hiding this comment.
Good point! I've removed the map and simplified the adapter. I think now it's much better.
WordPress/src/main/java/org/wordpress/android/ui/main/WPMainNavigationView.kt
Show resolved
Hide resolved
|
You can test the changes on this Pull Request by downloading the APK here. |
Fixes #11830
Previously we were using
FragmentManager.replaceto replace fragments on bottom navigation change. This call sets the Fragment as not active. However, we were still keeping and using the fragments in the sparse array. Overall this approach works but it causes some unnecessary fragment recreation and it's buggy with child fragments. If the parent is not active, the child fragments are also not active (even if the parent is actually visible). I'm replacing this functionality withshow/hidecalls instead. The fragments get initialized the first time you open the app and when you switch the tabs (or rotate the device), the fragments are retrieved from the fragment manager and shown/hidden.To test:
PR submission checklist:
RELEASE-NOTES.txtif necessary.