badoo / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel), inspired by Badoo MVICore library
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
826 stars 66 forks source link

Lost labels on init fragment view issue #165

Closed t1r closed 3 years ago

t1r commented 3 years ago

Hi there. Idk, it may be not a bug, but...

When we call store.accept(PermissionsStore.Intent.MapReady) like in a real use case with google map, when we wait until google map will be ready and OnMapReadyCallback are called. Handle this intent in store, and try to send a label, which calls a fragment method with showing dialog, for example. But we lost the sent label.

Because in the DefaultStore method fun labels() calls after accept(). It looks like a lifecycle and binding issue.

You can reproduce this just run my project and see the log with the DEBDEB tag. We won't see strings: "DEBDEB consume AskLocationPermissions label" "DEBDEB askLocationPermissions"

PermissionsFragment

PermissionsController

PermissionsStoreFactory

Whole project

class PermissionsFragment : Fragment() {
...
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        controller.onViewCreated(
            view = PermissionsViewImpl(view),
            lifecycle = lifecycle.asMviLifecycle(),
            viewLifecycle = viewLifecycleOwner.lifecycle.asMviLifecycle()
        )
        initUi()
    }

    private fun initUi() {
        //mapView?.getMapAsync(getOnMapReadyCallback())
        onOnMapReady()
    }

    //Call method, whick send intent
    private fun onOnMapReady() {
        //super.setupGoogleMap(googleMap)
        controller.onOnMapReady()
        Timber.d("DEBDEB onOnMapReady")
    }
...
}
class PermissionsController constructor(
    private val store: PermissionsStore,
    private val fragment: PermissionsFragment
) {
...
    fun onViewCreated(
        view: MviView<PermissionsView.ViewModel, PermissionsView.ViewEvent>,
        lifecycle: Lifecycle,
        viewLifecycle: Lifecycle
    ) {
        bind(viewLifecycle, BinderLifecycleMode.START_STOP) {
            store.states.mapNotNull(stateToViewModel) bindTo view
        }

        bind(viewLifecycle, BinderLifecycleMode.CREATE_DESTROY) {
            view.events.mapNotNull(viewEventToIntent) bindTo store
            store.labels.mapNotNull(labelToEvent) bindTo { consumeEvent(it) }
        }

        lifecycle.doOnDestroy(store::dispose)
    }

    fun onOnMapReady() {
        store.accept(PermissionsStore.Intent.MapReady)
    }

    private fun consumeEvent(event: PermissionsEvent) {
        when (event) {
            is PermissionsEvent.Back -> {
                //TOOD back
            }
            is PermissionsEvent.AskLocationPermissions -> {
                Timber.d("DEBDEB PermissionsEvent.AskLocationPermissions")
                fragment.askLocationPermissions()
            }
        }
    }
...
}
internal class DefaultStore<in Intent : Any, in Action : Any, in Result : Any, out State : Any, Label : Any> @MainThread constructor(
    initialState: State,
    private val bootstrapper: Bootstrapper<Action>?,
    executorFactory: () -> Executor<Intent, Action, State, Result, Label>,
    private val reducer: Reducer<State, Result>
) : Store<Intent, State, Label> {
...
    //Call call after
    override fun labels(observer: Observer<Label>): Disposable {
        assertOnMainThread()

        return labelSubject.subscribe(observer)
    }

    //Call first
    override fun accept(intent: Intent) {
        assertOnMainThread()

        intentSubject.onNext(intent)
    }
...
}
arkivanov commented 3 years ago

Hello! Thanks for the report. You bind Store.labels to consumeEvent using view lifecycle BinderLifecycleMode.CREATE_DESTROY in onViewCreated method. And you are sending Intent.MapReady from the same method - onViewCreated. Unfortunately, at this point view's Lifecycle is in INITIALIZED state, which means the binding is not yet happened. In your particular case you can bind Store.labels in the controller's init section using its own lifecycle and BinderLifecycleMode.CREATE_DESTROY. Please check the similar sample code.

t1r commented 3 years ago

Thank You for advice. But it's work only if we add something like this: post.{callSetupGmapMethod()} e. g. https://github.com/t1r/MVIKotlin-Resolve/commit/118e850c29480228be97ead5b7ae10de97ebb169

I don't sure about the reliability code with post. But it's kind of work and I use post with MVICore, ok.

And extra question, if we move labels binding

bind(viewLifecycle, BinderLifecycleMode.CREATE_DESTROY) { //viewLifecycle to lifecycle
    store.labels.mapNotNull(labelToEvent) bindTo { consumeEvent(it) }
}

to the controller's constructor, we need to replace viewLifecycle by lifecycle, it doesn't lead to unexpected behavior or smth? Or maybe the right way to always use lifecycle (not viewLifecycle) for binding labels?

arkivanov commented 3 years ago

It depends on what you are binding. E.g. Labels are available once the Store is created, so they can be bound from this point. But it also depends on the receiver's availability. So, I would say, every case is special.

Regarding your particular case, you can also do the following:

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)

        // ...

        viewLifecycleOwner.lifecycle.asMviLifecycle().doOnCreateDestroy(
            onCreate = ::initUi,
            onDestroy = {}
        )
    }