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

GlobalServices instance is overridden by Navigator.install() even if already set #158

Closed Zhuinden closed 4 years ago

Zhuinden commented 5 years ago

Basically this means that the objects you set should be singletons at time of registration.

The original intent was that you create them once, and they won't be re-created again, as expected by things like ViewModelProviders.Factory.

Maybe I really did underthink this damn builder and should have used something like the ServiceBinder... 😞

Zhuinden commented 5 years ago

But it will only make sense to use ServiceBinder if I move it out of ScopedServices so that it is not a static inner class.

Zhuinden commented 5 years ago

Well technically it also works if the global services come from application class. The GlobalServices aren't used to build the global scope if it already exists, anyway.

Zhuinden commented 5 years ago

The problem with using ServiceBinder for this is that it has a @NonNull T getKey() method, but the global scope has no key.

I'd either have to dissect the ServiceBinder to have a "keyed" version and a "global service builder version with no key", or just keep it as is.

I'll err on the side of "whatever" and keep it as it is, although moving out the ServiceBinder was definitely the right decision.

matejdro commented 4 years ago

Maybe a good compromise would be to provide lazy initializer for GlobalServices to Navigator.Installer?

Maybe like that:

interface GlobalServicesFactory {
   public GlobalServices createGlobalServices()
}

installer.setGlobalServices(factory)

Then this factory would only be invoked if Navigator is not already set.

Zhuinden commented 4 years ago

Technically I'd imagine such API like this:

interface GlobalServices {
    interface Factory {
        GlobalServices create();
    }
}

then

installer.setGlobalServices(new MyGlobalServicesFactory())

I think the original idea why I didn't do that was that if you'd use a factory lambda here, then that would be a memory leak, but this already applies to StateChangeListeners.

Definitely a good compromise, not sure why I did not think of that.

matejdro commented 4 years ago

This does not need to be breaking though. You can leave old setGlobalServices method in. This method would just create dummy Factory that would return the provided value.

Zhuinden commented 4 years ago

Hmm. I was considering depreciating the old variant but you're right.

Zhuinden commented 4 years ago

In hindsight, I really should have most likely initially added a .Factory too, but alas, this worked for my purposes too for some time.