airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.5k stars 733 forks source link

Nested Carousels lose state when scrolling down main recycler view #561

Closed tzaitoun closed 5 years ago

tzaitoun commented 5 years ago

Setup: A main recycler view with nested carousels.

Problem: When I scroll down the main recycler view, the nested carousel that goes off the screen loses its state. What could be causing this?

Also when navigating to another activity and going back, the state of the main recycler view is lost but the nested carousels' views are persisted?

elihart commented 5 years ago

Are you using the Carousel class directly from the Epoxy library? The main thing I would suspect is the model/view is not set up to save state correctly.

Otherwise it's possible that models are not set on the carousel correctly so they lose the scroll position.

I can't say more without seeing the code or more details. Have you looked at the example in the sample app?

tzaitoun commented 5 years ago

Yes, I'm using the Carousel directly from the library. Is it supposed to lose its state after being scrolled off the screen, do I have to do something extra to keep the state?

I did look at the sample app but to fix another problem which I was having previously: saving state across screen rotation and fragment view destruction, which works fine now.

How could the models not be set on the carousel properly? Anyways I'll look through the sample app again to see if I missed something.

tzaitoun commented 5 years ago

I made this shorter on purpose, so I just loop to make 10 carousels with the same data.

public class MovieController extends TypedEpoxyController<ArrayList<MovieListResult>> {

    private Context mContext;

    public MovieController(Context context) {
        mContext = context;
    }

    @Override
    protected void buildModels(ArrayList<MovieListResult> data) {

        for (int i = 0; i < 10; i++) {
            new CarouselModel_().id(i).models(buildMovieList(i, data.get(0).getMovieList())).addTo(this);
        }
    }

    /* This creates a MovieCetegoryListModel for each movie in the nested RecyclerView, this model
     * represents the view of each movie.
     */
    private ArrayList<MovieCategoryListModel_> buildMovieList(long id, ArrayList<Movie> movies) {

        ArrayList<MovieCategoryListModel_> movieCategory = new ArrayList<>();
        for (final Movie movie : movies) {
            movieCategory.add(
                new MovieCategoryListModel_()
                    .id(movie.getmId(), id)
                    .mMoviePoster(movie.getPosterPath())
                    .mMovieTitle(movie.getTitle())
                    .onClickListener(new OnModelClickListener<MovieCategoryListModel_, MovieCategoryListModel.ViewHolder>() {
                        @Override
                        public void onClick(MovieCategoryListModel_ model, MovieCategoryListModel.ViewHolder parentView, View clickedView, int position) {
                            Intent intent = new Intent(mContext, MovieDetailActivity.class);
                            intent.putExtra(MOVIE_ID_INTENT, movie.getmId());
                            mContext.startActivity(intent);
                        }
                    })
            );
        }

        return movieCategory;
    }
}
@EpoxyModelClass(layout = R.layout.viewholder_movie_category_list)
public abstract class MovieCategoryListModel extends EpoxyModelWithHolder<MovieCategoryListModel.ViewHolder> {

    @EpoxyAttribute String mMoviePoster;
    @EpoxyAttribute String mMovieTitle;
    @EpoxyAttribute View.OnClickListener onClickListener;

    @Override
    public void bind(@NonNull ViewHolder holder) {
        super.bind(holder);
        String url = "https://image.tmdb.org/t/p/w500";
        Picasso.get().load(url + mMoviePoster).into(holder.mPosterImageView);
        holder.mTitleTextView.setText(mMovieTitle);
        holder.mLinearLayout.setOnClickListener(onClickListener);
    }

    @Override
    public boolean shouldSaveViewState() {
        return true;
    }

    public static class ViewHolder extends EpoxyHolder {

        private LinearLayout mLinearLayout;
        private ImageView mPosterImageView;
        private TextView mTitleTextView;

        @Override
        protected void bindView(@NonNull View itemView) {
            mLinearLayout = itemView.findViewById(R.id.linear_layout_movie_viewholder);
            mPosterImageView = itemView.findViewById(R.id.iv_movie_poster);
            mTitleTextView = itemView.findViewById(R.id.tv_movie_title);
        }
    }
}

This is the layout for the fragment, I just define the top level (main) RecyclerView.

<?xml version="1.0" encoding="utf-8"?>
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    tools:context=".view.MovieListsFragment">

    <com.airbnb.epoxy.EpoxyRecyclerView
        android:id="@+id/rv_movie_lists"
        android:layout_width="match_parent"
        android:layout_height="match_parent"
        android:paddingTop="8dp">
    </com.airbnb.epoxy.EpoxyRecyclerView>

</FrameLayout>
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout
    xmlns:android="http://schemas.android.com/apk/res/android"
    android:id="@+id/linear_layout_movie_viewholder"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:orientation="vertical"
    android:layout_marginBottom="@dimen/movie_list_spacing"
    android:background="?android:attr/selectableItemBackground"
    android:clickable="true"
    android:focusable="true">

    <ImageView
        android:id="@+id/iv_movie_poster"
        android:layout_width="100dp"
        android:layout_height="140dp"
        android:scaleType="fitXY"/>

    <TextView
        android:id="@+id/tv_movie_title"
        android:layout_width="100dp"
        android:layout_height="wrap_content"
        android:maxLines="2"
        android:ellipsize="end"
        android:textColor="@color/mainTextColor"/>

</LinearLayout>

These are all the relevant files.

I looked at the sample app and did not notice any differences in how we do things except the sample app is more complex and uses a ModelGroup. Maybe I'm using ids wrong, if something wrong catches your eye, please let me know, I'm not really sure how to debug this now.

elihart commented 5 years ago

Your code looks right, I'm not sure what is going on. Make sure the carousel models definitely have unique ids.

Also, can you put breakpoints in BaseEpoxyAdapter - onBindViewHolder and onViewRecycled, and look at usage of viewHolderState? make sure state is being saved and restored for the models.

Also look at EpoxyRecyclerView#setModels. Make sure that a new controller is created and set each time the view is bound, make sure that models in the controller are added synchronously. It is important that models are built and added to the adapter before the saved scroll position is applied.

Did you enable async diffing by any chance?

tzaitoun commented 5 years ago

No I didn't enable async diffing. I tested it like you said and everything seems normal, except the state of the carousel is not saved, but only its children. The carousel being the actual carousel state and the children being the state of the viewholders in that carousel. The only carousel state that is saved is the first one. I tested this by scrolling down to recycle it and went back up and state remained the same.

I mean that for the other carousels onViewRecycled doesn't get called for them at all, it only gets called for the first one. But onViewRecycled does get called for their children views.

It's very weird behavior. Is there a limit on how many carousels can be saved?

milad-mr commented 5 years ago

I have same problem . my carousels position lost when fragment view is destroyed or some time when I scroll. I have. a recycle view in home fragment and multiple carousels exactly like google play store. I tried putting carousels in home recycler view build method or putting it on other EpoxyModel(Category Model) but it doesn't help.

tzaitoun commented 5 years ago

@milad-mr its supposed to lose its position when the fragment is destroyed or its view is destroyed. You need to save the state before the view gets destroyed and restore it after the view is created again. As for losing its state when scrolling of the screen, I am not sure, I have the same problem and maybe my setup is wrong.

milad-mr commented 5 years ago

@tzaitoun does saving carousel state work fine for you? I have 1 home fragment which has several carsouls models the position of main controller is saved but carsouls state lose. can u give me an example of how to use save state? the point is my fragment doesn't destroyed when I switch the fragment(with nav bar) so fragment's onSaveInstanceState doesn't called every time .

tzaitoun commented 5 years ago

@milad-mr I have a very similar setup. onSaveInstanceState only gets called when the fragment's hosting activity is destroyed (e.g. screen rotation). But when you switch fragments with the nav bar, the fragment or only its view is destroyed (depending on how you did it).

To solve this, I push one instance of each fragment on the back stack (I use FragmentManager.findFragmentByTag) so only its view is destroyed and its member variables remain intact.

I create a bundle as a member variable and use it to save the state of the controller in onStop and restore in onActivityCreated. Here's the fragment lifecycle: https://developer.android.com/guide/components/fragments so you know why I did it this way.

milad-mr commented 5 years ago

@tzaitoun thank you. Your trick worked for me but some time carousels lose state(specially when switching between fragments take lot of time . But they lose state when they went out of screen and I scroll other carasouls . but this one also happened by chance.

I think carousel may has some bugs (but I'm not sure) because it's in beta phase. @elihart @tzaitoun Can we use horizontal Epoxy recycler view instead of it? what's your suggestion.

tzaitoun commented 5 years ago

Hi Eli, I still can't figure it out, the only missing code that I haven't posted here is the fragment, if you have time please take a look:

public class MovieListsFragment extends Fragment {

    public static final String MOVIE_LISTS_KEY = "com.zaitoun.talat.moviecentral.movie_key";

    private CompositeDisposable mDisposables;
    private ArrayList<MovieListResult> mMovieLists;
    private MovieListResult mMovieListResult;
    private EpoxyRecyclerView mEpoxyRecyclerView;
    private MovieController mController;

    private Bundle mSavedStateBackStack;

    public MovieListsFragment() {
        mDisposables = new CompositeDisposable();
        mMovieLists = new ArrayList<>();
    }

    public static MovieListsFragment newInstance() {
        return new MovieListsFragment();
    }

    @Override
    public void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
    }

    @Override
    public void onActivityCreated(@Nullable Bundle savedInstanceState) {
        super.onActivityCreated(savedInstanceState);

        if (savedInstanceState != null) {
            mMovieLists = savedInstanceState.getParcelableArrayList(MOVIE_LISTS_KEY);
            mController.onRestoreInstanceState(savedInstanceState);
        }

        if (mSavedStateBackStack != null) {
            mController.onRestoreInstanceState(mSavedStateBackStack);
        }
    }

    @Nullable
    @Override
    public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup container, @Nullable Bundle savedInstanceState) {

        View view = inflater.inflate(R.layout.fragment_movie_lists, container, false);
        mEpoxyRecyclerView = (EpoxyRecyclerView) view.findViewById(R.id.rv_movie_lists);
        mController = new MovieController(getContext());

        return view;
    }

    @Override
    public void onResume() {
        super.onResume();

        // Get the TMDB api interface, gives us access to all defined CRUD operations for the api
        TmdbApiInterface service = TmdbApiClientInstance.getRetrofitInstance().create(TmdbApiInterface.class);

        SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(getContext());
        String accountId = preferences.getString(getString(R.string.user_id), null);
        String sessionId = preferences.getString(getString(R.string.user_session), null);

        if (mMovieLists.size() == 0) {
            /* The concat operator is used here to make 3 independent sequential api calls, these calls
             * will always be made in this order.
             */
            Observable.concat(service.getPopularMovies(getString(R.string.tmdb_api_key), "US"),
                    service.getNowPlayingMovies(getString(R.string.tmdb_api_key), "US"),
                    service.getUpcomingMovies(getString(R.string.tmdb_api_key), "US"),
                    service.getUserWatchlist(accountId, getString(R.string.tmdb_api_key), sessionId))
                    .subscribeOn(Schedulers.io())
                    .observeOn(AndroidSchedulers.mainThread())
                    .subscribe(new Observer<MovieListResult>() {
                        @Override
                        public void onSubscribe(Disposable d) {
                            // Used to dispose the connections when the fragment's view is destroyed
                            mDisposables.add(d);
                        }

                        @Override
                        public void onNext(MovieListResult movieListResult) {
                            // When an api result is returned, add it the array list
                            mMovieLists.add(movieListResult);
                        }

                        @Override
                        public void onError(Throwable e) {
                            // All unsuccessful responses arrive here, so handle errors
                        }

                        @Override
                        public void onComplete() {
                            // When all the data arrives, set up the recycler view and pass the data
                            mController.setData(mMovieLists);
                            mEpoxyRecyclerView.setController(mController);
                        }
                    });
        }
    }

    @Override
    public void onSaveInstanceState(@NonNull Bundle outState) {
        super.onSaveInstanceState(outState);
            outState.putParcelableArrayList(MOVIE_LISTS_KEY, mMovieLists);
            mController.onSaveInstanceState(outState);
    }

    @Override
    public void onStop() {
        super.onStop();
        mSavedStateBackStack = new Bundle();
        mController.onSaveInstanceState(mSavedStateBackStack);
    }

    @Override
    public void onDestroyView() {
        super.onDestroyView();
        mDisposables.clear();
    }
}
monikasaini856 commented 5 years ago

Facing same issue. Any solution for this ?

reavcn commented 5 years ago

+1 for us, using 2.12.0 (will update asap)

bagrusss commented 5 years ago

Hi! Do you have progress for this issue? I have the same problem with fragment and activity. I create an example that demonstrates this issue. It is necessary to enable switch "Don't keep Activities" in the developers options. I noticed that method onSaveInstanceState() of RecyclerView was not called before method onDestroy() of activity/fragment, but onSaveInstanceState() of RecyclerView had been called ofter onCreate()/onViewCreated() and only then onRestoreInstanceState() had been called.

elihart commented 5 years ago

@tzaitoun I believe your issue is that you are not setting the Epoxy controller on the recyclerview until after view state is restored.

@Override
                    public void onComplete() {
                        // When all the data arrives, set up the recycler view and pass the data
                        mController.setData(mMovieLists);
                        mEpoxyRecyclerView.setController(mController);
                    }

Models must be built and added to the RecyclerView before view state is restored

@bagrusss in your example code you are not calling onSaveInstanceState and onRestoreInstance on the Epoxy controller from your MainActivity

I cannot reproduce any issues with restoring state, in either the main vertical recyclerview or in nested Carousels, whether when scrolling vertically or when the activity is recreated

Unless somebody can point out a specific piece of code that has a bug in it, or a valid sample project is provided, I'm going to assume that you're using Epoxy incorrectly.

See the wiki for instructions for how to properly save and restore state

bagrusss commented 5 years ago

@bagrusss in your example code you are not calling onSaveInstanceState and onRestoreInstance on the Epoxy controller from your MainActivity

@elihart with onSaveInstanceState() and onRestoreInstance() it works! Thanks!