Zhuinden / simple-stack

[ACTIVE] Simple Stack, a backstack library / navigation framework for simpler navigation and state management (for fragments, views, or whatevers).
Apache License 2.0
1.36k stars 76 forks source link

Ability for scoped services to be initialized independently from state changing #212

Closed matejdro closed 4 years ago

matejdro commented 4 years ago

While attempting to make scoped-services constructor-injectable, I have noticed an interesting race condition.

Comment on the sample repo contains bulk of the explanation plus sample code.

Do you think it would be feasible to somehow separate scoped services initialisation? That way, when using constructor fragment injection, setStateChanger would not need to be called on different places depending on whether activity was created from saved state or not.

Zhuinden commented 4 years ago

If you're doing this:

https://github.com/matejdro/simple-stack-dagger-scoping-sample/blob/8cc8d09af4c72ec579b0e9945885c5c202dbac99/app/src/main/java/com/zhuinden/simplestack/navigator/BackstackActivity.kt#L51-L70

Then you don't need to use Navigator. As this is what Navigator is trying to do for you behind the scenes using a platform headless retained fragment that intercepts Activity callbacks. You can use Backstack directly.


However there is definitely an interesting (😞) race condition in regards to setting up a fragment factory, and initializing a Navigator instance.

super.onCreate() recreates the BackstackHost, and by that time, the fragmentFactory must be set for proper re-initialization. In fact, by that time, for Fragments to be initialized with scoped services that have their states properly restored, an initialization state change must have been executed, which means backstack.setup() happened, and so did backstack.setStateChanger().

If anything, this shows that Navigator is incompatible with FragmentFactory, which I find to be quite unfortunate. Thankfully your sample does not rely on it.


Internally, your state changer is wrapped with the managedStateChanger that actually executes the scoping-related changes. This is deferred because the active scope hierarchy wants to mimic the current active navigation history, based on which the current scoped service callbacks are triggered.

Theoretically it could be possible to force backstack.setup() to also eagerly construct a hierarchy, but I'm not sure it's a good idea to "handle state changes" while there is no real state changer. 🤔

(Especially considering restoration will happen when you call fromBundle() afterwards. How will I know if that is to happen or not?)



In your case, a simple workaround would be to backstack.setStateChanger(NoOpStateChanger()) where NoOpStateChanger would be

class NoOpStateChanger: StateChanger {
    override fun handleStateChange(stateChange: StateChange, callback: StateChanger.Callback) {
        callback.stateChangeComplete()
    }
}

Before super.onCreate(), and then you can set a real fragment state changer on the Backstack where you normally would (with no consideration for if(savedInstanceState == / != null)).

Setting a new state changer should trigger an initialization state change, and result in the state that you're looking for.

Zhuinden commented 4 years ago

@matejdro my outlined no-op init trick works as per https://github.com/matejdro/simple-stack-dagger-scoping-sample/pull/1

Not sure if I can offer anything safer on simple-stack side, to be honest. :thinking:

matejdro commented 4 years ago

Nice trick.