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

Callback method called multiply times #80

Closed
lectricas opened this issue Jan 23, 2017 · 13 comments
Closed

Callback method called multiply times #80

lectricas opened this issue Jan 23, 2017 · 13 comments

Comments

@lectricas
Copy link

public void loadTeasers(int skip, boolean refresh, Category category) {
        getViewState().showProgress();

        Subscription s2 = ActiveAndroidHelper.getSources()
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(teasers -> {
                    getViewState().onTeasersLoaded(teasers);
                    getViewState().hideProgress();
                }, this::onError);
        unsubscribeOnDestroy(s2);
    }

Suppose we have simple method which loads some data from network. I want to refresh my network data every onResume method.
And, in this case, on each call of loadTeasers(true, new Category()), the callback method onTeasersLoaded(teasers) is called N times, where N is the number of onResume methods (and loadTeasers) executed.
I think I need to unsubscribe and null the previous subsribition, but which method to use here?
Thanks

@senneco
Copy link
Collaborator

senneco commented Jan 23, 2017

Well you can use this method to do it =) Just like this:

Subscription loadTeasersSubscription;

public void loadTeasers(int skip, boolean refresh, Category category) {
        if (loadTeasersSubscription != null && !loadTeasersSubscription.isUnsubscribed()) {
                loadTeasersSubscription.unsubscribe();
        }

        getViewState().showProgress();

        loadTeasersSubscription = ActiveAndroidHelper.getSources()
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(teasers -> {
                    getViewState().onTeasersLoaded(teasers);
                    getViewState().hideProgress();
                }, this::onError);

        unsubscribeOnDestroy(loadTeasersSubscription);
    }

@lectricas
Copy link
Author

@senneco Nope, this does not work. That was only a guess about necessity to unsubsribe.
loadTeasersSubscription is not null, but unsubsribed.
I think the problem is somewhere else.
Haven't you encountered this issue already??

@senneco
Copy link
Collaborator

senneco commented Jan 23, 2017

It seems that I didn't understand what is your problem. Problem is that this method call each time after onResume, or problem that it's not? Or something else?

@A-Zaiats
Copy link
Contributor

A-Zaiats commented Jan 23, 2017

What strategy do you use for onTeaserLoaded(teasers) method? Was your view stopped before resume? Are you sure that onTeaserLoaded(teasers) method is called N times from subsrciber#onNext callback?

Looks like you use default strategy. So it can be following behavior:

  • the View was stopped
  • the View was detached from presenter
  • the View was attached to the Presenter
  • the ViewState restored state (called all onTeaserLoaded(teasers) that was called before)
  • one more onTeaserLoaded(teasers) was called by loadTeasers callback

@senneco can you correct me if I am wrong with Moxy lifecycle?

@lectricas
Copy link
Author

I just call loadTeasers() each time my fragment executes OnResume method.
And I get onTeaserLoaded(teasers) executed N times (over and over, after only single OnResume call).
And this N grows every time Fragment executed OnResume method.
Suppose you have two fragments and you just moving to and fro between them. The first fragment has a presenter, where loadTeasers() method is.
So the strategy is the default one, just copied the code from the sample.

@senneco
Copy link
Collaborator

senneco commented Jan 23, 2017

@lectricas it seems like bug is in your ActiveAndroidHelper.getSources(), or onTeasersLoaded(teasers) has default or AddToEndStrategy. Try to annotate your onTeasersLoaded(teasers) method with annotation @StateStrategy(AddToEndSingleStrategy.class).

@lectricas
Copy link
Author

@senneco
LogCat.

D/MyInteger: startLoadingInFragment 
D/MyInteger: subscribeInPresenter 
D/MyInteger: CallBackInFragment:1
D/MyInteger: startLoadingInFragment
D/MyInteger: CallBackInFragment:1
D/MyInteger: subscribeInPresenter
 D/MyInteger: CallBackInFragment:1
D/MyInteger: startLoadingInFragment
 D/MyInteger: CallBackInFragment:1
D/MyInteger: CallBackInFragment:1
 D/MyInteger: subscribeInPresenter
 D/MyInteger: CallBackInFragment:1
D/MyInteger: startLoadingInFragment
D/MyInteger: CallBackInFragment:1
 D/MyInteger: CallBackInFragment:1
 D/MyInteger: CallBackInFragment:1
 D/MyInteger: subscribeInPresenter
D/MyInteger: CallBackInFragment:1
public void testInteger() {
        getViewState().showProgress();
        s3 = Observable.just(1)
        .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(integer -> {
                    Log.d("MyInteger", "subscribeInPresenter");
                    getViewState().integerTest(integer);
                    getViewState().hideProgress();
                }, this::onError);
        unsubscribeOnDestroy(s3);
    }

Seems like callback method is called by its own. Presenter calls callback method correctly - single time on each subscribe.

@senneco
Copy link
Collaborator

senneco commented Jan 23, 2017

@lectricas could you provide me your MvpView interface?

@lectricas
Copy link
Author

@senneco

public interface BaseNewsViewState extends BaseViewState {
    void onTeasersLoaded(List<Teaser> teasers);
    void onTeasersQueried(List<Teaser> teasers);
    void onCurrencyLoaded(CurrencyRequestAnswer currencyAnswer);
    void integerTest(Integer integer);
}
public interface BaseViewState extends MvpView {
    void hideProgress();
    void showProgress();
    void onError(Throwable throwable);
}

@senneco
Copy link
Collaborator

senneco commented Jan 23, 2017

@lectricas try to apply AddToEndStrategy, like this:

@StateStrategy(AddToEndSingleStrategy.class)
public interface BaseNewsViewState extends BaseViewState {
    void onTeasersLoaded(List<Teaser> teasers);
    void onTeasersQueried(List<Teaser> teasers);
    void onCurrencyLoaded(CurrencyRequestAnswer currencyAnswer);
    void integerTest(Integer integer);
}

@lectricas
Copy link
Author

@senneco

D/MyInteger: startLoadingInFragment
D/MyInteger: CallBackInFragment:1
D/MyInteger: subscribeInPresenter
D/MyInteger: CallBackInFragment:1
D/MyInteger: startLoadingInFragment
D/MyInteger: CallBackInFragment:1
D/MyInteger: subscribeInPresenter
D/MyInteger: CallBackInFragment:1
D/MyInteger: startLoadingInFragment
D/MyInteger: CallBackInFragment:1
D/MyInteger: subscribeInPresenter
D/MyInteger: CallBackInFragment:1

Better now, but first callback is kinda redundant.

@A-Zaiats
Copy link
Contributor

I think SkipStrategy.class will be better.
If View is valid, callback will be called. If no, presenter reload data on resume.

@lectricas
Copy link
Author

@A-Zaiats
woohoo
works as desired. Thanks.

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

No branches or pull requests

3 participants