firestore-android-arch-components icon indicating copy to clipboard operation
firestore-android-arch-components copied to clipboard

ViewModel does not persist data through lifecycle changes as expected.

Open githubmonkey opened this issue 8 years ago • 5 comments

Thanks for publishing this sample. I learned a lot from studying your code. However, I am puzzled about one detail.

One of the biggest benefits of using a viewmodel is that model data survives destruction and recreation of the activity during configuration changes. This makes it easier to restore for example the position of the list. Scroll down a long restaurant list, rotate the phone, and watch the list being recreated at the same position.

Currently, your restaurant list does not respect the previous scroll position on rotation. I've added onSaveInstanceState and onRestoreInstanceState to capture the state of the layout manager during rotation. (See code snippet below.) However, this alone is not enough since the adapter is still empty at the time onRestoreInstanceState is called.

The adapter is only populated later through restaurant.observe which was set up in MainActivity.onCreate. This observer is triggered only when the repository has retrieved a new copy of the (unchanged) restaurant list from the firebase query.

IMHO this defeats the purpose of the ViewModel class. This layer is supposed to maintain a link to the data that survives rotation changes etc. By delegating data handling to Firebase the data is recreated after each lifecycle change.

Any idea how to make the restaurant data reference persist through the life time of the viewmodel?

Cheers!

P.S.: Here is the code snipped I tried to capture the layout manager state

@Override
protected void onSaveInstanceState(Bundle outState) {
    super.onSaveInstanceState(outState);

    if (binding.recycler != null) {
        RecyclerView.LayoutManager manager = binding.recycler.getLayoutManager();
        if (manager != null) {
            Parcelable parcel = manager.onSaveInstanceState();
            outState.putParcelable(BUNDLE_LAYOUT, parcel);
        }
    }
}

@Override
protected void onRestoreInstanceState(Bundle savedInstanceState) {
    super.onRestoreInstanceState(savedInstanceState);

    if (savedInstanceState != null) {
        // PROBLEM!! The adapter is not re-populated yet so the layout state is meaningless.
        Log.d("MainActivity", "current adapter size " + binding.recycler.getAdapter().getItemCount());
        
        RecyclerView.LayoutManager manager = binding.recycler.getLayoutManager();
        if (manager != null) {
            Parcelable savedRecyclerLayoutState = savedInstanceState.getParcelable(BUNDLE_LAYOUT);

            manager.onRestoreInstanceState(savedRecyclerLayoutState);
        }
    }
}

githubmonkey avatar Apr 07 '18 17:04 githubmonkey

You are very welcome. But I guess i stumbled upon the same problem here, and that solved it.

Waiting your feedback....

amrro avatar Apr 08 '18 10:04 amrro

Your solution is almost identical to what I did. However, it's not working in this case as the adapter (and with that the LayoutManager) does not have any data yet at the time onRestoreInstanceState() is called.

In fact, I found out after I compiled the initial comment that the problem runs deeper. ANY TIME the data changes on the server (let's say somebody else added a restaurant review), the firestore listener will trigger the adapter data to be replaced and the list view will automatically pop back to the top. Pretty irritating if you are the person just inspecting something at the bottom of the list.

I believe the only viable solution is to disallow firestore to trigger adapter updates and to let the viewmodel cash the firebase data and manage changes.

What do you think?

githubmonkey avatar Apr 08 '18 17:04 githubmonkey

any update on this issue? and how would you implement cashing firebase data and manage changes as you have pointed ?

Thanks...

thebatu avatar Nov 28 '18 14:11 thebatu

Hey @thebatu,

Currently, I am working on updating the project to Kotlin. I am using this pattern and I have never faced this issue; however, I will test that over the weekend and let you know.

amrro avatar Nov 28 '18 17:11 amrro

@amrro thanks.

thebatu avatar Nov 29 '18 09:11 thebatu