Direct FragmentRouteStorage

[sellmair]

It may be possible to implement a FragmentRouteStorage that takes the key of a route and retrieves the router later from the router by using the key again

[isaac-udy]

Can you provide some more information on what you're thinking could be done here?

[sellmair]

Sure! Right now, if a fragment asks for its corresponding route object, its arguments will get deserialized and returned by the ParcelableFragmentRouteStorageSyntax. It might be a good idea to go to the routing stack directly and retrieve the corresponding route based on the key of the routing stack entry, since its already there in memory!

[isaac-udy]

I've done something like this in the past, providing unique ids for each Route/Fragment pair, storing the Route in memory, and using the Storage object to retrieve from memory. In addition to this, the storage would be bound to an ActivityLifecycleCallbacks and a FragmentLifecycleCallbacks on the Application, and whenever a Fragment with an ID bound to an existing stored Route is called for onSaveInstanceState, the Storage object saves the corresponding Route to disk.

This has several advantages - performance in the case of starting Fragments, as nothing is serialised until onSaveInstanceState, and advantages for what data is actually able to be saved, as you avoid the 500kb that can cause TransactionTooLargeException. Obviously you shouldn't be aiming to store 500kb of data in a Route's instruction, but if a Route allows a List as a parameter, this is always a risk.

There are a few disadvantages to this solution, unchecked casts (but I think this will be the case with any solution), and needing to be careful about managing the storage (to avoid leaving dangling saved Routes on disk).

What do you think of this idea?

[sellmair]

In addition to this, the storage would be bound to an ActivityLifecycleCallbacks and a FragmentLifecycleCallbacks on the Application, and whenever a Fragment with an ID bound to an existing stored Route is called for onSaveInstanceState, the Storage object saves the corresponding Route to disk.

I did not understand why this solution would require us to save the routes to disk? Right now, the router itself has all the routes (stored as RoutingStack.Element which has a unique uuid for every route). It would be possible to just ask the router to provide the route.

as you avoid the 500kb that can cause TransactionTooLargeException

Do you know whether or not onSaveInstatnceState also has this 500kb limit? Because right now, we are storing the routes using the bundle provided here on orientation changes or process death. If this also has the limit, than we would be forced to write the routing stack on disk, yes :/

[isaac-udy]

I did not understand why this solution would require us to save the routes to disk?

Mainly because of onSavedInstanceState. See below:

Do you know whether or not onSaveInstatnceState also has this 500kb limit?

It certainly does - this was a recurring bug that we had at my workplace. We had a list, with 2kb items in it, and the "normal" size was around 1 - 20, but some users had over 250 items in the list and would consistently crash after back-grounding the app.

[Zhuinden]

Well they do say you should store filters and user inputs and IDs in the savedInstanceState Bundle, and not full lists of complex data.

Data should be reloaded based on the state.

[isaac-udy]

I do agree with you there @Zhuinden - I'm certainly not suggesting that what we were doing at my workplace was good architecture at all. I just find that it's useful to swap a crash for performance degradation in this case.

I can probably throw together a quick implementation that doesn't write to disk, and open a PR for everyone to have a look at.

[sellmair]

@isaac-udy Let's go for it! We can make it configurable which implementation the router will use under the hood 👌

Maybe defaulting to the direct storage would make sense!

[Zhuinden]

The tricky thing about this is that onSaveInstanceState is synchronous, but other means of disk persistence is typically not.

Also, you need to start considering that when you do exit the app with back presses, this stuff should be forgotten, but so should it be if the task is cleared, app is force stopped, etc.

And then you need to consider that you can have multiple tasks, and you shouldn't be using stuff of another task stack.

Disk persistence of navigation stuff is tricky, that's why I never added it, personally.

[isaac-udy]

I've just spent some time looking at this, and I've got a proof-of-concept, but I'm no longer sure this is a good idea.

I've spent some time looking at the implementation of the Bundle class, and as far as I can tell, this will only be serialized/deserialized just-in-time. Under the hood, Bundle uses a Map<String, Any>, and also has a Parcel object. Only one of these will be non-null at any given time. If the Bundle has been parcelled, it will be unparcelled and the data will be stored in the map until the the object is parcelled again.

I believe that this means a simple arguments.putParcelable shouldn't cost any more than storing the parcelable object in a map somewhere else, but will significantly increase the complexity of the code.

What thoughts do people have on closing this issue, and continuing to use the ParcelableFragmentRouteStorageSyntax?