Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detecting if presenter is attached on a BaseRxInteractor #46

Closed
paulleo opened this issue Oct 8, 2017 · 4 comments
Closed

Detecting if presenter is attached on a BaseRxInteractor #46

paulleo opened this issue Oct 8, 2017 · 4 comments

Comments

@paulleo
Copy link

paulleo commented Oct 8, 2017

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
Copy link
Owner

mkoslacz commented Oct 9, 2017

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
Copy link
Author

paulleo commented Oct 9, 2017

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
Copy link
Owner

mkoslacz commented Oct 9, 2017

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
Copy link
Author

paulleo commented Oct 10, 2017

Appreciate your response, thank you.

@paulleo paulleo closed this as completed Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants