Navigation action that removes part of the stack without altering the top key can cause detached fragments to not be removed when used with SimpleStateChanger

[Zhuinden]

Reported by JCR:

• Remove a key inside the stack

    val desiredKeyToRemove = backstack.getHistory<SimpleStackScreenKey>()
        .firstOrNull { it::class == keyToReplace }

    if (desiredKeyToRemove == null) {
        return
    } else {
        backstack.setHistory(
            History.builderFrom(backstack)
                .remove(desiredKeyToRemove)
                .build<Key>(),
            REPLACE
        )
    }

• Don't trigger navigation action with this (top key does not change)

simple-stack/simple-stack/src/main/java/com/zhuinden/simplestack/SimpleStateChanger.java

Lines 40 to 44 in d003d93

	if(stateChange.isTopNewKeyEqualToPrevious()) {
	completionCallback.stateChangeComplete();
	return;
	}
	navigationHandler.onNavigationEvent(stateChange);

• Key is no longer in the stack the next time top key changes, so fragment does not get removed by DefaultFragmentStateChanger (stays detached)

https://github.com/Zhuinden/simple-stack-extensions/blob/1296f6a07964be31220f4ab6b6af8724d6511f57/fragments/src/main/java/com/zhuinden/simplestackextensions/fragments/DefaultFragmentStateChanger.java#L189

---

Conclusion: SimpleStateChanger contains the built-in check from the original square/flow

https://github.com/square/flow/blame/438663b5755d7db124ed3d62889844bf565b15f7/flow/src/main/java/flow/KeyDispatcher.java#L65-L74

However in this case it doesn't work (as fragments have memory, while flow used to be used only with views (that have no memory)).

Proposal: what if

1.) SimpleStateChanger also calls "onNavigationEvent" when state change is not content equals (but top is the same)

2.) DefaultFragmentStateChanger does not apply animations for fragment transactions that don't change the top, but still applies the state changes themselves

However, the short-circuit for "is top the same" is very old. By making it "content equals", then if anyone ever relied on it actually always being "is top the same" would need to do the check themselves, which is behavior breaking change. 😓

[Zhuinden]

This is a very tricky problem. I advise using a custom StateChanger (which handles the case when top previous == top new && stateChange.previous != stateChange.new) that delegates to DefaultFragmentStateChanger in the common case.

[captainepoch]

Since the DefaultFragmentStateChanger is from the extensions library, and it might be possible not every single simple-stack user uses it, it would make sense to fix this at the SimpleStateChanger, since it's at the main library.

However, the short-circuit for "is top the same" is very old. By making it "content equals", then if anyone ever relied on it actually always being "is top the same" would need to do the check themselves, which is behavior breaking change.

This could be made in the next major version (to keep semver matched with the changes), and this fix can be in the next minor/patch version of the library.

[Zhuinden]

I do think about this change a lot, but it really will need to be 3.0 to fix it.

In the meantime, I recommend that if you know that the backstack manipulation doesn't change the top, then remove the fragment manually for now if this is viable.

I'm a bit uneasy about having to either recommend a workaround, or do something more workaround-like though, so I do think about it a lot.