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

Scroll position is lost if updateAsync is used when populating adapter from cached LiveData value #311

Closed
tfcporciuncula opened this issue Nov 19, 2019 · 3 comments
Labels

Comments

@tfcporciuncula
Copy link

tfcporciuncula commented Nov 19, 2019

Describe the bug
In a simple screen with a list, observing a LiveData and simply calling submitList() with the values received is currently a very common practice. Since LiveData will keep the last emission in memory, when orientation change happens the values are immediately available and we're able to populate the RecyclerView before it restores its state, so we can easily retain scroll position on orientation change.

If instead of ListAdapter.submitList() we use Groupie's updateAsync() (submitList() is async by default, so that would be the Groupie "translation"), then scroll position is lost on orientation change.

To Reproduce
Steps to reproduce the behavior:

  • Populate a list with updateAsync() with values from memory
  • Scroll a bit, rotate the screen
  • Verify scroll position is lost

Expected behavior
Scroll position is retained after orientation changes even if I'm populating the list with updateAsync().

Library version
2.7.1

Additional context
The implementation of ListAdapter.submitList() is pretty explicit when it comes to exceptional cases like when the lists are the same, or when the new list is null, or the current list is null. On the other hand, updateAsync() triggers the AsyncTask regardless, and that's what I think causes the scroll position to be lost since we won't be able to populate the RecyclerView fast enough.

I didn't dig too deep so I might be wrong. But I think we could just do the same as ListAdapter does. And I guess it'd be even better if it was possible for Groupie to actually use ListAdapter under the hood. I'm a new Groupie user so I'm sorry if that doesn't even make sense.

@tfcporciuncula tfcporciuncula added bug waiting for owners Waiting on an answer by project owners labels Nov 19, 2019
@ValCanBuild
Copy link
Collaborator

ValCanBuild commented Nov 21, 2019

Thanks for raising this @tfcporciuncula.

The reason this is happening is due to the fact that the RecyclerView.LayoutManager does not get its state restored properly.

When we call GroupAdapter.update from onCreate, it runs synchronously and completes before onRestoreInstanceState is called on the Activity. This allows for the LayoutManager to have its state restored correctly.

When we call GroupAdapter.updateAsync the adapter contents are bound after the LayoutManager state is restored, which is why we lose the scroll position.

When you recreate the activity you also recreate your adapter (be it a GroupieAdapter or ListAdapter). So ListAdapter isn't diffing an old list with a new list (since it's a brand new adapter). The reason it works for ListAdapter is due to this check in AsyncListDiffer:

    public void submitList(@Nullable final List<T> newList) {
        // other code
        // fast simple first insert
        if (mList == null) {
            mList = newList;
            mReadOnlyList = Collections.unmodifiableList(newList);
            // notify last, after list is updated
            mUpdateCallback.onInserted(0, newList.size());
            return;
        }

Which I think we could easily recreate in Groupie:

    public void updateAsync(@NonNull final List<? extends Group> newGroups, boolean detectMoves, @Nullable final OnAsyncUpdateListener onAsyncUpdateListener) {
        // fast simple first insert
        if (groups.isEmpty()) {
            update(newGroups, detectMoves);
            if (onAsyncUpdateListener != null) {
                onAsyncUpdateListener.onUpdateComplete();
            }
            return;
        }
        final List<Group> oldGroups = new ArrayList<>(groups);

        final DiffCallback diffUtilCallback = new DiffCallback(oldGroups, newGroups);
        asyncDiffUtil.calculateDiff(newGroups, diffUtilCallback, onAsyncUpdateListener, detectMoves);
    }

I think this will take care of your issue.

But the other thing they do where they check whether the two lists are the same is more challenging in Groupie since a GroupAdapter is made up of a mix of Groups and Items and you cannot compare different Groups for equality.

@ValCanBuild ValCanBuild removed the waiting for owners Waiting on an answer by project owners label Nov 21, 2019
ValCanBuild pushed a commit that referenced this issue Nov 21, 2019
- Add option to use updateAsync in example app
@ValCanBuild
Copy link
Collaborator

ValCanBuild commented Nov 21, 2019

Opened a PR for this #313. I've also updated the example code with an option to use updateAsync for the adapter in order to test this easier in the future.

ValCanBuild pushed a commit that referenced this issue Nov 25, 2019
- Add option to use updateAsync in example app
@ValCanBuild
Copy link
Collaborator

This is now deployed in 2.7.2. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants