Injecting items into PagedListEpoxyController makes RecyclerView jump upon update

[kakai248]

We have a list of items that come from the database. Based on their date we generate header models that we insert in the list using addModels.

We scroll for a bit, up to a point where the last item of the first page is at half of the screen (this means the second page will be loaded). Now we update some item in the database to trigger a new PagedList. Upon submitting this new PagedList to epoxy, AsyncPagedListDiffer will diff the items and update the list. This new list will not start at position 0 (that's how paging library works). This means that the initial items will be removed from the RecyclerView. Without adding extra models, the RecyclerView will be able to keep it's position. You will see exactly the same items on screen. As soon as we add extra items, the RecyclerView isn't able to keep the same position on screen and we see a jump when it removes the initial items.

Now I have two untested theories for this:

1. It's because only the item list gets diffed so the RecyclerView doesn't account for the extra items height (is this right?). In that case we have to diff the model list instead of the item list.

2. It's because of:

override fun onModelBound(
    holder: EpoxyViewHolder,
    boundModel: EpoxyModel<*>,
    position: Int,
    previouslyBoundModel: EpoxyModel<*>?
  ) {
    // TODO the position may not be a good value if there are too many injected items.
    modelCache.loadAround(position)
  }

Somehow, the controller would have to take the additional injected items into account.

I'm more inclined towards theory 1. But I don't know if the premise for that option is valid.

I'll try to build a small project to replicate the issue. Maybe @yigit can shed some light on this?

Thanks

[yigit]

Epoxy diffs everything, the integration uses the diff from Paging just to avoid model building.
The other TODO would cause it not to load enough items. We wanted to implement item to position caching to avoid that inconsistency but unfortunately i didn't have any time to get back to this.

Nevertheless, a sample project would help a great deal figuring out what is really going on. Even though paging fails to load enough items, it should still be fine and recover state properly.

[kakai248]

@yigit here's a sample project:
https://drive.google.com/open?id=1kOMdZd0q-D_cUkbfd_Fq12SDHpnyskgR

And a video showing it:
https://drive.google.com/open?id=1PjFWK98kncU3YwHX8V3edbfANpyQM7Qf

I'm not able to reproduce this 100% of the time, but it's very frequent.
The app lists some items and everytime you click on one, it updates its counter. It works with and without headers, so it's not related to that.

The most consistent way of doing it that I found was like in the video: scroll until Item 17 is halfway of the screen and then click on it.

[ganfra]

@kakai248 : I'm also facing this issue, did you get any chance with it?

[kakai248]

@ganfra Not really, still an issue :/

I don't know if it's a problem with epoxy or Paging library.

If you find anything or have any clue, please let me know.

[manthesepeasrock]

@yigit any news on this issue?

[PedroAlvarado]

This is likely related to the google issue below. See the comments at the bottom of that issue.

https://issuetracker.google.com/issues/123834703

Long story short, if you are using an ItemKeyedDataSource, on invalidation(e.g., update on the list), the current position on the screen will be ignored when computing the items/pages to load. This can lead to "jumps" to other parts of the list as the paging library will attempt to load pages that are not the ones in the current screen context.

I consider this a bug in Android's paging library. Everyone using ItemKeyedDataSource will eventually run into this issue with no recourse other than overriding complex library code.

If you are to chase this down, find implementations of the DataSource.InvalidatedCallback interface and notice that most of them boil down to a initializeKey = PagedList.getLastKey() followed by the instantiation of a new PagedList with setInitialKey(initializeKey). The getLastKey() method effectively runs the code below, leading to "jumps" as the current position is ignored.

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

        return getKey(item);
    }

[erluxman]

I tried extending PagedList (PagedStorage seems to be Package private ) and ContiguousPagedList(whole file Package Private also) to override this behavior but was not able to do so due to those restrictions. It needs to be fixed in the Paging Library Itself, don't see any other option here.

Current Hack :

Whenever we need to call invalidate() on the PagedList, find a model which is visible and set it's key to DataSourceImpl.lastVisibleItemKey with following impl of DataSource.

DataSourceImpl :  ItemKeyedDataSource{
    .....
    override fun loadInitial(params: LoadInitialParams<DataSourceKey>, callback: LoadInitialCallback<E>) {
        val paginationType = if (params.requestedInitialKey != null) PaginationType.BEFORE else PaginationType.AFTER

        loadData(params.requestedLoadSize, lastVisibleItemKey
                ?: params.requestedInitialKey, callback, paginationType)

        lastVisibleItemKey = null
    }
    .....
    companion object {
        var lastVisibleItemKey: DataSourceKey? = null
    }
}

Even this is far away from being called usable, let alone the smell and Spaghetti.

[PedroAlvarado]

@yigit Can you provide some input into this issue? The participants on this issue have provided a fair amount of detail on why this issue happens along with steps to reproduce. I understand that there is a fair amount of changes coming up in Android Pagination(coroutines, etc) and it will be great that an answer to this issue is provided as part of this iteration. Thank you for your help.