goTo(screen) should not navigate if top of stack == screen

[edenman]

I can write an extension method that does this for me:

fun Navigator.goToIfNecessary(screen: Screen) {
    if (peek() != screen) {
        goTo(screen)
    }
}

but I'd love for this to be the default behavior so I don't have to remember to call it anywhere there could be a duplication. Thoughts?

[ZacSweers]

I'd be open to this as a PR to NavigatorImpl. It is a bit of an odd case, but maybe we could make it configurable in rememberCircuitNavigator when creating the navigator. CC @stagg

[ZacSweers]

The main question would be what to do if someone tries it. I'm not a big fan of silently no-op-ing 🤔

[edenman]

Yeah. I'll admit it's a bit unexpected as the caller, but I can't imagine a situation where you'd want to navigate anywhere if you passed in the same screen we're already on. Maybe some apps would want to treat this as a refresh? I guess there could be a onGoToSameScreen callback but that seems like overkill.

[stagg]

Seems like a resonable guard to have. Think if you'd want to show the same Screen again it could easily be done with an unequal value. A sort of in place replace would still need to be done with a pop + push.

The main question would be what to do if someone tries it. I'm not a big fan of silently no-op-ing 🤔

This does open up goTo to return a result, either a success/fail boolean or a more descriptive enum.

[ZacSweers]

That's an interesting idea, and would give intercepting navigators the ability to communicate more too. I like it. I think starting with a Boolean return is good, we could look at an enum if it's really needed later

[edenman]

I'm not convinced returning a boolean/enum is the right ergonomic choice but obviously this is not my project so yall are free to do what you want.

That said, in my experience with Flow's goBack method that returned a boolean, it was a frequent source of bugs because inevitably callers would forget to check the return value. Or they'd do val ignored = flo.goBack().

I'd propose just changing the behavior, and optionally add a top-level onGoToSameScreen callback if there are apps that want to crash or log an error or show a toast/dialog or shake the screen or whatever.

[ZacSweers]

Implemented in #1331