Investigate setting ScopedServices with deferred initialization

[Zhuinden]

This is still an issue when using Navigator to create a backstack (backstack is not accessible until Navigator.install() is called). Is there a better workaround rather than creating BackstackHolder?

@matejdro I see a new issue wasn't opened. Does Navigator.configure().setDeferredInitialization(true) + Navigator.executeDeferredInitialization suit your needs? You effectively get a restored Backstack but without setting the StateChanger.

[matejdro]

Sorry, I did not have the chance to check this out until now.

My use case is that I want scoped services to be provided by the dagger (sub)component. But this subcomponent needs backstack on its creation, which creates a bit of circular reference. Currently I'm solving this with the BackstackHolder pattern since none of the dependencies need Backstack immediately, but it is pretty hacky pattern.

This is the furthest I've gotten without BackstackHolder, with deferred initialisation and backstack set after initialisation:

val backstack = Navigator.configure()
    .setStateChanger(AsyncStateChanger(composeStateChanger))
    .setDeferredInitialization(true)
    .install(this, androidContentFrame, History.of(GrocyListKey()))

val activityComponent = component.createActivitySubcomponentFactory().create(backstack)
backstack.setScopedServices(activityComponent.provideScopedServices())

Navigator.executeDeferredInitialization(this)

However, this crashes with the following exception:

     Caused by: java.lang.IllegalStateException: Scope provider should be set before calling `setup()`
        at com.zhuinden.simplestack.Backstack.setScopedServices(Backstack.java:252)

[Zhuinden]

Oof, I see what you mean. Theoretically I could either say use assisted injection and rely on serviceBinder.getBackstack() for the backstack reference (which sounds like a nightmare so it's not what you actually want), OR technically I can relax the requirement to "set scoped services before calling setStateChanger" considering before the initial state change, it could be set without issues. 🤔

[matejdro]

Not sure how assisted injection would help here. Then I would have to pass down Backstack manually throughout the whole injection chain which completely defeats the purpose of using DI (might as well continue using BackstackHolder)

What if backstack.setup call would also be deferred if setDeferredInitialization is set? That way backstack is essentially a blank slate when returned from Navigator.install component and can be customized at will until executeDeferredInitialization is triggered.

[Zhuinden]

@matejdro released in 2.6.1.

The restriction is now moved so that setScopedServices/setGlobalServices is allowed after setup() and before setStateChanger().

0fcd49a

That way backstack is essentially a blank slate when returned from Navigator.install component

I can't do that because of how state restoration works, but I don't think you'll need that for now.

[Zhuinden]

@matejdro While the issue is closed, please report back if this served your purposes 🚀

[Zhuinden]

So basically your snippet with execute deferred init should work now #243 (comment)

[matejdro]

Wow that was fast. Thanks a lot!

[matejdro]

Actually, this does not seem to fix everything.

After a configuration change, app still crashes on the setScopedServices line:

     Caused by: java.lang.IllegalStateException: Scope provider should be set before the initial state change!
        at com.zhuinden.simplestack.Backstack.setScopedServices(Backstack.java:254)

It seems to me that at this point, backstack is already set from the saved instance state (but scoped services are not, since they are bound to the activity, which is being recreated) and thus I cannot set scoped services.

[Zhuinden]

Technically, Backstack exists within the retained scope of the activity, so after rotation, you'd be getting the same Backstack instance which "had already had the state changer set".

I do admit, I was testing this with unit tests, but not in an app.

Theoretically, the following would still give you correct behavior:

if(lastNonConfigurationInstance == null) {
    backstack.setScopedServices(scopedServices)
}

Assuming you aren't trying to pass in an Activity-reference or something. That would always be a memory leak, as the scopes are NOT recreated after config change. 🤔

The sample above is not ideal, but I am also not sure how I would reliably detect that it is allowed now. I guess similarly to when "force execute state change" is called (onDestroy) but it'd be an extra method on Backstack and I'm not sure if I want that. 🤔

Can you check if that works for you?

[matejdro]

Thanks, that worked. Yeah, I see now that setting scoped services at every start is a bit pointless, since they are retained.

Maybe hasScopedServices() check or publicizing didRunInitialStateChange() would make above sample more ideal?

[Zhuinden]

I will add a new method called canSetScopeProviders(), although API-wise it's a bit shady. Still less shady than having to remember if(lastNonConfigurationInstance == null) { though

[Zhuinden]

Should be actually done now by 5d8b822 and 2.6.2, meaning that if(lastNonConfigurationInstance == null) { can be replaced with if(backstack.canSetScopeProviders()) {

[matejdro]

Confirmed working, thanks!