Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor initiazable, Community navhost navigation and fix rare crash in inbox #1210

Merged
merged 11 commits into from
Sep 7, 2023

Conversation

MV-GH
Copy link
Collaborator

@MV-GH MV-GH commented Sep 1, 2023

  • Removes initiaziable and replaced it with viewmodel constructor, this is more safe
  • Remove Community root and complex viewmodel sharing. This simpliflies the logic and prevents the need for a "redirect" hack to make deeplinks work for communities
  • Add logic to make forward passing of Parcelable possible (sendReturn, getPrevReturn)
  • Replace rootChannel and takeDepsFromRoot
  • Fixes for rare crash in inbox

Fixes #1208
Fixes #1211

@nahwneeth If interested

Just realized that the "forward passing" that I made, is just a reinvention of the rootChannel and takeDepsFromRoot

Hmm not sure which one is better atm

Edit:
I have chosen for my "reinvention" naming is still open for debate. The reason I took this over existing rootChannel is:

  • sender is now not a composable which gives it more freedom,
  • it is cleaner, the function itself, and the useability,
  • It should survive process death now
  • Should be leaner, as it does not use a ViewModel as intermediate storage

I tested everything change by change

@MV-GH MV-GH marked this pull request as draft September 1, 2023 22:38
@MV-GH MV-GH marked this pull request as ready for review September 2, 2023 03:33
@MV-GH MV-GH changed the title Refactor initiazable and Community navhost navigation Refactor initiazable, Community navhost navigationand fix rare crash in inbox Sep 2, 2023
@MV-GH MV-GH changed the title Refactor initiazable, Community navhost navigationand fix rare crash in inbox Refactor initiazable, Community navhost navigation and fix rare crash in inbox Sep 2, 2023
@MV-GH MV-GH marked this pull request as draft September 2, 2023 15:53
@MV-GH MV-GH marked this pull request as ready for review September 2, 2023 15:58
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few concerns about moving the initialized data completely out of the viewmodels, but I mostly blame the jetpack-compose tutorials for not having good examples for how to how to handle intialized data in conjunction with routing.

At some point we might have to entertain moving nearly all the variables out of the viewmodels (and possibly into a stored app state), and have them only consist of functions. When I initially wrote jerboa, the tutorials I'd followed recommended either one, but then most of their examples had data live in the viewmodels.

I trust you and nahweenth's additions for the correct way to do this. Once you get the conflicts resolved, feel free to merge anytime.

InitializeRoute(commentEditViewModel) {
commentEditViewModel.initialize(commentView)
}
val commentView = appState.getPrevReturn<CommentView>(key = CommentEditReturn.COMMENT_SEND)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the heart of it here, just an alternate way to send initial data, and remove it from the model.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even before Jetpack compose passing data between activities was never really done cleanly.

Take a look at IfR does, it uses intents to pass data between activities and Preferences API for persistence

https://github.com/Docile-Alligator/Infinity-For-Reddit/blob/02278263579d76f88eef9c643bd7ace412f99985/app/src/main/java/ml/docilealligator/infinityforreddit/activities/SearchActivity.java#L65-L75

Yeah basically. The big difference here, compared to consumeReturn, is that it never removes the data from the route, it only gets removed when the screen gets popped from backstack.

MV-GH and others added 2 commits September 5, 2023 00:48
# Conflicts:
#	app/src/main/java/com/jerboa/Utils.kt
#	app/src/main/java/com/jerboa/model/InboxViewModel.kt
#	app/src/main/java/com/jerboa/ui/components/community/Community.kt
#	app/src/main/java/com/jerboa/ui/components/inbox/InboxActivity.kt
#	app/src/main/java/com/jerboa/ui/components/person/PersonProfileActivity.kt
@dessalines dessalines enabled auto-merge (squash) September 7, 2023 13:28
@dessalines dessalines merged commit 85165d8 into LemmyNet:main Sep 7, 2023
1 check was pending
@MV-GH MV-GH deleted the refactor/initiable branch September 7, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants