Issue with FragmentStateChanger samples

[condesales]

https://github.com/Zhuinden/simple-stack/blob/e4f358ed6c559a5cab1acc880a2148bdb5ae3691/simple-stack-example-kotlin-community-sample/src/main/java/com/zhuinden/simplestackkotlindaggerexample/FragmentStateChanger.kt#L24

I just noticed that when we do call the line above, there's a chance the fragment manager still haven't pushed its changes and the application might enter in an invalid stack state.

I was able to get that issue happening to me every time when starting the app with a deep link.

The solution I found was to always do fragmentManager.executePendingTransactions() whenever there's a state change to make sure we have the correct fragment manager state to make the required checks.

[Zhuinden]

Oh wow, thanks for the notice.

At some point I changed the samples to use commitAllowingStateLoss because I ran into animation quirks when using commitNow for some reason, that had been my original intention.

I guess for safety sake, it can make sense to use executePendingTransactions(). I might change the samples back to commitNowAllowingStateLoss() to make sure it works (though then the code should never run when there can actually be state loss, that came from commit being posted rather than being immediate).

[condesales]

I'm actually using simply .commit() on my transactions and haven't noticed any issues.

My only concern with executePendingTransactions() is that it can cause some performance issue, but so far it looks ok to me.

[Zhuinden]

Well .commit() + .executePendingTransactions() is equivalent (supposedly?) to commitNow :wink:

I think the big trick is that in production, we delayed the call to completionCallback.stateChangeComplete() by the duration of the fragment animation, and that disallowed the ability to run fragment transactions "too fast".

I wonder if moving the stateChangeComplete to fragmentTransaction.doOnCommit on its own would already do the trick. I think that's what I'll do in the samples.

[Zhuinden]

Did you figure out anything regarding this problem?

I seem to have forgotten about it. :| all I can say is that what we were doing is that while fragment transitions are ongoing, we block onDispatchTouchEvent.

[Zhuinden]

I cannot change my samples to use .doOnCommit { without converting to AndroidX, and that'd be a major change.

One possible recommendation is to call the callback.stateChangeComplete() callback after some delay, potentially the same delay as how much time the transaction takes.

But just doOnCommit should already work.

[Zhuinden]

The solution I found was to always do fragmentManager.executePendingTransactions() whenever there's a state change to make sure we have the correct fragment manager state to make the required checks.

In the end, I actually ended up doing the same thing in https://github.com/Zhuinden/simple-stack/commit/a9adb034d363497b0ac110ed2c0d19309e2f8c62

I should have done that according to the issue report earlier, sorry.

[condesales]

Glad this was an acceptable solution. I feel better knowing it wasn't too hacky in the end 😅

[Zhuinden]

It's actually literally the only thing that actually works reliably, and doesn't cause any odd side-effects for DialogFragments and transitions/animations. 👀

The original reason for not adding that to the samples was that originally, I used to have a handler.postDelayed that would call callback.stateChangeComplete(); only after the Fragment animations have finished (which were generally 250ms long, so it would be a postDelayed(.., 250L)).

But forcing the pending transactions is saner and safer, actually. 👍