Proposed Changes for Common Use Cases

Jul 23, 2013 at 7:33 PM
First of all, I want to say I'm grateful for this library. The approach informed my own implementation of a helper, with a few tweaks I wanted to pass along that you might find useful for the library.

The first is that I don't strictly agree that the null check on line 43 of ApplicationState.cs has a lot of utility. If a property is decorated with TombstoneAttribute, you can probably safely make the assumption that the developer is expecting an assignment to that value and they need to decide wisely where in the page life cycle that restoration occurs. That's not a decision that should be made by the helper. Take for instance the following use case:
protected override void OnNavigatedTo(NavigationEventArgs e)
{
    base.OnNavigatedTo(e);

    if (this.IsNewInstance)
    {
        ViewModelBase viewModel = this.DataContext as ViewModelBase;

        // Run any parameterized initialization required by the viewmodel
        if (viewModel != null)
        {
            viewModel.Initialize(this.NavigationContext.QueryString);
        }

        RestorePageState();

        this.IsNewInstance = false;
    }

    TombstoneHelper.OnPageNavigatedTo(this, e);
}
You will notice in this pattern that the ViewModel is initialized with parameters passed during navigation. For instance, the ViewModel is given the record ID for a record to retrieve and load into its properties. The initialization will most often populate those properties with non-null values so the check would fail and the restore operation would be skipped. I don't think it's the responsibility of the tombstone helper to make assumptions about the state of the ViewModel on restore, so I'd suggest minimizing those assumptions and removing this check.

Finally, I'd also suggest a change to line 70 of ApplicationState.cs to include private properties and remove the check in lines 74 through 78. The suggested change is as follows:
IEnumerable<PropertyInfo> tsProps =
    from p in ViewModel.GetType()
        .GetProperties(BindingFlags.Instance |
                       BindingFlags.Public |
                       BindingFlags.NonPublic)
    where p.GetCustomAttributes(typeof(TombstoneAttribute), false).Length > 0
    select p;

return tsProps;
Reflection is pretty powerful, and there's no reason we can't leverage that to get a hold of private decorated properties. In my use case I have a private ViewModel list property that stores staged photos until a user hits "Save" and they're moved to their final location. The list is exposed by the ViewModel through a separate public read-only property which is really just a concatenation of existing saved photos and staged ones. The saved photos are easy to access on restore, but the staged photos are only stored in memory and need to be restored, but not necessarily accessed directly by the view. I imagine there are other cases where storing private properties makes sense, and it happens to save the cost of having to iterate the list again, inspecting properties and throwing an exception.
Coordinator
Jul 26, 2013 at 12:23 AM
Hi,

First of all thanks for the feedback.
I will definitely look into your comments. I'd have to revisit the code for a moment, because it's been a while since I've worked on it.

The null-check I remember putting in for a really good reason. I didn't want it either but in the end it was necessary. If I'm not mistaken, it's was because of the fact that you can't know when the application was tombstoned or just dormant. In case it was dormant, the value was still available and it would have been an bad to do the operation every time. I'm not a 100% sure though, so I will have to check that again.

As for the reflection, you're absolutely right, I think it just didn't occur to me to allow this to pass as well.

I will come back to you with more answers later. Thanks again!