adrielcafe / voyager

🛸 A pragmatic navigation library for Jetpack Compose
https://voyager.adriel.cafe
MIT License
2.4k stars 116 forks source link

Viewmodel not cleared when screen is disposed #210

Closed slickorange closed 8 months ago

slickorange commented 9 months ago

I am using AndroidScreen with HiltViewModel

When the screen is disposed, the viewmodel is not cleared.

object InviteCodesScreen : AndroidScreen() {
    @Composable
    override fun Content() {
        val screenModel = getViewModel<InviteCodesViewModel>()
        InviteCodesScreenUI(viewModel = screenModel)
        LifecycleEffect(
            onStarted = {
                screenModel.load()
            },
            onDisposed = {
                Timber.i("The screen is disposed...")
            })
    }
}
@HiltViewModel
class InviteCodesViewModel @Inject constructor(
    application: Application,
    val accessManager: AccessManager
) : BaseViewModel(application) {

    var uiState by mutableStateOf(InviteCodesUiState())

    fun load() {
        uiState = uiState.copy(accessDefinitions = accessManager.shareableDefinitions ?: emptyList())
    }

    override fun onCleared() {
        Timber.i("Viewmodel cleared")
    }

Another issue, which I am not sure is related, is that screens of the same type share viewmodels. Eg. I have a LessonScreen which displays a lesson. When I go from "lesson A" to "lesson B" by pushing a new LessonScreen the screen for LessonB does not get it's own instance of the viewmodel (this was the behaviour when I used RC 5, but not what I see with 7)

I am pretty stuck and would appreciate any help

KoTius commented 9 months ago
  1. Probably because you use singleton for your screen.
  2. Because you don't provide a tag when you get the viewModel.
programadorthi commented 9 months ago

I found the issue in the pull request #162 from @aftabahmadTW that removed the LifecycleOwner responsible to manager the ViewModel lifecycle. Without that the ViewModel lifecycle will be scoped to current Activity and keeping alive until the Activity is destroyed. CC @adrielcafe @DevSrSouza

slickorange commented 9 months ago
  1. Probably because you use singleton for your screen.

    1. Because you don't provide a tag when you get the viewModel.

As I mentioned, this used to work and only started happening once I updated to the latest version. (Without making any changes to my viewmodels or navigation.

Also not sure what you mean by providing a tag - I am just using the method showed in the documentation..

programadorthi commented 9 months ago

@slickorange try rollback to RC06. Pull request #162 is on the RC07. We will try fix that 👍

slickorange commented 9 months ago

I am having trouble with RC06 and the latest version of Jetpack compose.

osrl commented 9 months ago

I'm on RC06 and it's not working

slickorange commented 9 months ago

Any updates on this?

slickorange commented 8 months ago

I am still seeing this issue with the latest version (1.0.0-rc08)

DevSrSouza commented 8 months ago

The main issue here is that we change the behavior of AndroidScreenLifecycle, removing it from the AndroidScreen and injecting it by default for all screens. Your implementation of hilt viewModel extension was based on the fact that the Screen should be a AndroidScreenLifecycle. I figure this out late yeasterday.

The PR #212 from @programadorthi fix that issue by not casting Screen.lifeycleOwner to AndroidScreenLifecycle and instead using LocalLifecycleOwner, we should try reopen it and merge it to next release.

DevSrSouza commented 8 months ago

Fixed on 1.0.0-rc09