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.37k stars 74 forks source link

Service exit from all scopes callback? #151

Closed jamolkhon closed 5 years ago

jamolkhon commented 5 years ago

Is it possible to be notified when a service is destroyed completely, that is when it exist all scopes? ScopedServices.Scoped only notifies when services from any single scope. I need it to clear all disposables. Currently disposing inside onExitScope is dangerous because a service could be part of multiple scopes.

Zhuinden commented 5 years ago

I didn't expect the same service to be registered in every scope manually, as the idea was that if a service is shared then it'd be in a shared scope; technically from my side I expected GlobalServices to be used for such a thing (although of course that is the last thing you check during a lookup, as immediate parents are preferred).

Technically it's possible to track this (it'd be odd if not) but it'd be a new type of callback.


However, currently an amazingly tricky thing that you can do is that you actually get what scope tags you were registered with onEnterScope(String scopeTag), so if you collect those into a set, then in onExitScope(String scopeTag) you remove them from the set, and if the set is empty then you're not registered in any scopes.

jamolkhon commented 5 years ago

Do you consider disposing in onExitScope ignoring the scope parameter to be safe? I'm only using implicit scopes.

Zhuinden commented 5 years ago

Do you consider disposing in onExitScope ignoring the scope parameter to be safe?

I did, until I got this question 😄

This actually never came up and we've been using implicit scopes for quite a while now.

Technically there is nothing in ScopeManager that would stop it from being able to build/manage a Map<Object, Set<String>> where String is scope tags it was registered with as it is binding services to scope or destroying the scope.

I can envision a ScopedServices.Registered callback similarly to Scoped and Activated.

Zhuinden commented 5 years ago

This is gonna take me more time than I originally thought, but in the meantime it's actually fairly easy on user side to create these callbacks.

abstract class RegisteredService: ScopedServices.Scoped {
    private val scopeSet = mutableSetOf<String>()

    @CallSuper
    override fun onEnterScope(scopeTag: String) {
        if(scopeSet.isEmpty()) {
            onServiceRegistered()
        }
        scopeSet.add(scopeTag)
    }

    @CallSuper
    override fun onExitScope(scopeTag: String) {
        scopeSet.remove(scopeTag)
        if(scopeSet.isEmpty()) {
            onServiceUnregistered()
        }
    }

    abstract fun onServiceRegistered()
    abstract fun onServiceUnregistered()
}

Which is much easier than what I have to do 😄


The trickery with multi-registration is that I call Bundleable callback before calling onEnterScope callback.

I wonder if that should be moved to "before onServiceRegistered when I add it.

Zhuinden commented 5 years ago

On second thought, I think we actually currently have a subtle bug in our app directly caused by relying on onEnterScope in a similar manner.

I should definitely add onServiceRegistered, and call Bundleable before that.

Which ends up with the question "what is the use of ScopedServices.Scoped", with that, I'm not sure.


In the meantime, I've just now migrated over to the onServiceRegistered()/onServiceUnregistered() by the exact same schematics as I said above.

    private val registeredScopes = linkedSetOf<String>()

    @CallSuper
    open fun onServiceRegistered() {
        isServiceRegisteredRelay.accept(true)
        lifecycleSubject.onNext(ServiceLifecycleEvents.REGISTERED)
    }

    @CallSuper
    open fun onServiceUnregistered() {
        isServiceRegisteredRelay.accept(false)
        lifecycleSubject.onNext(ServiceLifecycleEvents.UNREGISTERED)
    }

    @CallSuper
    @Deprecated(message = "This method is not safe, use onServiceRegistered() instead", level = DeprecationLevel.ERROR)
    override fun onEnterScope(scope: String) {
        if (registeredScopes.isEmpty()) {
            onServiceRegistered()
        }
        registeredScopes.add(scope)
    }

    @CallSuper
    override fun onScopeActive(scope: String) {
        lifecycleSubject.onNext(ServiceLifecycleEvents.SCOPE_ACTIVE)
        isScopeActiveRelay.accept(true)
    }

    @CallSuper
    override fun onScopeInactive(scope: String) {
        lifecycleSubject.onNext(ServiceLifecycleEvents.SCOPE_INACTIVE)
        isScopeActiveRelay.accept(false)
    }

    @CallSuper
    @Deprecated(message = "This method is not safe, use onServiceUnregistered() instead", level = DeprecationLevel.ERROR)
    override fun onExitScope(scope: String) {
        registeredScopes.remove(scope)
        if (registeredScopes.isEmpty()) {
            onServiceUnregistered()
        }
    }

I guess I can't thank you enough for bringing this mistake to my attention 😞

jamolkhon commented 5 years ago

Thank you for actively responding and dealing with issues.

I guess I can't thank you enough for bringing this mistake to my attention

I don't deserve this much appreciation.

Thank you for this awesome lib!

Zhuinden commented 5 years ago

Thank you for this awesome lib!

I'm glad you like it, and that it also helps you simplify navigation and service lifecycle management 🙂

I also have some good news, ScopedServices.Registered is now available in 1.14.0 (see https://github.com/Zhuinden/simple-stack/pull/156 and https://github.com/Zhuinden/simple-stack/commit/86297b86c08dd12cfeb7c67efea069b10bdac1de )- "migration path" is to just ditch .Scoped and use .Registered instead.

jamolkhon commented 5 years ago

That was quick, thank you!

Zhuinden commented 5 years ago

If you're using implicit scopes, you might want to upgrade to 2.1.2 to disallow some crazy things that apparently some users do.

(EDIT: And then update to 2.3.2 because 2.3.1 fixed a critical bug. Boy, scopes are fun.)