IssueTracker

From what I undestand, the standard behavior of LivePagedListBuilder on invalidation is to initiate a load from the last loaded key,
which can be outside (farther down) of the range that is displayed in the RecyclerView,
leading the DiffUtil to notify displayed items as having been removed (because they are before the last loaded key so not loaded in the inital load after invalidation).

The only way I could imagine how to solve the issue is to provide feedback from the LayoutManager of the RecyclerView on what is the visible range.

So I created a copy of LivePagedListBuilder called ViewableLivePagedListBuilder that allows to set at any time the first key and minimum size to load in case of an invalidation.

https://github.com/HappyPeng2x/RoomRecyclerViewExample/blob/livepagedlistbuilder/app/src/main/java/org/happypeng/roomrecyclerviewexample/paging/ViewableLivePagedListBuilder.java

ViewableLivePagedListBuilder provides a new setLoadRange() method to set the required information, and the behavior of the create() method is modified.

By calling this setLoadRange() in the onSroll() method of a OnScrollListener added to the RecyclerView,
it is possible to make it so that in case of invalidation this ViewableLivePagedListBuilder triggers an initial load from the item corresponding to the first visible item in the RecyclerView,
for a number of pages that is big enough to cover all the items displayed on the screen.

I updated my example code in a new branch to demonstrate my ViewableLivedPagedListBuilder.

https://github.com/HappyPeng2x/RoomRecyclerViewExample/tree/livepagedlistbuilder

The new behavior is disabled by default and can be activated by a button ("Activate viewable mode") in the GUI.

The app in this branch also logs each item clicked by the user,
initializeKey and config.initialLoadSizeHint each time PagedList.Builder<>::build() is called by ViewableLivedPagedListBuilder,
and activation/desactivation of the new behavior.

This log shows the two behaviors clearly.

# First we start with the classical LivePagedListBuilder behavior
2019-02-09 17:51:16.578 6146-6146/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Viewable mode: false
2019-02-09 17:51:16.602 6146-6171/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Initialize key: null, range size = 20

# User clicks entry 30, we can see that on invalidation load starts at item 37
2019-02-09 17:51:26.896 6146-6146/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Clicked entry: 30
2019-02-09 17:51:26.908 6146-6171/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Initialize key: 37, range size = 20

# User clicks entry 30 one more time, we can see that this time load starts at 49: displayed items will be removed, this is the undesired behavior
2019-02-09 17:51:30.302 6146-6146/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Clicked entry: 30
2019-02-09 17:51:30.307 6146-6171/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Initialize key: 49, range size = 20

# New behavior is enabled
2019-02-09 17:51:54.767 6146-6146/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Viewable mode: true

# User clicks entry 30, we can see that this time on invalidation load starts at item 24 which is the first displayed on the screen
2019-02-09 17:51:58.797 6146-6146/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Clicked entry: 30
2019-02-09 17:51:58.817 6146-6175/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Initialize key: 24, range size = 20

# User clicks entry 30 one more time, we can see that on invalidation the load is exactly the same as after the previous click
2019-02-09 17:52:08.762 6146-6146/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Clicked entry: 30
2019-02-09 17:52:08.780 6146-6171/org.happypeng.roomrecyclerviewexample D/TEST_LIST_BUILDER: Initialize key: 24, range size = 20

Note than this requires to recover the first key from a call to the findFirstVisibleItemPosition() of the layout manager,
which works fine in my case because I can deduce it from the values included in my table, but cannot be applied to any random query,
so something would need to be done for this solution to be completely generic.

The problem that you're seeing is that Room's auto-generated DataSource implementation uses position as a key, which isn't stable across large insert operations above the currently focused row.

If you're always inserting at the top of the list, then you can work around the problem by reversing both your query sort order and list, since then the positions will start at the bottom. To reverse the RecyclerView order you should be able to use LinearLayoutManager.setReverseLayout(true) and possibly LinearLayoutManager.setStackFromEnd(true).

Generally I'd recommend to have a prefetch distance / page size to be around the same size as the largest insert operation you'll commonly see. That's not reasonable for your case where you're inserting 1000 items, but that often solves problems with smaller inserts.

The ideal way to have this work would be to have item keys (that is, ItemKeyedDataSource), but Room doesn't support generating code with keys embedded in items yet.

Thank you for your answer.

The use case I try to demonstrate is the modification of an item that is displayed on the screen.

I only insert rows to initially populate the table before the test, and in the case of the new version mentioned in comment #2 at the bottom with a BoundaryCallback in order to show that the infinite scrolling behavior is not broken by my custom LivePagedListBuilder.

Therefore I perform no insertion above the focused row.

I think the issue is that on invalidation the LivePagedListBuilder will trigger an initial load at the last loaded key, which in many cases is below the items that are displayed, and therefore can cause some of them to disappear by the action of the DiffUtil, because they have not been loaded.

In fact positions are stable in my particular example, and I suspect the issue would be the same with an ItemKeyedDataSource.

Thanks very much for clarifying, sorry I misunderstood the code and problem. I have found the specific problem though - the library typically triggers an initial load *around* the last accessed location, but when placeholders are disabled, this logic is bypassed so the load occurs *at* the last accessed location. Since this location is often the last partially-offscreen item bound by the RecyclerView, that means most of the items onscreen are missing from the initial load, and only paged in later.

I'll fix it in the library, but in the meantime, you can either workaround the problem by enabling placeholders, or using the following code to alter the load position in your app. The following appears to avoid the problem in your app (after a lot of scrolling and toggling).

1) Add the following class to MainActivity.java:

    static class RoomFactoryWrapper<T> extends DataSource.Factory<Integer, T> {
        final DataSource.Factory<Integer, T> m_wrappedFactory;

        RoomFactoryWrapper(@NonNull Factory<Integer, T> wrappedFactory) {
            m_wrappedFactory = wrappedFactory;
        }

        @NonNull
        @Override
        public DataSource<Integer, T> create() {
            return new DataSourceWrapper<>((PositionalDataSource<T>) m_wrappedFactory.create());
        }

        static class DataSourceWrapper<T> extends PositionalDataSource<T> {
            final PositionalDataSource<T> m_wrappedSource;

            DataSourceWrapper(PositionalDataSource<T> wrappedSource) {
                m_wrappedSource = wrappedSource;
            }

            @Override
            public void addInvalidatedCallback(@NonNull InvalidatedCallback onInvalidatedCallback) {
                m_wrappedSource.addInvalidatedCallback(onInvalidatedCallback);
            }

            @Override
            public void removeInvalidatedCallback(
                @NonNull InvalidatedCallback onInvalidatedCallback) {
                m_wrappedSource.removeInvalidatedCallback(onInvalidatedCallback);
            }

            @Override
            public void invalidate() {
                m_wrappedSource.invalidate();
            }

            @Override
            public boolean isInvalid() {
                return m_wrappedSource.isInvalid();
            }

            @Override
            public void loadInitial(@NonNull LoadInitialParams params,
                @NonNull LoadInitialCallback<T> callback) {
                // Workaround for paging bug: https://issuetracker.google.com/issues/123834703
                // edit initial load position to start 1/2 load ahead of requested position
                int newStartPos = params.placeholdersEnabled
                    ? params.requestedStartPosition
                    : Math.max(0, params.requestedStartPosition - (params.requestedLoadSize / 2));
                m_wrappedSource.loadInitial(new LoadInitialParams(
                    newStartPos,
                    params.requestedLoadSize,
                    params.pageSize,
                    params.placeholdersEnabled
                ), callback);
            }

            @Override
            public void loadRange(@NonNull LoadRangeParams params,
                @NonNull LoadRangeCallback<T> callback) {
                m_wrappedSource.loadRange(params, callback);
            }
        }
    }

2) Use the wrapper on the dataSourceFactory:

        new LivePagedListBuilder<>
                (new RoomFactoryWrapper<>(mDB.getMyDao().getAllPaged()), plConfig)

That effectively applies the bugfix I'll use inside the library by offsetting the initial load parameters.

Thanks for the very helpful repro case!

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 439b641366eaac4a4d4c7a069781be03809d7a8b
Author: Chris Craik <ccraik@google.com>
Date:   Wed Feb 13 15:37:23 2019

    Center positional loads around last access when placeholders disabled
   
    Test: ./gradlew paging:paging-common:test
    Fixes: 123834703
   
    For consistency, contiguous positional initial loads are now snapped to
    the nearest page boundary, and forced to be >= 2 pages in size.
   
    Change-Id: I7d729a1bcf5827b42677adc0cdeb01f257740680

M       paging/common/src/main/java/androidx/paging/PositionalDataSource.java
M       paging/common/src/test/java/androidx/paging/PositionalDataSourceTest.kt
M       paging/common/src/test/java/androidx/paging/TiledPagedListTest.kt

https://android-review.googlesource.com/904380
https://goto.google.com/android-sha1/439b641366eaac4a4d4c7a069781be03809d7a8b

It seems this issue is fixed for PositionalDataSource only. As mentioned in the previous comments, an ItemKeyedDataSource will trigger an initial load off the last loaded key effectively rendering the same issue.

ItemKeyedDataSource is expected to load data items around the passed key, not starting at the key. Documented here: https://developer.android.com/reference/androidx/paging/ItemKeyedDataSource.LoadInitialParams.html#requestedInitialKey

So, for example, a room/database implementation might load the first half of requested items before the passed key, and the second half after the last item loaded in the first load. We do this with ItemKeyedDataSource so it's less likely that an insert/delete near the user's scroll position will mean we don't get the currently visible items in the initial load.

Acknowledged. Nonetheless, the issue at hand is that, by default, the requestedInitialKey happens to always be the last loaded key(getLastKey()) and if a user happens to scroll up and tap to update an item past the set items "around" the key then, as a result of an invalidate(), the user will be scrolled to the item at the edge of the "load around " set. This will look as if a user jumped from one part of the list to another as demonstrated in the video provided by the initial reporter.

This is not a scenario where items are removed. Instead, an item in the view port is simply updated.

If this is the expected and final behavior, is there a recommended practive to deal with this scenario or is there a way to provide a hint to android pagination such that it can load around from a position other than getLastKey() for contiguous pagedlists?

Short of sample code to share, please find below the logical steps to reproduce:

0) Assume you can fit 8 to 10 items on the screen/viewport.
1) Load items with pageSize of 20. (default prefetch, intialload, etc)
2) Load through 100. (Effectively loading through 140 = 20(prefetch) + 20(next page))
3) Scroll all the way back to page one. That is, items 1-20.
4) Update item 1. (say, press a button in the item and change its color)
5) Invalidate the data source to reflect the update on the item.
6) Notice the jump all the way to item 110. (requestedInitialKey = 140, InitialLoadSize(default) = PageSize * 3 = 60, Initial Load Around = InitialLoadSize/2 = 30+-)
7) Notice that the requestedInitialKey is the key for item 140. Far away from the viewport.
8) Notice the poor experience having updated an item one place and being pushed to a seemingly random place on the list from the user perspective.
10) Notice this behavior is reproducible regardless of placeholder configuration.

#9 - getLastKey() gets the last *accessed* key, not the last loaded key (perhaps the name there is unclear)

If you scroll back up to item 1 in step 4) above, RecyclerView will be rebinding each item that was outside the viewport as you go.

In PagedListAdapter, when getItem(0) is called at the top, that internally calls PagedList.get(0), which (for ItemKeyedDataSource) internally updates ContiguousPagedList.mLastItem. That's where the key is queried from when getLastKey is called, so it'll return DataSource.getKey(itemAtPosZero).

Are you able to reproduce the behavior that gets you requestedInitialKey = 140 with an ItemKeyedDataSource?

Everything you are describing is correct. However, the issues described here stem from invalidating a data source(step 5 in my description above). When an invalidation takes place for ContiguousDataSources, position/mlastload is ignored. Per the example above and at step "5)",  after the invalidation is triggered, a new PagedList will be created via RxPagedListBuilder(in our case) with mlastload=0 and mLastItem=140th. As part of this process, a initializeKey will be computed using pagedlist.getLastKey() which will internally use ContiguousPagedList.getKey(mlastload=0 , mLastItem=140th). The code for this method in the case of the ItemKeyedDataSource is the one below.

    final Key getKey(int position, Value item) {
        if (item == null) {
            return null;
        }

        return getKey(item);
    }

This means that the paged list computed after invalidation will be based on the 140th item in the list which is the last item loaded. Obviously, this item is far away from the viewport, hence, causing a jump to a seemingly random place on the list which just happens to be some implementation dependent offset from the 140th item.

This is not trivial to explain and, unfortunately, I'm unable to provide a sample project to demonstrate this issue at the moment. Please let me know if there any additional details I can provide than can help you diagnose and/or understand the issue at hand.

I forgot to mention that I'm not using a PagedListAdapter and, instead, I'm using a custom adapter(airbnb/epoxy). It is my observation that this issue will also happen with the PagedListAdapter.

When your adapter binds an item that's already been queried out of the PagedList previously (e.g. item 0), are you calling PagedList.loadAround?

PagedListAdapter, each time an item is bound calls PagedList.loadAround and PagedList.get():

    @Nullable
    public T getItem(int index) {
        /* ... */

        mPagedList.loadAround(index);
        return mPagedList.get(index);
    }

This ensures that the 140th item is no longer the last item accessed, because when you scroll up, you rebind item 0.

https://github.com/googlesamples/android-architecture-components/pull/591/files

In normal situation, the pull on refresh would only happen when the page was on top so not flickering happened. Now we can refresh the even when the list is not at top so, when we are scrolling down 3 4 pages of items (with respect to config) and press refresh button, the flicker in the list happens.

Ideally the list was supposed to be same. I hope this sample/change will make you understand the problem we are facing.

```
Marked as fixed.
Project: platform/frameworks/support
Branch: androidx-master-dev

commit 439b641366eaac4a4d4c7a069781be03809d7a8b
Author: Chris Craik <ccraik@google.com>
Date:   Wed Feb 13 15:37:23 2019

    Center positional loads around last access when placeholders disabled
   
    Test: ./gradlew paging:paging-common:test
    Fixes: 123834703
   
    For consistency, contiguous positional initial loads are now snapped to
    the nearest page boundary, and forced to be >= 2 pages in size.
   
    Change-Id: I7d729a1bcf5827b42677adc0cdeb01f257740680

M       paging/common/src/main/java/androidx/paging/PositionalDataSource.java
M       paging/common/src/test/java/androidx/paging/PositionalDataSourceTest.kt
M       paging/common/src/test/java/androidx/paging/TiledPagedListTest.kt

https://android-review.googlesource.com/904380
https://goto.google.com/android-sha1/439b641366eaac4a4d4c7a069781be03809d7a8b
```

In which version of does this fix being released?

I'm seeing this issue in a PageKeyedDataSource. Scrolling n pages down and then updating an item in the list appears to cause the page to reload. This causes the RecyclerView to scroll to the previous page index. What is the solution here?

I have exactly the same issues as described above, Is the fix being released yet?

I don’t think much else can be done to explain this issue to google. This library is broken under the scenarios described. If you are here, your best path is to not to use this library. You can’t even patch this yourself without effectively writing your own library. Lastly, if you read the source code around this issue, this likely a major/architecture issue with no quick fix (e.g. interfaces may need to change).

Be advised.

First, thanks for the couple recent pings on the bug - it made us realize that the patch in #6 never actually went out, so we've shipped Paging 2.1.1 today with that fix. That fix only applies to PositionalDataSource. Very sorry about missing that patch.


Second, I wanted to clarify further that PositionalDataSource and ItemKeyedDataSource are the only sources in Paging that support in-place refresh. In order to get in-place refresh to work, you need to be calling through to pagedList.loadAround - the adapter and differ do this for you when you call `getItem(int item)`. If you aren't using those however, and accessing the PagedList items directly, you may need to call loadAround yourself (see comment #13).

PageKeyedDataSource does not support in-place refresh, generally due to network next/previous tokens not being reusable across refreshes.

The reason the pull request in #14 triggers a scroll is because either a) the refresh is triggering either model code in the sample (not the library) that is designed to only work for swipe refresh (db + network case), or b) the constraint mentioned here - PageKeyedDataSource doesn't support in-place refresh (network-only case).

For the db + network case, it is possible to support in-place refresh today, yourself, though it would require a lot of bookkeeping to determine approximately where the viewport is, in terms of network key.


Third, we are in the process of replacing the DataSource APIs for the next major version of Paging, which allows us to remove the above limitation on page-keyed refreshes, if your network API supports it. In the replacement, the Source itself is responsible for determining the refresh key, given its current position, so you can define the refresh behavior as you like (either restarting with null initial key, or carrying forward information from what's currently loaded). You can see that (work in progress) API here in the replacement for DataSource: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-master-dev:paging/common/src/main/kotlin/androidx/paging/PagedSource.kt;l=188;drc=32fa353c5bfbc8bed036d4375d5a019442c11c66

I'm still getting a similar problem. I want to recap what I have after grepping this conversation:

1. I am using ItemKeyedDataSource so I would expect refresh-in-place to work
2. I have latest 2.1.1 library with the fix that got released in #19
3. On invalidate, I'm seeing the list jump by only a single page (not all the way to the bottom of the list like #9)
4. I believe this is happening because of how PagedList.mLastItem is updated:


   @Override
    @Nullable
    public T get(int index) {
        T item = mStorage.get(index);
        if (item != null) {
            mLastItem = item;
        }
        return item;
    }

As a viewport is filled with items (either on initial population, or when scrolling down and back up) items are requested from top to bottom. So mLastItem will always be the bottom item on the screen. So when we call invalidate, internally it will call loadInitial with the key for the bottom item. So then the list will jump nearly a full page, putting the previously-on-the-bottom item at the top of the list.

You can actually see this same problem when the list has only a few items. As new items are loaded (i.e. we call invalidate on each new item) the only item that is "preserved" is item 1. That is the only item returned from loadInitial.

(The slow loading animation is just a function of the screen capture)

Ok our problem was that we were not properly using requestedInitialKey. We were only using it as the starting point. I misread some of these other answers. I thought when a "load around" was triggered that the library would call loadBefore and loadAfter for me. I didn't realize that it was up to me to actually do a load around inside loadInitial.

ItemKeyedDataSource is expected to load data items around the passed key, not starting at the key. Documented here: https://developer.android.com/reference/androidx/paging/ItemKeyedDataSource.LoadInitialParams.html#requestedInitialKey

Once we do this (basically mimicking workaround in #5) Everything worked fine:

private static final long NO_KEY = -1L;

@Override
public void loadInitial(@NonNull final LoadInitialParams<Long> params,
                        @NonNull final LoadInitialCallback<Item> callback) {
    long initialKey = params.requestedInitialKey == null ? NO_KEY : params.requestedInitialKey;
    int requestedLoadSize = params.requestedLoadSize;

    if (initialKey == NO_KEY) {
        // since this is initial load, we want to get any values inclusive of the the key
        getItemsStartingAtKeyInclusive(initialKey, requestedLoadSize)
                .subscribe(
                        callback::onResult,
                        Exceptions::propagate);
    } else {
        //  "load around" by loading items before (inclusive) AND after (exclusive) and zipping the results into
        //  a single list.
                Observable.zip(
                        getItemsStartingAtKeyInclusive(initialKey, requestedLoadSize),
                        getItemsEndingAtKeyExclusive(initialKey, requestedLoadSize),
                        (firstList, secondList) -> {
                            firstList.addAll(secondList);
                            // TODO sort
                            ...
                            return firstList;
                        })
                        .subscribe(
                                callback::onResult,
                                Exceptions::propagate);
    }
}

  The following changes were cherrypicked through Coastguard (https://android-build.googleplex.com/coastguard/)
  Release Track: (https://android-build.googleplex.com/coastguard/dashboard/6610315791630336/#/history/all)
  Created: 2020-03-04 18:48:41.256426+00:00
  Changes: aosp/1249160
 

  The following changes were cherrypicked through Coastguard (https://android-build.googleplex.com/coastguard/)
  Release Track: (https://android-build.googleplex.com/coastguard/dashboard/6610315791630336/#/history/all)
  Created: 2020-03-13 20:33:02.847411+00:00
  Changes: aosp/1249160
 

This bug is shameful. it really is. 5-10 useless classes, factories, complicated over engineered patterns that no one could remember. at in the end, what do you get? a list that scrolls on its own and jumps around.

Fortunately it is fixed now, so if you update to the latest version you will be OK.

Hello guys, I want to inform you that the bug is still present. I use paging library version 2.1.2, my RecyclerView has height depends on other views, parent layout is ConstraintLayout, here is an example

<RecyclerView
            android:id="@+id/recycler_view"
            android:layout_width="match_parent"
            android:layout_height="0dp"
            app:layout_constraintBottom_toTopOf="@id/bottom_view"
            app:layout_constraintTop_toBottomOf="@id/top_view" />

In that case I caught this bug. I couldn't fix it for a long time, but after that I changed ConstraintLayout to LinearLayout, and now bug is gone. I just decided to let you know to keep you informed

PS: If top and bottom views has visibility gone, everithing is fine

Paging 3, bug is still there.

Still there in 3.0.0beta01

Still there in 3.0.0-beta03

Guys... this bug is marked as fixed. I'm not sure Google even looks at it because of that. You'd better create a new issue.

The same issue happens in latest version of paging library with recycleview.

Bug still exists in Paging 3.1.1 with Room 2.5.0