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

Use SavedStateHandle in view models #4875

Merged
merged 18 commits into from
Oct 22, 2024

Conversation

Isira-Seneviratne
Copy link
Contributor

@Isira-Seneviratne Isira-Seneviratne commented Aug 19, 2024

Pass SavedStateHandle instead of Bundle to the view models. Values provided via an Intent's extras or a fragment's arguments are automatically passed to the state handle in the default view model factory implementation, which eliminates the need to implement a view model factory separately.

Copy link
Member

@dbrant dbrant left a comment

Choose a reason for hiding this comment

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

Thanks -- this looks like it will be a worthwhile change. However, a change of this size requires a much better description.
The word "simplify" is not meaningful enough. There is a considerable reduction in code, but why is there a reduction in code?

Is it because SavedStateHandle automatically provides access to the Activity's intent extras and/or the Fragment's arguments? If so, this should be very clearly specified in your description. I'm not even seeing this described in Google's own documentation of SavedStateHandle, which worries me a bit. Can you point to where in the documentation does it endorse using SavedStateHandle in this way?

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Aug 19, 2024

Thanks -- this looks like it will be a worthwhile change. However, a change of this size requires a much better description. The word "simplify" is not meaningful enough. There is a considerable reduction in code, but why is there a reduction in code?

I updated the description to be more informative, hope that's better.

Is it because SavedStateHandle automatically provides access to the Activity's intent extras and/or the Fragment's arguments? If so, this should be very clearly specified in your description.

That's exactly what it does. The default view model factory automatically forwards data passed via the extras or arguments:

https://github.com/androidx/androidx/blob/androidx-main/activity/activity/src/main/java/androidx/activity/ComponentActivity.kt#L547
https://github.com/androidx/androidx/blob/976ab1f419ea26d4f7481267ffba338fb9b256ff/fragment/fragment/src/main/java/androidx/fragment/app/Fragment.java#L471

Copy link

@machado001 machado001 left a comment

Choose a reason for hiding this comment

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

Minor comments =)

import androidx.paging.PagingSource
import androidx.paging.PagingState
import androidx.paging.cachedIn
import androidx.paging.*
Copy link

@machado001 machado001 Aug 19, 2024

Choose a reason for hiding this comment

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

Just curious, what's the reason for this wildcard? I've seen it adopted in another file, TemplatesSearchViewModel, too.

Isira-Seneviratne and others added 7 commits September 8, 2024 06:17
# Conflicts:
#	app/src/main/java/org/wikipedia/page/linkpreview/LinkPreviewViewModel.kt
#	app/src/main/java/org/wikipedia/watchlist/WatchlistExpiryDialogViewModel.kt
# Conflicts:
#	app/src/main/java/org/wikipedia/feed/onthisday/OnThisDayViewModel.kt
@trietbui85
Copy link
Contributor

This PR gets bigger and bigger. Should we separate it to multiple smaller PRs?

Isira-Seneviratne and others added 3 commits October 17, 2024 06:18
# Conflicts:
#	app/src/main/java/org/wikipedia/talk/TalkReplyViewModel.kt
# Conflicts:
#	app/src/main/java/org/wikipedia/descriptions/DescriptionEditFragment.kt
#	app/src/main/java/org/wikipedia/descriptions/DescriptionEditViewModel.kt
@dbrant dbrant merged commit ddf3c07 into wikimedia:main Oct 22, 2024
1 check passed
@Isira-Seneviratne Isira-Seneviratne deleted the SavedStateHandle branch October 22, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants