search

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

[MVI] intent(MviView::intent) does't fire before view is attached #303

Closed diogojg closed 6 years ago

diogojg commented 6 years ago

My question is somehow similar to this #242 So I have an intent in my view implemented like this:

override fun selectDateIntent(): Observable<Day> {
        return selectDatePubSub.distinctUntilChanged()
                .startWith(selectedDate)
                .doOnNext {
                        //this fires one time correctly because off startWith()
                }
    }

In the presenter I have this observable wrapped in the intent() function like this:

timetableInteractor.setSelectDateIntent(intent(TimetableView::selectDateIntent)} )

After debugging I noticed that the observable resulting from the wrapped intent does not emit nothing before the view is attached. So, if I add a delay to the intent implementation in the view, everything goes as expected. However, in the MVI sample code having Observable.just(object) is a recurring practice for the intent implementations, so I am curious to know what I'm doing incorrectly.

Thanks, Diogo

kusraevs commented 6 years ago

Binding intents actually starts in attachView method of MviBasePresenter. In MviActivity this method calls in onStart. So if you use PublishSubject item could be emitted before this time because it is a hot observable. If you use Observable.just(..) we don't have this problem. In this situation i use BehaviorSubject instead of PublishSubject.

But if you call startWith inside intent() function it should work ok. So this is issue is the same as #242 as far as i concern

sockeqwe commented 6 years ago

@kusraevs is right, intent() actually subscribes in presenter.attachView() which actually is called in onStart(). The idea is to not have hot observables in view (hot observable intetns). So the idea is that the presenter subscribes after both, view and presenter, are ready which is only guaranteed after onStart().

So basically you have to wrap your head around a little bit but rather than start loading data in onCreate() provide give your view a view.loadDataIntent() that is a cold observable (observable that only emits once a subscriber is subscribed (presenter). Lifecycle should be like this:

So if you emit intents outside of onStart() and onStop() the presenter will miss it.

With that said, I think that BehaviorSubjects are only the last resort to use in the view layer for delaying emit intents. If you follow the onStart() onStop() lifecycle you should not need workarounds like BehaviorSubjects.

jhowens89 commented 6 years ago

@sockeqwe Did you ever come to a conclusion about #242? I was still very new to RxJava back then and I'd like to think the code I write down is less problematic, but I can still replicate this issue. 

class MatchPlayRoundSelectorPresenter @Inject constructor(val interactor: MatchPlayRoundSelectorInteractor) : MviBasePresenter<MatchPlayRoundSelectorView, MatchPlayRoundSelectorViewState>() {
    override fun bindIntents() {
        val loadMatchPlayRoundSelectorDataIntent = intent(MatchPlayRoundSelectorView::connectDataIntent)
                .doOnNext { Timber.e("TESTING_BEFORE 1") }
                .switchMap { interactor.loadMatchPlayRoundSelectorData() }
                .doOnNext { Timber.e("TESTING_BEFORE 2") }
                .subscribeOn(Schedulers.io())
                .doOnError { Timber.e(it,"Fatal error occurred") }

        val selectNewRoundIntent = intent(MatchPlayRoundSelectorView::selectNewRoundIntent)
                .map { (round, page) -> interactor.newRoundSelected(round, page) }

        val initialState = interactor.getInitialState()

        val allIntents = Observable.merge(loadMatchPlayRoundSelectorDataIntent, selectNewRoundIntent)
                .observeOn(AndroidSchedulers.mainThread())

        subscribeViewState(allIntents.scan(initialState, ::viewStateReducer)
                .distinctUntilChanged(), MatchPlayRoundSelectorView::render)}
}

The other side of the equation looks like:

override fun connectDataIntent(): Observable<Unit> = Observable.just(Unit)//.doOnNext { Timber.e("TESTING_BEFORE 0") }

If I run this code, I'll get the following relevant logs: 02-22 17:46:12.605 7068-7068/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE render com.tour.pgatour.match_play.leaderboard.match_play_round_selector.MatchPlayRoundSelectorViewState$WithoutData@7830fd3

If all I do is uncomment that doOnNext: 

02-22 17:44:57.205 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE render com.tour.pgatour.match_play.leaderboard.match_play_round_selector.MatchPlayRoundSelectorViewState$WithoutData@fb867e3
02-22 17:44:57.206 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE 0
02-22 17:44:57.206 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorP: TESTING_BEFORE 1
02-22 17:44:57.368 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorP: TESTING_BEFORE 2
02-22 17:44:58.099 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE render WithData(viewData=MatchPlayRoundSelectorViewData(roundPlayTabs=[RoundTab(config=TabConfig(text=Round 1, backgroundColor=-16777216, textColor=-1, selectedBackgroundColor=-1, selectedTextColor=-16777216), option=com.tour.pgatour.shared_rel....

It's like I'm so close to the boundary of the race condition that even a log will push it over. Thoughts?

sockeqwe commented 6 years ago

I will work on that next week

jhowens89 notifications@github.com schrieb am Fr., 23. Feb. 2018, 01:51:

@sockeqwe https://github.com/sockeqwe Did you ever come to a conclusion about #242 https://github.com/sockeqwe/mosby/issues/242? I was still very new to RxJava back then and I'd like to think the code I write down is less problematic, but I can still replicate this issue.

class MatchPlayRoundSelectorPresenter @Inject constructor(val interactor: MatchPlayRoundSelectorInteractor) : MviBasePresenter<MatchPlayRoundSelectorView, MatchPlayRoundSelectorViewState>() { override fun bindIntents() { val loadMatchPlayRoundSelectorDataIntent = intent(MatchPlayRoundSelectorView::connectDataIntent) .doOnNext { Timber.e("TESTING_BEFORE 1") } .switchMap { interactor.loadMatchPlayRoundSelectorData() } .doOnNext { Timber.e("TESTING_BEFORE 2") } .subscribeOn(Schedulers.io()) .doOnError { Timber.e(it,"Fatal error occurred") }

    val selectNewRoundIntent = intent(MatchPlayRoundSelectorView::selectNewRoundIntent)
            .map { (round, page) -> interactor.newRoundSelected(round, page) }

    val initialState = interactor.getInitialState()

    val allIntents = Observable.merge(loadMatchPlayRoundSelectorDataIntent, selectNewRoundIntent)
            .observeOn(AndroidSchedulers.mainThread())

    subscribeViewState(allIntents.scan(initialState, ::viewStateReducer)
            .distinctUntilChanged(), MatchPlayRoundSelectorView::render)}

}

The other side of the equation looks like:

override fun connectDataIntent(): Observable = Observable.just(Unit)//.doOnNext { Timber.e("TESTING_BEFORE 0") }

If I run this code, I'll get the following relevant logs: 02-22 17:46:12.605 7068-7068/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE render com.tour.pgatour.match_play.leaderboard.match_play_round_selector.MatchPlayRoundSelectorViewState$WithoutData@7830fd3

If all I do is uncomment that doOnNext:

02-22 17:44:57.205 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE render com.tour.pgatour.match_play.leaderboard.match_play_round_selector.MatchPlayRoundSelectorViewState$WithoutData@fb867e3 02-22 17:44:57.206 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE 0 02-22 17:44:57.206 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorP: TESTING_BEFORE 1 02-22 17:44:57.368 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorP: TESTING_BEFORE 2 02-22 17:44:58.099 6216-6216/com.tour.pgatour E/MatchPlayRoundSelectorL: TESTING_BEFORE render WithData(viewData=MatchPlayRoundSelectorViewData(roundPlayTabs=[RoundTab(config=TabConfig(text=Round 1, backgroundColor=-16777216, textColor=-1, selectedBackgroundColor=-1, selectedTextColor=-16777216), option=com.tour.pgatour.shared_rel....

It's like I'm so close to the boundary of the race condition that even a log will push it over. Thoughts?

— You are receiving this because you were mentioned.

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

sockeqwe commented 6 years ago

@jhowens89 Do you use a Fragment or Activity. I'm trying to reproduce this ...

sockeqwe commented 6 years ago

Fixed. If not, please comment on #242

shmakova commented 6 years ago

Hello, I have a problem.

I want to use PublishSubject in onActivityResult, but it is called before onStart, so intent doesn't fire. Is there any way to fire it?

I have tried to use BehaviorSubject but it repeats last intent after I return back to the screen

sockeqwe commented 6 years ago

UnicastSubject should do the trick (instead of PublishSubject or BehaviorSubjct).

http://reactivex.io/RxJava/javadoc/io/reactivex/subjects/UnicastSubject.html

UnicastSubject internally stores all emitted data until there is one subscriber. Once there is a subscriber it acts like PublishSubject.

kusraevs commented 5 years ago

UnicastSubject will not act like PublishSubject because if another subscriber will subscribe to UnicastSubject, it will receive an IllegalStateException, even if previous subscriber was disposed. That's why we cannot use it in this situation.