Navigation Enhancements x2 #765
Conversation
nahwneeth
force-pushed
the
declarative-navigation
branch
2 times, most recently
from
70fe11d to
87aee1a
Compare
nahwneeth
force-pushed
the
declarative-navigation
branch
2 times, most recently
from
86b56f6 to
df6cc51
Compare
nahwneeth
force-pushed
the
declarative-navigation
branch
from
4388ed4 to
1bd59d5
Compare
|
Whoa this is huge! I pry won't be able to test / go over this for a few days, because it has so many changes.
This is my main concern. ViewModels are supposed to exist outside of activities, so that they can persist without them. Otherwise things like, going to a post, then clicking someone's profile, then clicking the back button, will reload the post, rather than just going to place you were at in the post before. Does this PR mean that essentially all back button, and viewing of old routes, essentially has to get loaded from scratch? Not only that, but thing like comment edits, that initialize data into the view models, would have to start with fresh data. |
|
I have been testing this build since yesterday, and i haven't encountered any issues yet
It behaves like before. In accidentally (or I assume so) is that it removes the navController from many things. Which prevents now the constant recompositions of the bottamappbar . But sadly the scrolling still feels janky. So there are more underlying problems But it is a good start. |
|
The reloading thing has been taken care of. I've created a new composable called @Composable
fun InitializeRoute(initializationBlock: suspend CoroutineScope.() -> Unit) {
// rememberSaveable persists this information across navigations
var initialized by rememberSaveable { mutableStateOf(false) }
// LaunchedEffect runs everytime we navigate. Therefore we need a guard.
if (!initialized) {
LaunchedEffect(Unit) {
this.initializationBlock()
// The effects might get cancelled sometimes and the initialization wouldn't have happened.
// Therefore this has to be inside the effect and after the initialization block.
initialized = true
}
}
}In the post activity we do this: val postViewModel: PostViewModel = viewModel()
InitializeRoute {
postViewModel.initialize(id = postArg)
postViewModel.getData(account)
}The view model gets scoped to the Going to a profile and coming back will not reload the I can add a video demonstrating all the improvements if it helps. |
|
The commit is huge largely because of making the routes type safe. Most of them are new files listing dependencies and the possible navigations and the extracting the arguments from the route in a typesafe manner. In the existing files, only the Do take your time to test it, no worries. Do let me know if there are any issues. |
| val onSelectTab = { tab: HomeTab -> | ||
| if (tab.needsLogin() && account == null) { | ||
| feedNavController.toLogin.navigate() | ||
| } else { |
There was a problem hiding this comment.
This is a change in behavior though, it will navigate to the login instead of displaying a toast like before. Not sure if this is wanted.
There was a problem hiding this comment.
One of the frequent complaints by new users is that it is difficult to find the option to log in. Redirecting users to log in when they attempt to use a function that requires being logged in seems like a good solution to that problem that many have requested.
There was a problem hiding this comment.
Well one of the other suggestions was to add a login button in the more options dialog and/or show login snackbar on initial app use.
It still doesn't solve the root problem as new users still won't know where to switch/log out.
And now is inconsistent with the other buttons that require login(upvotes , ...) that still display display this toast.
Either way, the current dropdown should be made more clear. Maybe we could add a "Click here to login" text next to anonymous. That only shows on initial startup and disappears forever once you click on the anonymous drop down.
I just want a proper solution as after that we will probably see a bunch of "Add log out option" issues instead.
There was a problem hiding this comment.
For the bottom nav bar, the nav controller was already available so went ahead with navigating to the login page. For the upvotes, downvotes and reply, the nav controller (or callbacks) have to be passed deeper down the tree. There's a TODO to show the login page along with the toast, so I assume this will be revisited.
Agree with the log out thing.
|
Found some issues though, clicking on @ linking to subs causes a crash (community@instance) java.lang.IllegalArgumentException: Navigation destination that matches request NavDeepLinkRequest{ uri=android-app://androidx.navigation/lemmy.world/c/syncforlemmy } cannot be found in the navigation graph d0(0x0) startDestination={a(0x8a77bdc8) route=home?tab={tab}}
|
The last commit fixes this. So try with the latest changes. Thanks. |
I did and i just fetched it again and im having the exact same errors. Am i perhaps pulling it wrong? I am doing git fetch upstream pull/765/head:declarative-navigation
git checkout declarative-navigation |
|
Also the about page is even more wrong than before see #725, but i dont think its related to this pr. I think there might be some issues with our resource bundle
|
I've fixed deep links. The community activity needs some refactor and I've added a TODO for it.
And yeah, the about activity issue isn't from this PR. |
|
I can confirm that it is fixed now. |
| } | ||
|
|
||
| @Composable | ||
| fun <D : ViewModel> dependencyContainer(): DependencyContainer<D> { |
There was a problem hiding this comment.
Why do we need all of this? It seems counter intuitive a ViewModel that holds ViewModel to create them... a few options here:
- You can use the compostable function
val vm = viewModel { MyViewModel() }for creating your VMs. - You can call
viewModelFactory { MyViewModel }to create aViewModelProvider.Factoryon the fly. - If you want to recover down the tree to a unique VMFactory, you can create a LocalComposition and set it from the root.
There was a problem hiding this comment.
Ah, I think I finally understood what you are trying to do here. Please refer to #765 (comment) for a conversation about this.
There was a problem hiding this comment.
Yep, the gist is instead of reusing global view models for route functionality, use global view models as proxy to move just the dependencies required for initializing the route (and it's respective viewmodel).
| import androidx.compose.animation.slideInHorizontally | ||
| import androidx.compose.animation.slideOutHorizontally | ||
|
|
||
| val enterTransition = slideInHorizontally { it } |
There was a problem hiding this comment.
The PR seems related to scooping the ViewModels per NavBackStackEntry. Isn't it better to do other improvements (such as animations) in a follow-up PR to make it easier to review?
There was a problem hiding this comment.
Animation was a few lines, so included it here. Agree this should have been a different PR.
| abstract class NavControllerWrapper { | ||
| protected abstract val navController: NavController | ||
|
|
||
| fun canPop() = navController.previousBackStackEntry != null |
There was a problem hiding this comment.
Why not make it as an extension function for NavController?
There was a problem hiding this comment.
So initially, I had a Flow in there and the back button would listen to the nav stack changes and show the appropriate icon. I kinda later realized I could just remember that value.
Also, none of the routes get the navController as is. They will be wrapped and hence this makes it easier to pass navigation stuff that's common to all the routes (like canPop here).
| // https://stackoverflow.com/a/69533584/13390651 | ||
| @OptIn(ExperimentalLayoutApi::class) | ||
| @Composable | ||
| fun PaddingValues.bottomIfKeyboardNotOpen(): State<Dp> { |
There was a problem hiding this comment.
Why an extension function that handles bottom paddings are placed inside a NavControllerWrapper file? That looks a little lost.
There was a problem hiding this comment.
Will move this to a different file.
There was a problem hiding this comment.
One irritating thing about this is that IsImeVisible has an initial value of true which add some padding for a few milliseconds when the app is opened.
| private val navigateToDestination: () -> Unit, | ||
| ) { | ||
| fun navigate(dependencies: D) { | ||
| container.dependencies = dependencies |
There was a problem hiding this comment.
I have concerns with the design here. If a Process Death happens, Navigation Component will recover the navigation state but your "dependencies" will be lost. That may cause an inconsistent state and/or crashes.
Did I miss something? I'm struggling to understand the intention here, so it is likely that I'm missing something.
There was a problem hiding this comment.
Honestly, did not think about process death when implementing this.
The intention:
Take a look at the PostEditViewModel in the main branch:
https://github.com/dessalines/jerboa/blob/30aa860bf40e2e7b522e9abb590ae41223085cd6/app/src/main/java/com/jerboa/ui/components/post/edit/PostEditViewModel.kt#L53-L59
Since the post can be edited from the home (or feed) activity, post activity, community activity & a person's profile activity, all these view models have to be passed to the PostEditViewModel to update the data. Usually after editing a post, you go back to any one of the these activities and it's good UX to show the updated data (for confirmation etc). Also the update logic should ideally not be in the post edit view model.
My intention was to invert this:
With this PR one can pass a callback for post edit and any activity can update itself accordingly.
In home/feed activity:
https://github.com/nahwneeth/jerboa/blob/bcefa3a67e8d81ac4398b70b2afa393dafa04028/app/src/main/java/com/jerboa/ui/components/home/FeedActivity.kt#L320-L327
onEditPostClick = { postView ->
navController.toPostEdit.navigate(
PostEditDependencies(
postView = postView,
onPostEdit = homeViewModel::updatePost,
),
)
},In post activity:
https://github.com/nahwneeth/jerboa/blob/bcefa3a67e8d81ac4398b70b2afa393dafa04028/app/src/main/java/com/jerboa/ui/components/post/PostActivity.kt#L254-L261
onEditPostClick = { pv ->
navController.toPostEdit.navigate(
PostEditDependencies(
postView = pv,
onPostEdit = postViewModel::updatePost,
),
)
}
Person profile activity:
https://github.com/nahwneeth/jerboa/blob/bcefa3a67e8d81ac4398b70b2afa393dafa04028/app/src/main/java/com/jerboa/ui/components/person/PersonProfileActivity.kt#L381-L388
onEditPostClick = { pv ->
navController.toPostEdit.navigate(
PostEditDependencies(
postView = pv,
onPostEdit = personProfileViewModel::updatePost,
),
)
},
I hope u get the idea. I tried a few things and I felt this was the best way to pass dependencies/callbacks. Of course, there might be a better way to do this which I'm not aware of. Open to suggestions and ideas.
The second intention is to scope the view models to the routes:
Right now these view models are declared globally and they are reused. This has resulted in stale data and the workaround currently being used to reload the view models when navigating back which is leading to other issues like the data getting reset, scroll positions not being retained and filters getting reset. These global view models also, if i'm not wrong, will not survive process death.
Edit:
Also the home activity in the main branch does this before navigating:
https://github.com/dessalines/jerboa/blob/30aa860bf40e2e7b522e9abb590ae41223085cd6/app/src/main/java/com/jerboa/ui/components/home/HomeActivity.kt#L322-L325
One might forget to initialize the postEditViewModel when navigating to the post edit activity. With the currently PR you get a compile time error. So, let's say the dependencies changed later, the error shows up in all the places where this might fail. This is also why I went with the dedicated nav controller wrapper for each route.
Look at the dependencies and one will know what a route requires to function properly. Look at the nav controller wrapper and one will know what all navigations are possible from that route.
| import com.jerboa.ui.components.common.MarkdownTextField | ||
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun CommentEditHeader( | ||
| navController: NavController = rememberNavController(), | ||
| navController: NavControllerWrapper, |
There was a problem hiding this comment.
Instead of a wrapper, why not pass a function reference to navigate: () -> Unit, navigate: (arg1) -> Unit, canPop: () -> Unit, etc? That removes the need for the wrapper all together.
There was a problem hiding this comment.
For the header i agree that we could have passed a closure. But this is just a personal preference: either pass multiple closure or pass an object will all the closure/functions inside it.
| import kotlinx.coroutines.CoroutineScope | ||
|
|
||
| @Composable | ||
| fun InitializeRoute(initializationBlock: suspend CoroutineScope.() -> Unit) { |
There was a problem hiding this comment.
I have concerns about the initializeRoute function. If a Process Death happens, initializationBlock won't be re-called because you used rememberSaveable for the isInitialized: Boolean but the ViewModel will be recreated when the navigation graph is restored. It seems that could place the ViewModel in an inconsistent state and/or cause crashes.
There was a problem hiding this comment.
Did not have process death in mind while implementing this. Looks like the isIntialized variable is better stored in the viewModel itself since that is what is currently focusing on here.
But again the dependencies will not be stored which is required for initializing the route. In fact any data in the view model is vulnerable unless we store it in the the saveStateHandle or database ig. However, callbacks cannot be stored anywhere.
One approach we could take is to check if the dependencies exist in the dependency container and redirect to home otherwise. We could prevent crashes due to inconsitent state this way and the UX would also be good. The data in acitivities like CreatePostActivity can be persisted and reloaded when the user revisits this activity from the home activity.
What do u think?
|
I looked into the feasibility of persisting view models. I've opened an issue #810 to make the autogen types parcelable or serializable. For now, I'll try serializing the types manually with Gson. Moving this to draft. |
Continuing work done in #670
Scoped view models
Closes #704
Closes #402
Closes #774
Declarative navigation
navController.toCommentReply.navigate( CommentReplyDependencies( ReplyItem.PostItem(pv), isModerator = isModerator( pv.creator, postRes.data.moderators, ), onCommentReply = postViewModel::appendComment, ), )Bottom navigation bar
Closes #714
Closes #722 (Closes #719 similar)
Partially addresses #346
Screen transitions
Closes #300
Other changes
HomeActivitydon't have a back button.Closes #789
I've tried not to modify any logic other than navigation. Let me know if anything is not working as expected.