LemmyNet / jerboa

A native android app for Lemmy
GNU Affero General Public License v3.0
1.17k stars 168 forks source link

Refactor `JerboaAppState` into a viewmodel, instead of passing data between screens using `SavedStateHandle` #1728

Open dessalines opened 2 days ago

dessalines commented 2 days ago

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 JerboaAppState into 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.

MV-GH commented 1 day ago

Untyped

Since 2.8 navController they added type safety features SafeArgs https://developer.android.com/guide/navigation/design/type-safety

Requires parcelable and serialization.

All Datatypes implement this. Its not that much of a drawback.

SavedStateHandle

SavedStateHandle is the only persistence that survives process-death. https://developer.android.com/topic/libraries/architecture/saving-states#options

Currently we kinda use all three approaches.

I don't exactly see how you are going to replace SavedStateHandle with Viewmodel. Are you going to add a destination state variable per screen?

The advantage with SavedStateHandle is that the state bound to the route. Meaning if you go deeper and then pop back. The state is still there. And if you go open something again it doesn't retain the previous navigation state. And you can control this behaviour see https://developer.android.com/develop/ui/compose/navigation#bottom-nav

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 JerboaAppState to pass data between them? And can just use a normal viewmodel?

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. https://github.com/LemmyNet/jerboa/issues/1531

dessalines commented 1 day ago

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.

  • Global Viewmodels like siteViewModel AccountViewModel
  • SavedStateHandle to pass data between Screens.
  • Route Arguments to get state from the route path.

We don't really need the 2nd of the third approach, with all this custom code around consuming saved state handles. We can refactor JerboaAppState into a global viewmodel as suggested here to store and read shared data that needs to be passed between screens AND panes, independent of any router.

The advantage with SavedStateHandle is that the state bound to the route. Meaning if you go deeper and then pop back. The state is still there. And if you go open something again it doesn't retain the previous navigation state. And you can control this behaviour see https://developer.android.com/develop/ui/compose/navigation#bottom-nav

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 JerboaAppState into a viewmodel, since its currently serving that purpose.

MV-GH commented 13 hours ago

de-coupling shared data from the router / navcontroller.

This has advantages though

How will you replicate this behaviour?

-> home -> open post -> go to profile of a commenter -> open a post -> go back -> go back

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.

dessalines commented 9 hours ago

-> home -> open post -> go to profile of a commenter -> open a post -> go back -> go back

None of that actually needs a shared app state, all that's required there is to pass a navcontroller to the screen, and use onClickPost or onClickProfile events to navigate (or pop back, in the case of things like creating comments)

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.