Refactor JerboaAppState into a viewmodel, instead of passing data between screens using SavedStateHandle
#1728
Comments
dessalines
added
enhancement
Since 2.8 navController they added type safety features SafeArgs
All Datatypes implement this. Its not that much of a drawback.
SavedStateHandle is the only persistence that survives process-death. Currently we kinda use all three approaches.
I don't exactly see how you are going to replace The advantage with Currently I don't really know/experience about panes/embedded navhosts. But if they all exist with the same destination then you don't need I would need to see much more concrete example but imo I don't see anything wrong with JerboaAppState other then we need to use type-safety. #1531 |
|
Fair enough w/ typing, but just as important IMO, is de-coupling shared data from the router / navcontroller. For example, with this tablet UI, we now have 2 different nav routes for posts:
Lets say you create a comment or post for the 2nd case: the embedded router has no idea about the action, because its tied to a navcontroller you might not be using.
We don't really need the 2nd of the third approach, with all this custom code around consuming saved state handles. We can refactor
A viewmodel should be able to do that also (the data will persist in the same way), and if we need to remove the data after its consumed, that should be possible also. My tablet PR removes that bottom nav in favor of their new adaptive navbar also. The main difference between this new global shared viewmodel I'm proposing, that's different from our current viewmodels, is that the purpose of those is mainly to do network requests and persist that data for a specific screen. This new one will specifically be about sharing data between screens. We might as well refactor |
This has advantages though How will you replicate this behaviour? -> home Needs to show the original post again? Assuming you are suggesting class RouterVM {
var postState ...
fun goToPost(id...) {
postState = id...
navController.navigate(Route.Post) // ?? still need navController
}
}I just fear it will be very messy, and it still not clear to me, how exactly you are going to replace it? It will need to support process-death so the RouterVM needs to use SaveStateHandle too. And it will need to support deeplinks. |
None of that actually needs a shared app state, all that's required there is to pass a navcontroller to the screen, and use The problem I'm describing is about sharing data between screens.
The process death is not important IMO because the shared data could get cleared out after its read / consumed anyway. |
|
Okay its bit more clear now but arguably my example still kind of applies. Let me show you can example but its a bit of an edgecase but its similar to a bug we have had. It is just for illustrative purposes but there are probably better cases. But its an edge case we have to support regardless imo
This also reminds me of a bug in the early days where we used to have only global viewmodels. When it failed to a load a posts comments it would show the comments of the previous post. Which was very confusing :D
Debatable, you don't always consume it, here an example
We can still use Now back to
I didn't make my argument clear before. We absolutely do want our state to be scoped to the "screen destination entry". (little example)
Each should hold its own scoped state and should be cleared when the entry is removed from the stack. Using the navcontroller was/is ideal for this, we can access the stack and thus the JerboaAppState wraps the navController as its unstable + now only exposes "typesafe" navigations + Framework to persist data in the As for your suggested approach: We can also access the But we need also need to scope this viewmodel to the "screen destination entry"/backstackEntry But if you scope it to the screen you can't access it from another screen. Thus you can't pass the data through the viewmodel like this. :/ So you would have to make a "global" viewmodel i.e one bound to the activity. But then you will have the problems I just described here. And the ones we have before when we had global viewmodels everywhere. But it ll be more complex as we will still have the main scoped viewmodel + now RouterViewmodel global state. (States no longer in sync) I understand because of this it doesn't exactly work right now with panes. Atm I don't know a lot of panes but I think its possible that we can still apply the same trick with panes. As long as it has the same I used NowInAndroid as an inspiration originally and they are using adaptive now too. So we can do the same again. |
|
The link outlines the problem I'm running into: that panes and tablet UI's use their own navhost / navigator. Normally UI's do this by "protecting / guarding" edit/create-type routes, so that when you navigate away from them, it warns you that the data will be lost. Its a bit overkill IMO to try to keep edit or create screen data on the stack also, it should probably popUpTo wherever it came from, rather than trying to keep all this shared data scoped to specific routes. Also in the case above you could easily create a I greatly doubt most lemmy ui's do anything this complicated, and its not necessary. Most importantly though, its not flexible enough to use with tablet UI's and different navhosts. |
Pre-Flight checklist
Describe The Feature Request Below
@MV-GH I wanted to get your input on this before I start it.
After reading this article on passing data between screens, it seems that the way we're passing data between composables (Using the SavedStateHandle), isn't as good as using a viewmodel (ignore their hilt recommendation, we can just use a regular jetpack one).
Cons
It'd be much better if we turned
JerboaAppStateinto a viewmodel, and used it as a persistent way to pass data between screens. Wouldn't rely on any navcontroller, and would be well typed.The text was updated successfully, but these errors were encountered: