airbnb / mavericks

Mavericks: Android on Autopilot
https://airbnb.io/mavericks/
Apache License 2.0
5.83k stars 500 forks source link

Resetting MvRx args when using activityViewModel #391

Closed roncbird closed 4 years ago

roncbird commented 4 years ago

I'm working on a feature that includes a history screen that displays a list of past check deposits. When the user clicks on one of the list items, it takes them to a detailed screen. We pass the transaction id to the details screen using MvRx args, and use the transaction id to make a network call to get the details for that specific transaction. The details screen uses an activityViewModel since one of the buttons on the details screen will take the user to a new screen that displays the check images (front, and back). Instead of having to create another ViewModel and state class for the check images screen, we use activityViewModel so check images screen can use the same ViewModel as the details screen, which already contains the check images urls.

Everything works fine for the first time the user clicks on a check deposit history list item, but if they click back to the history screen, and click on another item, the details screen shows the same details for the previous check the user clicked on, and not the new list item they clicked on. This is because, when an activityViewModel is used, the state is not getting recreated each time a new item is clicked on, so the new transaction id passed to the details screens state using MvRx args does not get updated, and the old value is used, until the activityViewModel is completely killed.

Here are some code snippets to help give a little more context:

MvRx args setup in nav graph

 <fragment
            android:id="@+id/checkDepositDetailsFragment"
            android:name="com.varomoney.bank.presentation.main.checkdeposit.depositdetails.CheckDepositDetailsFragment"
            android:label="@string/check_deposit_details">

            <argument
                android:name="mvrx:arg"
                app:argType="string"
                app:nullable="false" />

            <action
                android:id="@+id/action_checkDepositDetailsFragment_to_checkImagesFragment"
                app:destination="@+id/checkImagesFragment" />

            <action
                android:id="@+id/action_checkDepositDetailsFragment_to_legalFragment"
                app:destination="@+id/legalFragment" />

        </fragment>

How the state is getting populated

data class CheckDepositDetailsState(
    val depositId: String,
    val depositDetails: Async<UseCaseResponse<Rdc>> = Uninitialized
) : MvRxState {
    constructor(id: String) : this(depositId = id)
}

onClick logic in the CheckDepositHisotryFragment

 onClick { _ ->
                findNavController().navigate(
                    CheckDepositHistoryFragmentDirections.actionCheckDepositHistoryToCheckDepositDetailsFragment(item.id)
                )
            }

Is there a way, when using an activityViewModel to ensure the state replaces the old value with the new MvRx args value in the scenario above?

gpeal commented 4 years ago

You can make the photo fragment a child of the details fragment and use by parentFragmentViewModel()

roncbird commented 4 years ago

Thank you for the quick reply. I appreciate it. So I apologize if I didn't explain it very well, but the problem actually exists between the CheckDepositHistoryFragment and CheckDepositDetailsFragment, not the CheckDepositDetailsFragment, and the CheckImagesFragment.

Let me see if explaining it this way gives a little bit better clarity. The CheckDepositDetailsFragment, and the CheckImagesFragment use the same ViewModel CheckDepositDetailsViewModel, which I'm making an activityViewModel, so that CheckImagesFragment can use the url values stored in CheckDepositDetailsState. This works fine. The issue I'm running into is when I go back from CheckDepositDetailsFragment to CheckDepositHistoryFragment and select another check deposit item. Instead of the new item being shown, the previous item that was selected is shown. This is because the new MvRx args item.id that is passed in through the onClick logic below in the CheckDepositHistoryFragment:

 onClick { _ ->
                findNavController().navigate(
                    CheckDepositHistoryFragmentDirections.actionCheckDepositHistoryToCheckDepositDetailsFragment(item.id)
                )
            }

does not update the state, since CheckDepositDetailsState was already instantiated, and depositId was already assigned a value from the previous check deposit item that was clicked on. Since depositId gets assigned a value like this:

data class CheckDepositDetailsState(
    val depositId: String,
    val depositDetails: Async<UseCaseResponse<Rdc>> = Uninitialized
) : MvRxState {
    constructor(id: String) : this(depositId = id)
}

when CheckDepositDetailsState is instantiated, it won't be assigned a different value until the activity is destroyed, since using activityViewModel holds onto the state and ViewModel as long as it is alive.

Hopefully that made a little more sense. Let me know if you'd like me to share a video of the flow to help give a little more context.

If I was using a fragmentViewModel the state and ViewModel would be created each time the fragment got created, so this wouldn't be an issue, but I'd rather use an activityViewModel which CheckDepositDetailsFragment and CheckImagesFragment both use, instead of having to pass the image urls through fragment args.

Basically, I'm just wondering if there is a way to force the state to be updated with a new MvRx args value that is passed in using Android Navigation logic, after the state has already been instantiated, and while using an activityViewModel?

gpeal commented 4 years ago

@roncbird I think I understand the issue and my comment would solve it. By having your ViewModel be scoped to the details ViewModel and making the images fragment a child of the details fragment and using parentFragmentViewModel(), the details and images fragment would share a ViewModel and the view model would have the lifecycle of details which would mean it would get destroyed when returning to the deposit details fragment

roncbird commented 4 years ago

@gpeal ok, thank you for the suggestion. I'll give it a try, and will let you know if that solves it.

roncbird commented 4 years ago

So, we are using Android Navigation Components, and I was unable to figure out how to make CheckImagesFragment a child of CheckDepositDetailsFragment with Android Navigation, so what I'm probably going to do is just give CheckImagesFragment it's own ViewModel and state class, and use another mvrx:arg to pass CheckImagesFragment what it needs.

samjackrabbit commented 3 years ago

hey @roncbird we are trying to pass mvrx args like you did in your example, but they never come through. Was there any special setup you did so the state object could grab those nav args.

We have it working outside the nav graph, just not when using navigation with action from nav graph.

roncbird commented 3 years ago

@samjackrabbit sorry for the delayed reply. I just noticed your question in my sea of emails in my inbox.

To answer your question, we are not really doing any special in setup other than what is required per MvRx documentation to get the mvrx args to work. Here is how we basically initialize everything.

CheckDepositDetailsFragment

class CheckDepositDetailsFragment : EpoxyBaseFragment() {

    val viewModel: CheckDepositDetailsViewModel by fragmentViewModel()

CheckDepositDetailsViewModel

class CheckDepositDetailsViewModel(
    initialState: CheckDepositDetailsState
) : BaseMvRxViewModel<CheckDepositDetailsState>(initialState) {

    companion object :
        MavericksViewModelFactory<CheckDepositDetailsViewModel, CheckDepositDetailsState> {

        override fun create(
            viewModelContext: ViewModelContext,
            state: CheckDepositDetailsState
        ): CheckDepositDetailsViewModel? {
            return CheckDepositDetailsViewModel(
                state,
            )
        }
    }
}

CheckDepositDetailsState

data class CheckDepositDetailsState(
    val id: String,
) : MvRxState {
    constructor(rdcId: String) : this(id = rdcId)
}

Hopefully, that helps. Let me know if you have any other questions.

sanginovs commented 2 years ago

@roncbird here's a sample code on how to setup parent child fragment in Mvrx: https://github.com/airbnb/mavericks/tree/master/sample/src/main/java/com/airbnb/mvrx/sample/features/parentfragment