mkoslacz / Moviper

A Mosby based VIPER library for Android
Apache License 2.0
77 stars 9 forks source link

Detecting if presenter is attached on a BaseRxInteractor #46

Closed paulleo closed 7 years ago

paulleo commented 7 years ago

Hi, I think I can understand why a BaseRxInteractor needs no isPresenterAttached() logic as with BaseInteractor. However, in the case where one had a Presenter that had an addSubscription:

public void attachView(ViewInterface view) {
        addSubscription(
            getInteractor().getData()
                    .subscribeOn(Schedulers.io())
                    .observeOn(AndroidSchedulers.mainThread())
                    .subscribe(presenterData -> {
                        if (isViewAttached()) {
                            ViewModel vm = createViewModel(presenterData);
                            getView().updateViewModel(vm);
                        }
                    }, error -> {
                        if (isViewAttached()) {
                            ViewModel vm = createrViewModel(null);
                            getView().updateUserViewModel(vm);
                            didFailUserData(error.getLocalizedMessage());
                        }
                    })
        );
}

We have found that we required similar checks for subscriber.isDisposed() on any async work.

For example:

public Single<PresenterData>> getData() {
        return Single.create(subscriber -> {
            mService.getAPIData()
                    .subscribeOn(mJobScheduler)
                    // throttling logic and exponential retry logic goes here
                    .subscribe( 
                            response-> {
                                // business logic here to parse data, filter, groom, etc 
                                // PresenterData presenterData = doWork(response);
                                if (!subscriber.isDisposed()) { //check if presenter is still attached
                                    subscriber.onSuccess(presenterData);
                                }
                            }, throwable -> {
                                if (!subscriber.isDisposed()) {
                                    subscriber.onError(throwable);
                                }
                            }
                    );
        });
}

In light of this do you think the BaseRxInteractor should have some kind of isPresenterAttached() logic?

mkoslacz commented 7 years ago

I'm sorry but I can't get your usecase. First of all, you don't need to call isDisposed() on any async work, it needs to be called only inside one of the create(...) methods (as you can see here). In addition, an Interactor just provides the streams for Presenter as the tools, it doesn't need to be aware of the entity that uses it or its lifecycle. That's Presenter work to handle such cases, and that's why it provides an addSubscription(...) method.

I understand that the code you attached is just an example, but to avoid any potential problems in your production code - what do you want to achieve through wrapping a getAPIData() call in Single.create(...)? If I understand it correctly, it's redundant here.

If this comment doesn't answer your question, provide me more context and more specific usecase please.

paulleo commented 7 years ago

Sorry the initial question was a little rushed. I've cleanup up the code to give a better example. You were right that the getAPIData() was redundant when not grooming the data for the Presenter. I just wanted to mention that in the case where the view and presenter is detached (and therefore the subscriber is disposed because of the addSubscription) while the interactor was doing async work to prepare the presenter data, it would crash without a check for disposed. So in this scenario, I was wondering whether it would be prudent to have a isPresenterAttached() function anyway?

mkoslacz commented 7 years ago

I'm still not sure if I get you correctly, but the Interactor won't crash after the Presenter disposal. The Interactor in the Rx flavor of this lib shall not have reference to the Presenter, so Presenter actions shall not influence it.

Moreover, if the Observer gets disposed via the Disposable.dispose() method the Observer won’t get any emission from the Observable and that's all. Observable can still emit its items and nothing dangerous will happen. As I have mentioned above, the only scenario where you should call isDisposed is inside one of the create(...) methods, but it's up to internal implementation of your components, and it does not depend on the Presenter-Interactor link and lifecycle.

paulleo commented 7 years ago

Appreciate your response, thank you.