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

Async set fragment when app invisible #257

Closed DrobyshevAlex closed 7 years ago

DrobyshevAlex commented 7 years ago

When I receive a response from the server, I create a new fragment. If the application is minimized I get an error

io.reactivex.exceptions.OnErrorNotImplementedException: Can not perform this action after onSaveInstanceState

    public void render(LogoViewState logoViewState) {
        if (logoViewState instanceof LogoViewState.LoadingState) {
            statusView.setText("Connecting...");
        } else if (logoViewState instanceof LogoViewState.DataState) {
            statusView.setText(logoViewState.toString());
            if (getActivity() != null) {
                ((MainActivity)getActivity()).showHomeFragment();
            }
        }
    }
sockeqwe commented 7 years ago

Minimized with Don't keep Activities options? How do you compose your intents? Do you use subscribeViewState() or manage subscription manually?

DrobyshevAlex notifications@github.com schrieb am Do., 29. Juni 2017, 00:12:

When I receive a response from the server, I create a new fragment. If the application is minimized I get an error

io.reactivex.exceptions.OnErrorNotImplementedException: Can not perform this action after onSaveInstanceState

public void render(LogoViewState logoViewState) {
    if (logoViewState instanceof LogoViewState.LoadingState) {
        statusView.setText("Connecting...");
    } else if (logoViewState instanceof LogoViewState.DataState) {
        statusView.setText(logoViewState.toString());
        if (getActivity() != null) {
            ((MainActivity)getActivity()).showHomeFragment();
        }
    }
}

— 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/257, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrkyU_912mdjB4BjrrpIVdpajFL4sks5sIs_ggaJpZM4OImad .

DrobyshevAlex commented 7 years ago

1) shutdown server 2) run app 3) press home button or launch other app 4) run server

public class LogoPresenter extends MviBasePresenter<LogoView, LogoViewState> {
    @Inject
    WebSocket webSocket;

    @Override
    protected void bindIntents() {
        Observable<LogoViewState> logoViewStateObservable =
            intent(LogoView::serverVersion)
                    .flatMap(version -> {
                        Observable<Version> v = webSocket.on("version", Version.class);
                        return v;
                    }).map(LogoViewState.DataState::new)
                    .cast(LogoViewState.class)
                    .observeOn(AndroidSchedulers.mainThread());

        subscribeViewState(logoViewStateObservable, LogoView::render);

        webSocket.emit("version", new Version(2));
    }

}

error on line ((MainActivity)getActivity()).showHomeFragment();

because app in pause

sockeqwe commented 7 years ago

Im not familiar with Websocket api. Could you please add some logs at:

Thanks

DrobyshevAlex notifications@github.com schrieb am Do., 29. Juni 2017, 01:06:

public class LogoPresenter extends MviBasePresenter<LogoView, LogoViewState> { @Inject WebSocket webSocket;

@Override
protected void bindIntents() {
    Observable<LogoViewState> logoViewStateObservable =
        intent(LogoView::serverVersion)
                .flatMap(version -> {
                    Observable<Version> v = webSocket.on("version", Version.class);
                    return v;
                }).map(LogoViewState.DataState::new)
                .cast(LogoViewState.class)
                .observeOn(AndroidSchedulers.mainThread());

    subscribeViewState(logoViewStateObservable, LogoView::render);

    webSocket.emit("version", new Version(2));
}

}

— You are receiving this because you commented.

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

DrobyshevAlex commented 7 years ago

Android not allowed use FragmentManager.replace if app on pause.

If app try set new fragment without action user (callback websocket, http, or just other thread after some time) when app background (incoming call, run other app, etc...) i got crash app.

DrobyshevAlex commented 7 years ago

I removed websocket and error gone... I'll try tomorrow more tests

sockeqwe commented 7 years ago

You can also try it with switchMap () instead of flatMap (). Not sure if that matters though.

DrobyshevAlex notifications@github.com schrieb am Do., 29. Juni 2017, 02:08:

I removed websocket and error gone... I'll try tomorrow more tests

— You are receiving this because you commented.

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

DrobyshevAlex commented 7 years ago

I don't understand how i can use websocket.

If i want to show actual info from server without reuest.

It is like a

// subscribe to socket event
Observable<Object> model = ws.on('user_status');

How i can render it with MVI?

In MVP it's easy

        disposable.add(model
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe( s -> {
                    if (isViewAttached()) {
                        getView().showUpdateStatus(s);
                    }
                }, throwable -> {
                    throwable.printStackTrace();
                })
        );
sockeqwe commented 7 years ago

Wait, lets step back a moment. Your original question was that you run in the IllegalStateException. This seems to be caused by the fact that the rx obsrvale stream is still alive but shouldn't. This could actually be a bug of Mosby. Thats why I have asked you if you can reproduce the error with some logs.

DrobyshevAlex notifications@github.com schrieb am Do., 29. Juni 2017, 13:10:

I don't understand how i can use websocket.

If i want to show actual info from server without reuest.

It is like a

// subscribe to socket event Observable model = ws.on('user_status');

How i can render it with MVI?

In MVP it's easy

    disposable.add(model
            .observeOn(AndroidSchedulers.mainThread())
            .subscribe( s -> {
                if (isViewAttached()) {
                    getView().showUpdateStatus(s);
                }
            }, throwable -> {
                throwable.printStackTrace();
            })
    );

— You are receiving this because you commented.

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

DrobyshevAlex commented 7 years ago
    @Override
    protected void bindIntents() {

        Observable<LogoViewState> o = webSocket.on("version", Version.class)
                .doOnNext( i -> Log.d("Presenter", "doOnNext") )
                .observeOn(AndroidSchedulers.mainThread())
                .cast(Version.class)
                .map(LogoViewState.DataState::new);

        o.doOnDispose( () -> {
            Log.d("Presenter", "doOnDispose");
        } );

        subscribeViewState(o, LogoView::render);

        webSocket.emit("version", new Version(1));
    }
    @Override
    public void render(LogoViewState logoViewState) {
        if (logoViewState instanceof LogoViewState.LoadingState) {
            statusView.setText("Connecting...");
        } else if (logoViewState instanceof LogoViewState.DataState) {
            Log.d("fragment", "1");
            statusView.setText(logoViewState.toString());
            if (getActivity() != null) {
                Log.d("fragment", "2");
                ((MainActivity)getActivity()).showHomeFragment();
            }
        }
    }

D/Activity: onCreate D/Activity: onResume D/Activity: onPause D/Activity: onSaveInstanceState D/Fragment: onStop D/Presenter: doOnNext D/fragment: 1 D/fragment: 2 W/System.err: io.reactivex.exceptions.OnErrorNotImplementedException: Can not perform this action after onSaveInstanceState E/AndroidRuntime: FATAL EXCEPTION: main Process: ru.terraideas.smart, PID: 8574 io.reactivex.exceptions.OnErrorNotImplementedException: Can not perform this action after onSaveInstanceState

DrobyshevAlex commented 7 years ago

I think webSocket need put to intent. But i can do it only from MvpView.

Updated: I put webSocket.on in view but app is crashed

    @Override
    public void detachView(boolean retainInstance) {
        super.detachView(retainInstance);
        Log.d("Presenter", "detachView(" + (retainInstance ? "true" : "false") + ")");
    }

detachView in presenter havn't executed

sockeqwe commented 7 years ago

Could you please add a log to presenter.detachview(retain)

DrobyshevAlex notifications@github.com schrieb am Do., 29. Juni 2017, 14:06:

I think webSocket need put to intent. But i can do it only from MvpView.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/257#issuecomment-311946930, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrtEEzX-td9Z0J_k0e-woSXErGNw2ks5sI5MzgaJpZM4OImad .

DrobyshevAlex commented 7 years ago

The code is in the previous message.

public class LogoPresenter extends MviBasePresenter<LogoView, LogoViewState> {
    @Inject
    WebSocket webSocket;

    @Override
    protected void bindIntents() {

        Observable<LogoViewState> o = intent(LogoView::subscribe)
                .doOnNext( i -> Log.d("Presenter", "doOnNext") )
                .observeOn(AndroidSchedulers.mainThread())
                .map(LogoViewState.DataState::new).doOnDispose( () -> {
                    Log.d("Presenter", "doOnDispose");
                } ).cast(LogoViewState.class);

        subscribeViewState(o, LogoView::render);

        webSocket.emit("version", new Version(1));
    }

    @Override
    public void detachView(boolean retainInstance) {
        super.detachView(retainInstance);
        Log.d("Presenter", "detachView(" + (retainInstance ? "true" : "false") + ")");
    }
}

Log from detachView haven't printed

DrobyshevAlex commented 7 years ago

detail log

D/Activity: onPause D/FragmentMviDelegateImpl: Saving MosbyViewId into Bundle. ViewId: c7e9b66f-844d-4a3c-8376-fe36709c5bad D/ActivityMviDelegateImpl: Saving MosbyViewId into Bundle. ViewId: ced14644-d2aa-4441-848a-2d3133cb572f D/Activity: onSaveInstanceState D/Fragment: onStop D/ActivityMviDelegateImpl: detached MvpView from Presenter. MvpView view.MainActivity@343dd24b Presenter: view.MainPresenter@3dfa8a7a D/Presenter: doOnNext crash..

detached only Activity. Can I detach Fragment?

I think the problem is that the subscriber can change Activity after the application is stopped.

I would like to "ViewState" be changed after "onStop" but "render" run after the application be "onStart".

DrobyshevAlex commented 7 years ago

if i can move code from FragmentPresente to ActivityPresenter app work good!

But i need some fragments with intent code... I don't want make some activites :)

Can you help me please?

sockeqwe commented 7 years ago

Yeah, currently Mosby uses onCreateView() and onDestroyView() ... I might change that to solve this issue

DrobyshevAlex notifications@github.com schrieb am Do., 29. Juni 2017, 21:42:

if i can move code from FragmentPresente to ActivityPresenter app work good!

But i need some fragments with intent code... I don't want make some activites :)

Can you help me please?

— You are receiving this because you commented.

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

DrobyshevAlex commented 7 years ago

It is not easy :( If i attach/detach view in onStart/onStop from fragment the crash gone. But after onStart "Android" run FragmentManager transaction and i have new crash

java.lang.IllegalStateException: FragmentManager is already executing transactions

DrobyshevAlex commented 7 years ago

Another error. unsubscribe don't work when frament destroy.

    @Override
    public void onDestroyView() {
        super.onDestroyView();
        unbinder.unbind();
        presenter.detachView(false);
    }
  • so work fine
sockeqwe commented 7 years ago

I'm sorry but I don't understand your latest comment?

DrobyshevAlex commented 7 years ago

I have a fregment with presenter. If i press back button for close app then fragment has destroed. But subscribers from presenter don't dispose.

When i run application after destroy the fragment i watch in log 2 line that print from doOnNext.

D/App: test D/App: test

If a press back button again then nex run application print 3 line to log.

D/App: test D/App: test D/App: test

etc...

I create BaseFragment

public abstract class BaseFragment<V extends MvpView, P extends MviPresenter> extends MviFragment {
    protected P presenter;
    protected Unbinder unbinder = null;
    private boolean isAttached = true;

    @Override
    public void onStart() {
        super.onStart();
        if (presenter != null && !isAttached) {
            presenter.attachView(this);
        }
    }

    @Override
    public void onStop() {
        super.onStop();
        if (presenter != null && isAttached) {
            presenter.detachView(true);
        }
    }

    @Override
    public void onDestroy() {
        super.onDestroy();
        if (presenter != null) {
            presenter.detachView(false);
        }
        if (unbinder != null) {
            unbinder.unbind();
        }
    }
}

Now everything works as it should

presenter.detachView(false); - fix it

sockeqwe commented 7 years ago

Yes, i have already said in a previous comment, that lifecycle should be between onStart() and onStop() and that I will fix it soon. Thanks for providing your feedback, logs and help!

DrobyshevAlex notifications@github.com schrieb am So., 2. Juli 2017, 21:07:

I have a fregment with presenter. If i press back button for close app then fragment has destroed. But subscribers from presenter don't dispose.

When i run application after destroy the fragment i watch in log 2 line that print from doOnNext.

D/App: test D/App: test

If a press back button again then nex run application print 3 line to log.

D/App: test D/App: test D/App: test

etc...

I create BaseFragment

public abstract class BaseFragment<V extends MvpView, P extends MviPresenter> extends MviFragment { protected P presenter; protected Unbinder unbinder = null; private boolean isAttached = true;

@Override
public void onStart() {
    super.onStart();
    if (presenter != null && !isAttached) {
        presenter.attachView(this);
    }
}

@Override
public void onStop() {
    super.onStop();
    if (presenter != null && isAttached) {
        presenter.detachView(true);
    }
}

@Override
public void onDestroy() {
    super.onDestroy();
    if (presenter != null) {
        presenter.detachView(false);
    }
    if (unbinder != null) {
        unbinder.unbind();
    }
}

}

Now everything works as it should

presenter.detachView(false); - fix it

— You are receiving this because you commented.

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

sockeqwe commented 7 years ago

So in latest version 3.0.5-SNAPSHOT the lifecycle has been changed to onStart() and onStop(). For now onStop() is called before onSaveInstanceState(), therefore you won't run in

Can not perform this action after onSaveInstanceState

However, it is not super clear when onSaveInstanceState() is actually called.

Note however: this method may be called at any time before onDestroy()

Source

With the current Fragment implementation of support lib 25.x it is called after onStop(). However, this seems to be an implementation detail and could change any time in the future.
Hence, a more generic solution might be required in the future.

Please test your app with latest 3.0.5-SNAPSHOT (see instructions in README) and let me know if this fixes your issue. Thanks in advance!

DrobyshevAlex commented 7 years ago

I'm sorry :) I bad speak english.

Last commit don't fix all bugs. It' fixed first error.

Second error in my last message.

Fragment.onDestroy is not trigger detach(false) therefore after each exit application and run - observable send +1 duplicate event to subscribers.

First run app

  • Run app
  • Get response from server
  • Press back button to exit

Second run app (without kill)

  • Run app

  • Get response from server 2 times

  • Press back button to exit

  • Run app

  • Get response from server 3 times

because Fragment.onDestroy() not call presenter.detachView(false)

sockeqwe commented 7 years ago

So the workflow should be as follows:

  1. Fragment.onStart() --> presenter.attachView() --> presenter.bindIntents()
  2. As long as your Fragment is in foreground it will be "updated" by calling view.render().
  3. You press home button --> presenter.detachView(true) --> Presenter is kept (not destroyed). Every incomming websocket message will still be handled in RxJava chain but since no view is attached the latest one will be "stored" internally and once View gets attached the latest "stored" one will immediately hand over to view.render(latestState)
  4. Apps comes in foreground again, so Fragment is visible again --> presenter.attachView() --> view.render(lastState)
  5. User press back button to exit --> fragment.onStop() --> presenter.detachView(false) --> RxJava Chain from bindIntent() gets disposed / unsubscribed.
  6. Start the app again: Then a new Presenter Instance should be instantiated and workflow starts at 1.

So you are saying that this is not how your app works? Can you please post the logs you see when you run this workflow.presenter.detachView(false) should be called.

Get response from server 3 times

I can't explain where this comes from. Are you sure that websocket is not caching all received messages while no subscriber and redeliver them once presenter is subscribed to it?

DrobyshevAlex commented 7 years ago

I debug the app. Really presenter.onDestroy(false) was triggered.

But if a remove line presenter.detachView(false); from fragment onDestroy then unsubscribe not work...

DrobyshevAlex commented 7 years ago

I found.

I debug again. So:

  • App run
  • App -> setFragment LogoFragment (without add in BackStack)
  • App -> setFragment HomeFragment
  • Logo.detach(true) (its right so app is not stopped)
  • Press Back Button
  • HomeFragment.detach(false)

so we have'nt detach(false) for hidden fragment if close app

sockeqwe commented 7 years ago

Got it! Thanks for the quick response.

Well this is another kind of issue, it's not related to your original one. It's backstack management related.

Do you mind to open a new issue where you describe which fragment is on the backstack and which one is "hidden" etc. so that I can reproduce it in isolation and write the corresponding tests.