ViewModel onClear not called when do bottom navigation with compose
mengdd opened this issue · 4 comments
Background:
Using enro version: 1.6.0
Setup Bottom Navigation with Enro, UI is 100% built using Compose.
The MainActivity's code is similar with sample code here: https://github.com/isaac-udy/Enro/blob/b4ecd50c9acdf01ce2a003038ea4b87e01d720f0/example/src/main/java/dev/enro/example/MultistackCompose.kt
Each tab's content UI is built with Compose, and has its own ViewModel.
The issue
The onCleared() method of each tab's ViewModel will never be invoked, even if hitting back button until MainActivity destroy.
The code:
If one of my tab's UI code is like this:
@Composable
@ExperimentalComposableDestination
@NavigationDestination(DashboardKey::class)
fun DashboardScreen() {
val viewModel: DashboardViewModel = viewModel()
val text = viewModel.text.observeAsState()
Text(text = text.value ?: "")
}And the ViewModel is injected by Hilt:
@HiltViewModel
class DashboardViewModel @Inject constructor() : ViewModel() {
private val _text = MutableLiveData<String>().apply {
value = "This is dashboard Fragment"
}
val text: LiveData<String> = _text
init {
Log.i("viewmodel", "DashboardViewModel init ${hashCode()}")
}
override fun onCleared() {
super.onCleared()
Log.i("viewmodel", "DashboardViewModel onClear ${hashCode()}")
}
}Some clues
Dive into the code for a while, found the ViewModel clear logic is located here:
It waits an ON_DESTROY event to trigger the clear action.
But actually the lifecycle events only hitted ON_PAUSE:
Not sure under which condition it will hit ON_DESTROY:
Thanks for reporting this issue. I am currently on holiday and out of the country, so my replies to this issue will be rather slow in the next few weeks. However, I don't quite understand why this is a problem for you, so you might need to explain in more detail what you are trying to achieve and why this behaviour is not appropriate for you.
This behaviour is what I would expect, given the example code that you linked. In the example, when you press the "back" button, the EmptyBehavior asks the current composable container controller to be changed, but does not ask the current container to close or destroy the active destination. See the code below:
EmptyBehavior.Action {
composableManager.setActiveContainer(redController)
true
}The "true" being returned from the EmptyBehavior says "I have consumed the request, take no further action". The reason this behaviour is desirable is because the active Composable (and related ViewModel) on each tab is not supposed to be cleared when another tab is selected.
For example:
- There are two tabs, A and B
- Both tabs made an API request to load some data
- The user selects tab A for the first time, so it loads the data
- The user then selects tab B for the first time, so it loads the data
- The user presses the back button, which causes tab A to be selected. Tab A has already loaded the data and should not re-load the data.
- Now the re-selects tab B, but tab B should also not need to re-load the data, because it is a root level tab in a mutli-tab view.
Is there a reason that you want to completely destroy the backstack when another tab is selected? This shouldn't be too difficult to add as an option, but I am wary of doing so because it doesn't feel like the correct thing to me.
Hi @isaac-udy , thanks for the reply.
Totally got your point, when switching bottom tabs, there are good reasons we should keep the viewModels for each tab.
Maybe I'm not make it clear enough before.
The main point is, the viewModels of bottom tab will never be cleared, even if when MainActivity onDestroy.
Which happens when user click back bottom until exit the app.
A sample for the issue
I made a sample here: https://github.com/mengdd/enro-bottom-navigation-sample
to better elaborate the issue.
A brief introduction:
- there is a singleton shared state manager sending events every second.
- one tab's ViewModel (DashboardViewModel) collects the shared events in ViewModel's init block.
When user exit the app and back from launcher or recent app, go to the dashboard tab:
A new DashboardViewModel will be starting collecting the events too.
So the event log in collect block will be printed twice, if we keep doing so, more log will be duplicated.
Expected
We suppose when MainActivity onDestroy, the tabs' viewModel will all be cleared.
If the viewModelScope get cancelled from onCleared, the events won't be collected multiple times.
Maybe I'm not using Enro in right way, or not combining it with Hilt correctly?
The expected behavior would be somehow make the viewmodel init and clear as a couple,
So users won't carelessly init multiple times and not cleared once.
Thank you for putting together that example. I misunderstood your original message, and thought the ViewModels were only being cleared when the Activity was being destroyed. The behaviour you're seeing is definitely a bug!
I won't be able to release a proper fix that's integrated with the library until I am back in New Zealand, but I will look into a temporary workaround for you now.
I believe I have a temporary fix for this issue. Here's the code:
package /* whatever you like */
import androidx.fragment.app.FragmentActivity
import androidx.lifecycle.*
/**
* This object contains a temporary fix for an issue with Enro reported here: https://github.com/isaac-udy/Enro/issues/52
* To apply this fix, call `EnroComposableViewModelFix.applyFix` in an Activity's onCreate method, immediately after
* the call to `super.onCreate()`
*
* Example:
* ```
* class ExampleActivity : AppCompoatActivity() {
* override fun onCreate() {
* super.onCreate(savedInstanceState)
* EnroComposableViewModelFix.applyFix(this)
* }
* }
* ```
*/
@Suppress("UNCHECKED_CAST")
object EnroComposableViewModelFix {
fun applyFix(activity: FragmentActivity) {
activity.executeImmediatelyBeforeClearingViewModelStore {
activity.getComposableDestinationContextReferences()
.forEach {
it as LifecycleOwner
val lifecycleRegistry = it.lifecycle as LifecycleRegistry
lifecycleRegistry.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)
}
}
}
private fun FragmentActivity.executeImmediatelyBeforeClearingViewModelStore(block: () -> Unit) {
// This is the same way that the base ComponentActivity uses to determine
// if a ViewModelStore will be cleared or not, and this will execute
// See the ComponentActivity constructor (around line 250) to see the original code
lifecycle.addObserver(LifecycleEventObserver { _, event ->
if (event == Lifecycle.Event.ON_DESTROY) {
if (!isChangingConfigurations) {
block()
}
}
})
}
// Returns a list of all the top level "dev.enro.core.compose.ComposableDestinationContextReference" instances
// that are owned by a particular Activity. This method relies on reflection to access internal Enro classes.
private fun FragmentActivity.getComposableDestinationContextReferences(): List<Any> {
val destinationStorageClass = Class.forName("dev.enro.core.compose.EnroDestinationStorage") as Class<ViewModel>
val destinationStorageInstance = ViewModelProvider(this).get(destinationStorageClass)
val getDestinationsMethod = destinationStorageClass.getMethod("getDestinations")
val destinationMap = (getDestinationsMethod.invoke(destinationStorageInstance) as Map<String, Map<String, Any>>)
return destinationMap.values.flatMap { it.values }
}
}To apply this fix, call EnroComposableViewModelFix.applyFix(this) in an Activity's onCreate method, immediately after the call to super.onCreate(). Please let me know if this does not solve the issue as you would expect.
A proper fix will be included with the next release of Enro.
I will leave this issue open to post updates about the fix included with the next release of Enro, or to answer any questions/solve any problems related to the temporary fix. Let me know if you have any questions.