Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Presenters cannot restore instance state after process has died #39

Open
ihbyun opened this issue Nov 18, 2016 · 9 comments
Open

Presenters cannot restore instance state after process has died #39

ihbyun opened this issue Nov 18, 2016 · 9 comments
Labels

Comments

@ihbyun
Copy link

ihbyun commented Nov 18, 2016

It looks like there's no good way to restore the instance state of a Presenter in ThirtyInch when the process is killed and restarted.

The PresenterSavior handles the case when the process is NOT killed by essentially holding a static map of Presenters, but this map will get cleared when the process dies and the Activity gets restored.

This can be reproduced by running the sample code and taking the following steps:

  1. Go to Developer Options and turn on Don't Keep activities and set Background Process limit to "No background processes"
  2. Install the provided sample apk from https://github.com/grandcentrix/ThirtyInch/tree/v0.7.1/sample
  3. Open the app
  4. Press Home
  5. Navigate back to the app

After following the steps you can see in the logs

11-13 05:49:47.407 909 909 I ThirtyInch: HelloWorldActivity:TiActivity@9cbcc1d: could not recover the Presenter although it's not the first start of the Activity. This is normal when configured as .setRetainPresenterEnabled(false).

As far as I understand setRetainPresenterEnabled is true by default and no one sets it in the sample code.

Is this an oversight or is there a recommended way to take care of this case?

@passsy
Copy link
Contributor

passsy commented Nov 20, 2016

ThirtyInch can nothing do against process death and has no APIs to restore the state. It was developed by assuming that the process rarely gets killed which is true for modern phones.
When you see your app process getting killed you have all Android tools to restore the state you already know.

savedInstanceState

You can save the state of the presenter in onSaveInstanceState(Bundle) and restore the state when the presenter gets recreated providePresenter() after a process death

public class MyActivity extends TiActivity<MyPresenter, MyView> implements MyView {

    // make it accessible in providePresenter()
    private Bundle mSavedInstanceState = null;

    @Override
    protected void onCreate(final Bundle savedInstanceState) {
        mSavedInstanceState = savedInstanceState;
        super.onCreate(savedInstanceState);
    }

    @Override
    protected void onSaveInstanceState(final Bundle outState) {
        super.onSaveInstanceState(outState);
        // using a byte[] here because the presenter should have zero Android dependencies
        // Bundle is hard to mock
        outState.putByteArray("presenterState", getPresenter().getPersistentState());
    }

    @NonNull
    @Override
    public MyPresenter providePresenter() {
        // called when the presenter gets first created and 
        // when the presenter gets created after a process death
        byte[] persistentState = null;
        if (mSavedInstanceState != null) {
            persistentState = mSavedInstanceState.getByteArray("presenterState");
        }
        return new MyPresenter(persistentState);
    }
}

app storage

You should really think about your usecase. Often it is not useful to restore a state when the Activity was in background for days. Another solution is to manually save the state as file in the app directory. Or in the SharedPreferences. It could be good practice to also save the timestamp and check if the state is still relevant in the current situation.

@k0shk0sh
Copy link
Contributor

this also could happen in Activity B where it crashed, then activity A (MainActivity) is now being recreated.

@vRallev
Copy link
Contributor

vRallev commented Dec 14, 2016

Would you be open for a PR? I have a working solution which looks roughly like this

  • Bundle is wrapped by a TiBundle interface (and their implementation for the JVM and Android in the corresponding modules)
  • TiConfiguration gets a new field which enables this feature (off by default)
  • TiPresenter has a new onSaveInstanceState(TiBundle) method

For recreating the state I see two options. The first would be changing TiPresenter.onCreate() to onCreate(TiBundle). Problem is that this would break every app out there. So better would be this option:

  • TiPresenter has the methods restoreInstanceState(TiBundle) and onRestoreInstanceState(TiBundle), which is invoked by the delegates, if the presenter needs to be recreated (not found in memory) and the bundle is not null
  • The method is invoked before .create(), because then we already need to use the saved state

That's nothing complicated, but would be a nice addition.

(I also saw the other issue where someone needs to store the webview state in a bundle, this would fix this issue probably, too)

@passsy
Copy link
Contributor

passsy commented Dec 14, 2016

@vRallev I fear that the TiPresenter ends up being a subset of Activity with similar callback methods and similar lifecycle but surviving orientation changes. While this could be a reasonable direction for this library I'm looking forward to move it in a more advanced direction.
I prefer to interpret callbacks from the Activity like Activity#onSaveInstanceState(outstate) as indicators when it is a good time to do something rather then using the bundle to save the state.
I'd prefer writing the state to disk without having to worry about the Activity. onSaveInstanceState would trigger this mechanism but also the user of Ti could decide when it is a good time to save the state and do it manually as often as he likes. It also grants us to save the state while the Activity is paused and onSaveInstanceState has already been called. Think about a heavy calculation which finishes when the activity is in background. It's not possible to save such a state with onSaveInstanceState.
The past showed that using Activity APIs isn't the best solution. I.e. onRetainCustomNonConfigurationInstance() to survive the orientation change works (most of the time) but it's better to keep a reference in our own PresenterSavior.

Having a complete standalone way to save the state is much more flexible. It would allow us to have a maximum of X active Presenters while the others will be killed (and save state). Another idea would be to kill all Presenters when the App has been moved to background after X Seconds. So many possibilities which aren't possible when we rely on onSaveInstanceState.

Nothing has been decided yet and I'd be happy to get more feedback from people like you. Anyways, it's open source and you have all rights to maintain you own fork which could be integrated once a decision has been made.

@k0shk0sh
Copy link
Contributor

I really love seeing what @vRallev suggested, this will save us loads of stuff to do on the activity/fragment, imagine we are saving some state of an expensive API call in the presenter, the activity is being recreated due to some activity in the backstack is being crashed, now the whole activities are being recreated and the presenter lost its state & data. i really appreciate your effort on making this possible @vRallev & @passsy

@k0shk0sh
Copy link
Contributor

k0shk0sh commented Jan 5, 2017

@passsy any news about this?

@passsy
Copy link
Contributor

passsy commented Jan 5, 2017

Work started based on #50. Not finished yet but you can have a look:
branch feature/persist-presenter

@vRallev
Copy link
Contributor

vRallev commented Jan 5, 2017

@passsy You might want to create another PR even if it's WIP. I already have a few comments.

@k0shk0sh
Copy link
Contributor

k0shk0sh commented Jan 5, 2017

cool, i'm looking forward for it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

4 participants