ViewModel's SaveState/ReloadState and NavigationService

[nmilcoff]

This is a concern for every platform, but maybe Android is the most affected one because if we don't do something, apps will crash.

Basically the SaveState/ReloadState mechanism is activated when the device goes out of memory, so what we do internally is to provide a way for users to save the state of their ViewModels and then a way to reload it. This is how it goes inside the framework:

1. If there is only one ViewModel going into tombstone (of if many, then the last one): This ViewModel will not be destroyed, it will be saved in a "fast" cache named IMvxSingleViewModelCache instead. Then when the View is coming back, the ViewModel is just retrieved from the cache.

2. For the rest of the ViewModels (these are probably part of a backstack): When the Android View is destroyed, each ViewModel gets a chance to save its state by populating a Bundle (which contains a Dictionary<string,string>) before being disposed. After this process - when the View comes back - the ViewModel is loaded from the ground up, and ReloadState is supposed to be called with a Bundle that contains the previously saved data.

I have done some testing for this and so far it seems to be working for Activities + new AndroidViewPresenter + MvxNavigationService I didn't test it for fragments yet, as we didn't finish the caching functionality yet.

BUT there is a problem with lifecycle events for ViewModels: As ReloadState is part of the “old” CIRS (Constructor - Init - Reload State - Start), it is called by IMvxViewModelLocator.RunViewModelLifecycle, so currently this is what the chain looks like when a ViewModel is coming back to life:
Init -> ReloadState -> Start.

Just that. Prepare and Initialize are not getting called.

This makes sense - in a way - but if these methods are not getting called then the developer can't really recreate the ViewModel.

That being said, I think it will be really difficult to make a call to Initialize when reloading the state, because we are working in a sync context (View lifecycle events).

I have two alternatives in mind right now:

1. Store the entire stack of ViewModels in a cache singleton (instead of the last one opened). This will probably require the developer to be responsible for configuring which ViewModels should be stored in that cache.

2. Leave the process as it is right now. This means that once we finally deprecate CIRS, the process or saving/reloading will only involve SaveState -> ReloadState. This leaves the responsibility of running the ViewModel initialization to the user.

[nmilcoff]

When you say a viewmodel is stored in a cache. Does that mean that you serialize the viewmodel using the json serializer and then dispose it ? If yes, you are serializing potentially unserialiable things. A default behavior is still a nice idea.

No, it isn't serialized. A reference to the ViewModel object is kept in a singleton cache.

Should the viewmodel's SaveState then Dispose methods be called instead ? And for rehydratation, should it be recreated and reinitialized with a rehydratation parameter instead ? (like the bundle parameter in android's oncreate) ?

MvxViewModel doesn't implement IDisposable currently. Are you suggesting we should add it? When a ViewModel is rehydrated, it is recreated and it receives all saved information in a bundle, specifically in ReloadState(MvxBundle bundle).

The problem is, the ViewModel is completely recreated and it receives some parameters, but Prepare and Initialize are not called (old Init and Start are still called)

[ElteHupkes]

I bumped into this issue after investigating how to properly do tombstoning in Android / Mvx 5.3 and realized that indeed Prepare and Initialize would never be called, and I have some remarks:

• As you mention, the singleton cache only applies if a single view gets tombstoned. In my experience this rarely happens anymore - much more common is the scenario where the app isn't used for a while, the entire process is killed, and is then revived by the user by going to the task list and tapping the thumbnail. This is not quite similar to the "Don't keep activities" debug option, which keeps all the services and such alive. Approach (1) is therefore probably not a good idea, because the cache will also be gone.
• The main issue with Prepare and Initialize arises when the view model has parameter data. In 4.x when using ShowViewModel<TViewModel> with a parameter object, the documentation explicitly limited this parameter object to have only "simple" properties so it could be serialized into the Android Intent (and probably other places). Upon reviving the view model, the appropriate Init method would be called with the unserialized parameter data. Right now, because Initialize() and Prepare(TParameter p) are skipped, parameter data is essentially lost upon tombstoning rendering the whole process quite useless for view models that require them. The same is true for returning data from view models using the new Close() mechanism by the way, but that's an entirely different discussion.

The way I see it there are two options to solve this particular problem:

1. Require parameter objects to be serializable, save them to the Intent, and work Prepare() and Initialize() into the rehydration process somehow. Indeed their being async is a bit of an issue. Because everything is being revived and there isn't much of an active UI at this point, having them run synchronously might not be the biggest issue. Either that or the task bubbles up to an async void eventually, which at least lets the view continue loading...
2. Basically your option 2: Explicitly state that Prepare() and Initialize() are not called when rehydrating a view model and leave the responsibility to the user to work around this in SaveState()/ReloadState(). This is the more flexible option, but it does leave things broken out of the box (as is the case now of course). You run the risk of most people not going through the hassle of figuring it out and fixing it at all. Even more so because the scenario is quite a hassle to test - you have to actually adb kill the app process to make it happen.

For me personally, (2) would be fine; I already have a neat BaseViewModel in place which stores parameter data in the bundle using a custom JSON serialization. I can't really say how this would affect other people.

[nmilcoff]

Just to elaborate my first alternative a bit more, what I mean is to save ViewModels by default in the singleton cache, and give the user the ability to disable this.

If disabled, a ViewModel could use SaveState/ReloadState (without Prepare and Initialize) to rehydrate.

[ElteHupkes]

@nmilcoff I understand your solution, but in my experience it is not actually a solution for the most common scenario. Most phones have plenty of memory these days, so it's rare to see individual activities being killed unless "Don't keep activities" is turned on (which is purely artificial). What does appear to happen with our app regularly is that it is backgrounded for a long period of time and then recalled from the app drawer hours later, at which point the entire app has been cleared from memory and every single activity is restored from saved state. In this case the singleton cache is gone as well.

@softlion In a nutshell:

public class BaseViewModel<TParameter> : MvxViewModel<TParameter> 
        where TParameter: class, new()
    {
        public TParameter Parameter { get; protected set; }

        protected override void SaveStateToBundle(IMvxBundle bundle)
        {
            base.SaveStateToBundle(bundle);
            bundle.Data.Add("Parameter", ObjectSerializer.Serialize(Parameter));
        }

        protected override void ReloadFromBundle(IMvxBundle state)
        {
            base.ReloadFromBundle(state);

            string data = null;
            state.SafeGetData()?.TryGetValue("Parameter", out data);
            if (data != null)
            {
                var parameter = ObjectSerializer.Deserialize<TParameter>(data);
                Prepare(parameter);
            }
        }

        public override void Prepare(TParameter parameter)
        {
            Parameter = parameter;
        }
    }

[ElteHupkes]

@softlion I realize there are several ways in which activities can disappear - if you want to be able to deal with all of them (and especially the most common one) a singleton cache won't do.

With regards to Initialize - if I look at MvxNavigationService, it is called after the view presenter has done its work, so after Activity.StartActivity. ReloadFromBundle will be called in Activity.OnCreate, because that's where the savedInstanceState is passed in, so you would be calling it significantly earlier, which may or may not be a problem. Then there's the little matter of Initialize being async, so it also depends on how you want to handle that. In my case (above is not my actual code, just "paraphrasing"), in most cases, making ReloadFromBundle async void and calling await Initialize(); at the bottom probably works just fine.

[nmilcoff]

@ElteHupkes once again... I'm not saying we save only one ViewModel in the cache, but the entire stack (by default). So the scenario you mention would be covered.

Also looking at your code, there's nothing special in it other than calling Prepare from ReloadFromBundle... Which is neat. We can probably add that call in the fwk (need to carefuly place it) and then call Initialize using a TaskSandbox that logs any exceptions.

[ElteHupkes]

@nmilcoff Ok, I may be misunderstanding you, I don't mean to be rude or anything. The way I interpret your initial post, is the reason for storing all view models in the singleton cache is to all get them into state (1) where they essentially don't need to go through ReloadState because the instances still exist. If not that, what is it you're trying to achieve with the singleton cache?

[nmilcoff]

@ElteHupkes yes, that's the idea :). Basically, if they were kept alive, there's no need to rehydrate them.

I'd propose to do this anyway:

• Don't call Prepare when rehydrating VMs. In the old CIRS, when coming back from tombstone Init was called without parameters, so this would be very similar. Initial values should be stored using SaveState / ReloadState.
• Call Initialize after ReloadFromBundle using a task sandbox to avoid an async void method in the platform lifecycle. Something like this should probably work:

private static async void RunInSandbox(Func<Task> myTask)
{
  try
  {
    await Task.Yield();
    await myTask.Invoke();
  }
  catch (Exception ex)
  {
    // log the exception and maybe rethrow
  }
}

What do you guys think? Would be nice to have your opinions here as well @MvvmCross/core.

[ElteHupkes]

@nmilcoff Okay, but my point is the entire app is often cleared from memory - the singleton cache is gone too. Rehydration is required one way or another. I've had some additional beef with the singleton cache because the instance living inside it is never garbage collected, which forces you to be much more careful with any event handling / message subscriptions you may have going on in there. From ViewDestroy you won't be able to tell if your activity is gone for good or simply backgrounded, so you may never know when to clear a view model out of the cache.

Actually, in the old CIRS, any parameter data passed to ShowViewModel would be serialized into the Extras of the initial Intent used to launch the activity. Since Android recreates this activity when returning from a suspended state any Init(TParam param) would actually be called upon rehydration - I've relied on this quite heavily in the past. If that weren't the case not calling Prepare would make more sense. One could argue that a proper view model SaveState should include any Prepare data as well, but this is not necessarily how it worked in the past.

With regards to the async issue, I guess the question is what else is going to on while the view model is awaiting the Initialize method - does it even make sense to yield? Also, what's the advantage of the sandbox method, doesn't that just move the problem?

[nmilcoff]

@ElteHupkes we're trying to phase out from reflection during navigation, so probably the best way to advance on this is just require users to save their parameters in SaveState.

I think it makes sense to yield, because otherwise the method might run in a sync way and block the UI.
The sandbox is just a proposal, which lets us avoid marking lifecycle methods as async at a framework level... but I'm happy to hear any other ideas, let's provide a solution for this guys!

@softlion AFAIK it's not possible to use ConfigureAwait(false) in a Task.Yield call. Can you please elaborate?

[Cybrosys]

I was surprised by this as well. I started migrating the navigation code from 4.x to 5.x and wiped out all my Init, ReloadState and SaveState code...

I thought that since I started using the MvxViewModel<TParameter> you would do the Serialization/Deserialization for me, since you're doing json serialization in Prepare.

I'm surprised this isn't mentioned in the documentation when you go over the steps to migrate from 4.x to 5.x or in the Lifecycle documentation. You do talk about SaveState* under the 4.x header during VM Lifecycle, but not under 5.x.

https://www.mvvmcross.com/documentation/fundamentals/navigation
https://www.mvvmcross.com/documentation/fundamentals/viewmodel-lifecycle

[Cybrosys]

In addition, I had some recovery code or recreation code that basically did a StartActivity(Intent) and FinishActivity() when something crashed, or when I needed to re-localize the UI. But since moving to the new Prep/Init pattern, the Intent extra/bundle is just a blank version of the view model (not the one that was sent during the navigate call), so I had to do a IMvxNavigationService.Navigate call instead with explicit values that I knew was sent.

I get the new prepare and initialize, but I don't think the intent extra/bundle state stuff should get broken because of it. Is there a best practice to accommodate both the old and the new? Which ViewModel methods should I implement/override in order to be sure that the state is properly saved and available when needed, whether it's to recover from a crash or due to a navigation request?

[nmilcoff]

@Cybrosys I'm not sure which intent / bundle are you referring to, can you please point out a line of code? Presentation bundles and stuff wasn't removed.

@ElteHupkes as far as I know, MvvmCross never did automagically restore the state of ViewModels. When recovering from tombstone, Init was normally called with parameters = null and you received the saved state in the bundle for ReloadFromBundle. So I'm not sure I understand how it worked for you in the past.

[Cybrosys]

@nmilcoff before the 5x migratiopn I used the following method:
ShowViewModel<TViewModel>(object parameterValuesObject, IMvxBundle presentationBundle = null)
which looked like this:
ShowViewModel<ShoppingControlsViewModel>(new ShoppingControlsViewModel.SavedState { })

That method populated the activity intent with the passed in SavedState object, so I could, if needed, do a StartActivity(Intent) inside the new activity (if I wanted to recreate it).

But now that I'm using:
Task Navigate<TViewModel, TParameter>(TParameter param, IMvxBundle presentationBundle = null)
which looks like this:
await _navigationService.Navigate<ShoppingControlsViewModel, ShoppingControlsViewModel.SavedState>(new ShoppingControlsViewModel.SavedState{ })

The Intent doesn't contain the SavedState object any longer. There's probably a good reason why it worked last time and why it doesn't work any longer. In either case, i've found a work-around for when I need to recreate activities.

[rrispoli]

The solution posted by @ElteHupkes works for me, I just adapt it in my BaseViewModel to work in all my viewmodels:

public abstract class BaseViewModel<TParameter> : BaseViewModel, IMvxViewModel<TParameter> where TParameter : class
{
	private TParameter _parameter;
	public TParameter Parameter
	{
		get { return _parameter; }
		set
		{
			_parameter = value;
			RaisePropertyChanged(() => Parameter);
		}
	}

	public void Prepare(TParameter parameter = null)
	{
		if (parameter != null)
		{
			Parameter = parameter;
			SpecificPrepare(parameter);
		}
	}

	public virtual void SpecificPrepare(TParameter parameter)
	{
		//Implement in ViewModel to do specific code
	}

	protected override void SaveStateToBundle(IMvxBundle bundle)
	{
		base.SaveStateToBundle(bundle);

		if (Parameter != null)
		{
			bundle.Data["ViewModelType"] = GetType().ToString();
			bundle.Data["Parameter"] = JsonConvert.SerializeObject(Parameter);
		}
	}

	protected override void ReloadFromBundle(IMvxBundle state)
	{
		base.ReloadFromBundle(state);

		try
		{
			if (state != null && state.Data != null && state.Data.Any())
			{
				string viewModelType = "";
				string parameter = "";

				state.SafeGetData()?.TryGetValue("ViewModelType", out viewModelType);
				state.SafeGetData()?.TryGetValue("Parameter", out parameter);

				if (!string.IsNullOrWhiteSpace(viewModelType) && viewModelType == GetType().ToString() && !string.IsNullOrWhiteSpace(parameter))
				{
					Prepare(JsonConvert.DeserializeObject<TParameter>(parameter));
					RunAsyncTaskInVoid(Initialize);
				}
			}
		}
		catch (Exception)
		{
			//Navigate to your FirstViewModel
		}
	}

	protected async void RunAsyncTaskInVoid(Func<Task> myTask)
	{
		try
		{
			await Task.Yield();
			await myTask.Invoke();
		}
		catch (Exception)
		{
		}
	}
}

But I was thinking, in some simple cases, there is no reason to restore the viewmodel/view. @nmilcoff What I need to do to "restart" the app from the SplashScreen again?

[nmilcoff]

I'm working on a fix for this, I'll make a PR later today.

@rrispoli I guess you need to create an Intent for it and call StartActivity(). Probably a bit offtopic for this issue.

[rrispoli]

@nmilcoff Ok, thanks!

[ElteHupkes]

@nmilcoff Actually, when calling MvxNavigatingObject.ShowViewModel<TViewModel>(object param) in for instance 4.3, the parameter is serialized into an IMvxBundle, passed along as the parameterBundle of the view model request. The MvxAndroidViewsContainer uses the INavigationSerializer to serialize the entire MvxViewModelRequest into the Intent used to launch the activity - when restoring the Activity an identical Intent will be used by Android. When the view model is loaded for the restored activity, the view model request is recreated from the Intent:

https://github.com/MvvmCross/MvvmCross/blob/4.3.0/MvvmCross/Droid/Droid/Views/MvxAndroidViewsContainer.cs#L91

In the end, the view model will be created in the same fashion whether it be from ShowViewModel or from a tombstone, and Init is actually called with the initial data. I always thought that was rather neat, though the execution path to get there is next to impossible to follow.

[nmilcoff]

You're right @ElteHupkes. But for fragments I think it didn't work like that. When recovering from tombstone, Init never had the navigation parameters. So the PR I made will provide a standard behavior - we can improve that later if we come up with a way to do it inside the framework -.

[ElteHupkes]

Aah okay, I wasn't working with fragments that had state before tbh, so I didn't know that, thanks for the clarification.

With regards to your PR, over the past days I've come to the conclusion more and more that VM lifecycle really needs to be handled by the ViewModelLoader, so definitely good thinking there.

There's an ugly case if you use fragments as in the examples, by opening them with NavigationService.Navigate() combined with a fragment or tab presentation attribute. If the fragment is restored from state, it is quite possible that it already has a view model. Because Navigate always creates a new one, there now are two view models. The old one is replaced by the new one, leaving the new one orphaned without going through any lifecycle methods. Unsubscribing view model event handlers that, for instance, should really only be active when the view is visibile is now completely hopeless. This could probably only be solved by centralizing view model initialization.