navigator.pop() followed by navigator.push(screen) does not dispose the ScreenModel from the popped screen

[projectdelta6]

Describe the bug
I have been trying to figure out why my screenModel retains its state and data after the screen is popped and the screenModel should be disposed, I have my screen defined as this:

class ScreenB(
	private val data: BDataClass
) : Screen {
	@Composable
	override fun Content() {
		val navigator = LocalNavigator.currentOrThrow
		val viewModel = getScreenModel<ScreenBViewModel>(
			parameters = {
				ParametersHolder().apply {
					add(data)
				}
			}
		)
		ScreenBContent(
			viewModel = viewModel,
			onNavigateBack = {
				navigator.pop()
			},
			onNavigateScreenC = { id ->
				navigator.pop()
				navigator.push(ScreenC(id))
			}
		)
}

//ScreenModel
class ScreenBViewModel(
	private val data: BDataClass
) : ScreenModel {
	init {
		Log.v("testing", "Created ScreenBViewModel")
	}

	override fun onDispose() {
		super.onDispose()
		Log.v("testing", "Disposed ScreenBViewModel")
	}
}

//Koin Module:
val viewModelModule = module {
	factory { params ->
		ScreenBViewModel(params.get())
	}
}

Then if I navigate from ScreenA to ScreenB, I see the "Created ScreenBViewModel" Log
Then if I navigate back (either with system back button or my onNavigateBack callback I do see the "Disposed ScreenBViewModel" (this is as expected)
however if, while on ScreenB, I call my onNavigateScreenC callback I do not see the "Disposed ScreenBViewModel" and then if I navigate back to ScreenA and then forward into ScreenB again I do not see the "Created ScreenBViewModel" Log meaning that this is still using the previous instance that should have been disposed.

in Testing I have found that if I replace the

navigator.pop()
navigator.push(ScreenC(id))

with

navigator.replace(ScreenC(id))

then the ScreenModel is disposed correctly which is fine for this example but in other places I may need to pop 2 or 3 times before pushing a new screen, and I need to screenModel(s) to be disposed correctly in those instances

To Reproduce
Steps to reproduce the behavior:

1. Set up App with ScreenA, ScreenB(with ViewModel) and ScreenC
2. from ScreenA push ScreenB (with some data for the ScreenModel)
3. from ScreenB pop and then push ScreenC
4. from ScreenC pop (you should now be back on ScreenA)
5. from ScreenA push ScreenB (with different data than on step 1)
6. See that ScreenB's ScreenModel still has the data from step 1

Expected behavior
I would expect that is I pop a screen, then it's associated ScreenModel should be disposed. Even if that pop is immediately followed by a push.

Voyager module and version:

• voyager-navigator:1.0.0
• voyager-screenmodel:1.0.0
• voyager-tab-navigator:1.0.0
• voyager-transitions:1.0.0
• voyager-koin:1.0.0

Koin module and version:

• koin-bom:3.5.3
• koin-core
• koin-compose
• koin-android

Snippet or Sample project to help reproduce
See code snippet above

[DevSrSouza]

I will have to take a look on this. We have a popUtil function, we could have a replaceUntil as well, this would combine multiples pops and the push at the same time.

[DevSrSouza]

The way that Voyager disposes the Screens and ViewModel/ScreenModel is the next tick of the composition when the event is a dispose event, when you do pop+ push at the same time, the last event will be a push, so voyager can't dispose the screen that you have done pop. This is the nature of the library being built fully with Compose.

We have introduce a new way of handling disposing for Transition API that is still experimental, it does not relay on the lastEvent, I have not tested, but maybe it could work with pop + push, can you have a look?
#436

The docs are here: https://voyager.adriel.cafe/transitions-api/
Is the disposeScreenAfterTransitionEnd = true

We are considering replace the StepDisposable with this approach if is stable enough.

[projectdelta6]

I tried upgrading to 1.1.0-beta02 and using disposeScreenAfterTransitionEnd = true as you suggested but I am now getting the Screen was used multiple times crash mentioned in the docs when I use either pop + push or just replace when navigating from ScreenB to ScreenC
I have tried setting a ScreenKey on the screen using the class name + data ID but I still get the crash... the error is Key ScreenC:{ID}:transition was used multiple times

[DevSrSouza]

Try to use uniqueScreenKey instead

[projectdelta6]

ah I didn't realise uniqueScreenKey was an actual val...
So that stops the crash, but now it causes ScreenC's ScreenModel to be re-created in an infinite loop for some reason, I'm using replace to go from ScreenB to ScreenC
and I get the same using pop + push

[projectdelta6]

ah I didn't realise uniqueScreenKey was an actual val... So that stops the crash, but now it causes ScreenC's ScreenModel to be re-created in an infinite loop for some reason, I'm using replace to go from ScreenB to ScreenC and I get the same using pop + push

I realised this issue was because I had the key as override val key: ScreenKey get() = uniqueScreenKey, so I changed it to override val key: ScreenKey = uniqueScreenKey and now using pop + push it appears to work with ScreenB's ScreenModel being disposed, though I do also see the log saying that ScreenC's ScreenModel was also disposed but ScreenC appears for now to be working. However if I use replace instead I do still get the Screen was used multiple times crash.

[ashwani280496]

Can someone write the steps to solve this

[acmpo6ou]

The way that Voyager disposes the Screens and ViewModel/ScreenModel is the next tick of the composition when the event is a dispose event, when you do pop+ push at the same time, the last event will be a push, so voyager can't dispose the screen that you have done pop. This is the nature of the library being built fully with Compose.

We have introduce a new way of handling disposing for Transition API that is still experimental, it does not relay on the lastEvent, I have not tested, but maybe it could work with pop + push, can you have a look? #436

The docs are here: https://voyager.adriel.cafe/transitions-api/ Is the disposeScreenAfterTransitionEnd = true

We are considering replace the StepDisposable with this approach if is stable enough.

This really helped me, thank you. If I add a delay in between the pop and push calls, it will work:

val coroutineScope = rememberCoroutineScope()
...
Button(
    onClick = {
        navigator.pop()
        // don't push immediately, wait for Compose tick to happen, and for the screen model to be disposed
        coroutineScope.launch {
            // the tick can be just 1 millisecond, it doesn't have to be long
            delay(1)
            navigator push MyAwesomeNextScreen()
        }
    }
)
...

This works, but because of the delay, even thought it's tiny (just 1 millisecond), the screen flickers, at least in my use case, it doesn't look very nice.

@DevSrSouza Is there an API we could use to dispose a screen model manually? Without needing a delay? I have tried using the link you provided, but I don't use transitions on Desktop, I present my screens side-by-side instead. I also tried setting
disposeBehavior = NavigatorDisposeBehavior(disposeSteps = false), in the Navigator, but that didn't help.

[hristogochev]

Hello! A little late to the discussion but I've found a solution that acts as replaceUntil:

import cafe.adriel.voyager.core.stack.Stack

fun <Item> Stack<Item>.replaceUntil(item: Item, predicate: (Item) -> Boolean) {
    val items = items.takeUntilInclusive(predicate) + item
    this.replaceAll(items)
}

fun <T> List<T>.takeUntilInclusive(predicate: (T) -> Boolean): List<T> {
    val result = mutableListOf<T>()
    for (item in this) {
        result.add(item)
        if (predicate(item)) break
    }
    return result
}

Due to how voyager works internally the items inserted into the replaceAll call that already exist won't be recreated, all new ones will be created and all items that are no longer there will get disposed!