sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

RxBottomNavigationView.itemSelection stop working after activity recreated #202

Closed tagantroy closed 7 years ago

tagantroy commented 7 years ago

I use rxbinding 2.0.0-snapshot and found 1 strange bug with RxBottomNavigationView and MVI.

class MainPresenter : MviBasePresenter<MainView, MainViewState>() { override fun bindIntents() { val menu = intent(MainView::selectMenuItem) .map { when(it.itemId){ R.id.search -> MainViewState.Search R.id.favorite -> MainViewState.Favorite R.id.profile -> MainViewState.Profile else -> MainViewState.Messages } } .startWith { MainViewState.Search } subscribeViewState(menu,MainView::render) } }

interface MainView : MvpView{ fun selectMenuItem() : Observable<MenuItem> fun render(state: MainViewState) }

` ... override fun createPresenter(): MainPresenter { return MainPresenter() }

override fun selectMenuItem(): Observable { return RxBottomNavigationView.itemSelections(bnvMain) }

override fun render(state: MainViewState) { ... } ... ` bindIntents() executed only once and selectMenuItem() executed each time, when activity recreated

sockeqwe commented 7 years ago

bindIntents() executed only once and selectMenuItem() executed each time, when activity recreated

Sounds like the expected behavior, what exactly is the unexpected/ buggy behavior?

Ivan Balaksha notifications@github.com schrieb am Mi., 1. Feb. 2017, 21:50:

I use rxbinding 2.0.0-snapshot and found 1 strange bug with RxBottomNavigationView and MVI.

class MainPresenter : MviBasePresenter<MainView, MainViewState>() { override fun bindIntents() { val menu = intent(MainView::selectMenuItem) .map { when(it.itemId){ R.id.search -> MainViewState.Search R.id.favorite -> MainViewState.Favorite R.id.profile -> MainViewState.Profile else -> MainViewState.Messages } } .startWith { MainViewState.Search } subscribeViewState(menu,MainView::render) } }

interface MainView : MvpView{ fun selectMenuItem() : Observable fun render(state: MainViewState) }

` ... override fun createPresenter(): MainPresenter { return MainPresenter() }

override fun selectMenuItem(): Observable { return RxBottomNavigationView.itemSelections(bnvMain) }

override fun render(state: MainViewState) { ... } ... ` bindIntents() executed only once and selectMenuItem() executed each time, when activity recreated

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/202, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrhvm1iBgYJ5odDKc87PZNujz1DuAks5rYPAZgaJpZM4L0Xpy .

tagantroy commented 7 years ago

yes, it's sound like expected behavior,but your observable for bottom menu will not work anymore. How can I fix it? I'm use it incorrectly?

sockeqwe commented 7 years ago

So selectItemMenu() doesn't work anymore after a screen orientation changes, right? What is MainView extending from? Activity or Fragment, FrameLayout etc?

tagantroy commented 7 years ago

class MainActivity : MviActivity<MainView, MainPresenter>(), MainView

sockeqwe commented 7 years ago

I cant see anything wrong. Does RxBottomNavigationView observalbe ever emit a onComplete() event. Could you just add a log ?

override fun selectMenuItem(): Observable { return RxBottomNavigationView.itemSelections(bnvMain).doOnComplete { Log.d("Foo", "completed")} }

Ivan Balaksha notifications@github.com schrieb am Mi., 1. Feb. 2017, 22:19:

class MainActivity : MviActivity<MainView, MainPresenter>(), MainView

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/202#issuecomment-276785590, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnruC2zr3gTYni75mpjJCXmsozoJgLks5rYPbxgaJpZM4L0Xpy .

tagantroy commented 7 years ago

added override fun selectMenuItem(): Observable<MenuItem> { return RxBottomNavigationView.itemSelections(bnvMain) .doOnNext { Log.e("SelectMenuItem","title = ${it.title}") } .doOnComplete { Log.e("SelectMenuItem","onComplete}") } }

and intent(MainView::selectMenuItem) .doOnNext { Log.e("intent", "title = ${it.title}") }...

I see logs "SelectMenuItem: title = ...." but nothing with "intent"

sockeqwe commented 7 years ago

Is there a way I could take a look at your concrete code to debug it? Is your code public available on github?

tagantroy commented 7 years ago

i'll provide link tomorrow

tagantroy commented 7 years ago

link: https://github.com/tagantroy/bottomnavigationmvibug

sockeqwe commented 7 years ago

Thanks. Indeed, there must be an internal bug of MviBasePresenter. I havent found the issue yet, but it is really strange that this works in the mvi sampe app (from this repository) and not in your demo app.

However, it is an internal bug for sure, I'm working on it.

tagantroy commented 7 years ago

Thank you. I hope mvi future of android development(I'm use it instead mvvm)

sockeqwe commented 7 years ago

Ok, I found the bug. Actually it is not Mosby related.

The issue is the startWith() operator which blocks because it never emits an item to start with. Don't ask me how startWith(ObservableSource) is used properly.

Usually you should prefer startWith(concreteValue):

class MainPresenter : MviBasePresenter<MainView, MainViewState>() {
    override fun bindIntents() {
        val menu = intent(MainView::selectMenuItem)
                .map {
                    when(it.itemId){
                        R.id.search -> MainViewState.Search
                        R.id.favorite -> MainViewState.Favorite
                        R.id.profile -> MainViewState.Profile
                        else -> MainViewState.Messages
                    }
                }
                .startWith(MainViewState.Search) // This is a concrete value to start with
        subscribeViewState(menu,MainView::render)
    }
}

Last but not least I just wanted to warn you that navigation and how you are aproaching Navigation may lead to some (state) issues because FragmentManager and BottomNavigationView have their own internal state (which they will restore by their own).

i.e. you have a method like this which you call from your view's render() method:

fun changeFragment(fragment: Fragment) {
        val transaction = supportFragmentManager.beginTransaction()
        transaction.replace(R.id.fragment_container, fragment)
        transaction.commit()
    }

So far that's fine, but after a screen orientation change (rotate your device) FragmentManager will automatically restore the previous Fragment AND MviBasePresenter will call render() again with the latest value before the screen orientation change which then will call changeFragment() again and replace the previously restored Fragment (restored by FragmentManager) with a new instantiated Fragment. Keep that in mind.