ThirtyInch icon indicating copy to clipboard operation
ThirtyInch copied to clipboard

Presenters cannot restore instance state after process has died

Open ihbyun opened this issue 7 years ago • 9 comments

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?

ihbyun avatar Nov 18 '16 21:11 ihbyun

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.

passsy avatar Nov 20 '16 19:11 passsy

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

k0shk0sh avatar Nov 30 '16 06:11 k0shk0sh

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)

vRallev avatar Dec 14 '16 13:12 vRallev

@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.

passsy avatar Dec 14 '16 15:12 passsy

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 avatar Dec 14 '16 15:12 k0shk0sh

@passsy any news about this?

k0shk0sh avatar Jan 05 '17 00:01 k0shk0sh

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

passsy avatar Jan 05 '17 12:01 passsy

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

vRallev avatar Jan 05 '17 12:01 vRallev

cool, i'm looking forward for it.

k0shk0sh avatar Jan 05 '17 12:01 k0shk0sh