ScreenModel is not disposed when poped and pushed a new screen

[sdercolin]

I found that a ScreenModel is not disposed (i.e. onDispose() not called; next call of rememberScreenModel() returns the existing instance) when doing:

// In `FooScreenModel`
navigator.popUntilRoot() // or pop() 
navigator push BarScreen

If only popUntilRoot()/pop() is called, without a following push(), it's properly disposed.

Version: 1.0.0

Workaround

The following code works (properly disposed)

navigator.replaceAll(MyRootScreen, BarScreen)

I'm not sure if the API is designed as so; if yes, maybe we can have some docs about it?

[DevSrSouza]

Can you share more details, maybe a sample code?

[sdercolin]

Sure. I made a minimal reproducible app with compose desktop.

// Main.kt

// imports omitted

class HomeScreen : Screen {

    @Composable
    override fun Content() {
        val navigator = LocalNavigator.currentOrThrow
        Column(
            Modifier.fillMaxSize(),
            verticalArrangement = Arrangement.Center,
            horizontalAlignment = Alignment.CenterHorizontally
        ) {
            Text("Home Screen")
            Button(onClick = { navigator.push(ContentScreenA()) }) {
                Text("Open Screen A")
            }
        }
    }
}

class ContentScreenAModel : ScreenModel {
    val text = "Content Screen A\nCreated at ${Calendar.getInstance().time}"

    override fun onDispose() {
        println("ContentScreenA disposed")
    }
}

class ContentScreenA : Screen {

    @Composable
    override fun Content() {
        val screenModel = rememberScreenModel { ContentScreenAModel() }
        val navigator = LocalNavigator.currentOrThrow
        Column(
            Modifier.fillMaxSize(),
            verticalArrangement = Arrangement.Center,
            horizontalAlignment = Alignment.CenterHorizontally
        ) {
            Text(screenModel.text)
            Button(onClick = {
                navigator.pop()
                navigator.push(ContentScreenB())
            }) {
                Text("Pop and Open Screen B")
            }
            Button(onClick = {
                navigator.replaceAll(listOf(HomeScreen(), ContentScreenB()))
            }) {
                Text("Replace with Screen B")
            }
            Button(onClick = {
                navigator.pop()
            }) {
                Text("Pop")
            }
        }
    }
}

class ContentScreenB : Screen {

    @Composable
    override fun Content() {
        val navigator = LocalNavigator.currentOrThrow
        Column(
            Modifier.fillMaxSize(),
            verticalArrangement = Arrangement.Center,
            horizontalAlignment = Alignment.CenterHorizontally
        ) {
            Text("Screen B")
            Button(onClick = { navigator.pop() }) {
                Text("Pop")
            }
        }
    }
}

@Composable
@Preview
fun App() {
    MaterialTheme {
        Navigator(HomeScreen())
    }
}

fun main() = application {
    Window(onCloseRequest = ::exitApplication) {
        App()
    }
}

https://github.com/adrielcafe/voyager/assets/7665216/6a6264f9-beee-4235-81c8-1afba7f29ca3

In the video, you can see that:

• When doing pop() or replaceAll() from screen A, the model is disposed(log printed and the created time is changed when the screen A is entered the next time)
• When doing pop() + push() from screen A, the model is not disposed(the created time remains unchanged)

Source code: voyager-test.zip

[DevSrSouza]

Thanks, I will investigate

[DevSrSouza]

Instead of

navigator.pop()
navigator.push(ContentScreenB())

Use:

navigator.replace(ContentScreenB())

[Syer10]

I've run into this issue as well, when we need to pop x screens with .popUntil, and then push a new screen.

[Syer10]

Ended up just pushing the screen once, and then use a .replace with the same screen

[DevSrSouza]

Basicly, you can't do pop and push at the same time.

Voyager in built in with Compose and it composition, so when you do a pop operation, it will have a recompisition and the lastEvent will change to pop event, when this happens, is when Voyager dispose the Screen and it ScreenModels as well.

By doing pop + push the next composition will be a PUSH event and the disposable will not happen, this is why exist the replace.

The navigator API should always be a single operation of the Voyager Stack API.

• push(screen) or push(list)
• replace(screen)
• replaceAll(screen) or replaceAll(list)
• pop
• popAll
• popUntil(predicate)

[sdercolin]

Understood. Maybe adding some descriptions in the Stack API doc would help users to be aware of this better?

Also I wonder if there's a way to do:

when stack is: A, B, C, D
pop until B and push E, so the stack will be: A, B, E

[Syer10]

The stack keeps all the changes you make to it, but if you don't replace or pop as the last action, it wont cleanup resources like ScreenModels

[DevSrSouza]

The issue is not with Stack API it self, it can handle it. But if you are listening a State of it in Compose, any state, not particular to Stack API, if you update two times the state, in the next composition you will receive only the last state, is how Compose composition works. The Navigator API is built with the composition events.

[sdercolin]

Thanks for the details!

Yes, I understand it's not an issue. To me, it could be more friendly to new users to have some explanations that we are not supposed to use pop + push. The pop + push way works at first glance: the screen stack is updated as expected. So it might be difficult for developers to be aware there's something unexpected happening (model not getting disposed). Another idea is to add some runtime warnings.

The current doc has full info though. People should understand the recommended usage after reading it. (I didn't read it carefully before running into this issue 😅 )

---

(Answering my own question) I think we can get the current stack contents via navigator.items, edit it whatever we like, and then pass it through replaceAll().

[DevSrSouza]

We will document it! Thanks for openning the issue, this was something that daily we understand but forgot to document! Glad that it was not a blocker.