Sticky headers displays wrong header or crashes after dataset change

[cbeyls]

StickyHeaderLinearLayoutManager does not handle adapter dataset changes correctly.

Specifically, it doesn't handle properly the case where a set of changes notifications includes an insert followed by one or more deletes or moves. This causes the wrong view to be used as sticky header in some cases, and in rare cases it can also cause an out of bounds error because the code tries to access data outside the range of the adapter.

The flawed logic is located in the child class HeaderPositionsAdapterDataObserver. As long as items are deleted or moved, everything works fine. But when onItemRangeInserted() is called, there is a problem: the code tries to access the adapter immediately to check if there are headers in the new inserted elements, but instead it must wait for the other adapter updates to be dispatched before accessing the adapter data to be able to compute the adapter positions correctly.

Because adapter updates are asynchronous, multiple calls to notify* can be done in a row for a single change of adapter data. It's only after that, during the next layout pass that the adapter data can be accessed.

Here is a simple example to illustrate the issue:

• An adapter initially contains 4 items
• The items list is updated with a new list of 4 items: a new item has been added at the end and the first item has been deleted
• To notify the changes, DiffUtil will call notifyItemInserted(4, 1) immediately followed by notifyItemDeleted(0, 1)
• When onItemRangeInserted() is called, HeaderPositionsAdapterDataObserver in StickyHeaderLinearLayoutManager will immediately try to access the adapter element at position 4 to check for new headers. But this position is out of range: the list only contains 4 elements. The adapter needs to wait for the following call to onItemRangeRemoved() to be able to compute the adapter positions correctly. The adapter should only be accessed asynchronously during the next layout pass.

Because of this, sticky headers are unstable and cannot be used with DiffUtil. Only notifyDataSetChanged() works reliably.

[elihart]

Thanks for the detailed bug report. Sticky headers were contributed by an external person and I am not too familiar with the code and don't maintain it myself, but you or anyone else are welcome to submit a PR for a fix.

[cbeyls]

A simple fix would be to request a full header positions scan on each adapter change which would not be the most efficient. I'm currently using a modified version of StickyHeaderLinearLayoutManager in my own projet, I may create a pull request with my changes later.

[AndrazP]

@cbeyls thanks for investigating this issue. Can you maybe share your current workaround?

[Apsaliya]

@cbeyls can you share gist of modified version of StickyHeaderLinearLayoutManager? i'm experiencing similar issues on onItemRangeInserted

[cbeyls]

My version is heavily modified and doesn't depend on Epoxy anymore. For example, I'm passing the adapter directly in the constructor. Feel free to check the changes I made and see if you can backport them in Epoxy: https://gist.github.com/cbeyls/db4351870e493a817bf0f4fd84e2b598

[samuelchou]

I took a look into @cbeyls 's code. The code changes compared to current version --- after cleaned up and simplified --- is very simple. It just did 2 things:

1. Make sticky-header-list generation/update into a late-time operation, which is called ONLY in onLayoutChildren.
2. Mark sticky-header-list changes in the HeaderPositionsAdapterDataObserver: onChanged, onItemRangeInserted, onItemRangeRemoved, and onItemRangeMoved.

And that's it. The key changes that ensure the sticky-header-list stay correct. The generation/update was just like the ones that did in current HeaderPositionsAdapterDataObserver.

However, It turns out that the script change-mark is quite limited, causing it ran more "whole-refresh" than needed. For example, if I insert/remove/change more than 2 times of the item list, the LayoutManager might thus refresh the whole list, making it difficult to keep the list at the same position after changes.

Because of the problem above, I don't think this would be a prefect solution (for a PR). I'm still investigating on how to prevent (or to improve) that problem.

[samuelchou]

In my personal opinion, I came up with a thought: if this issue happens because of the sticky-header-list generated incorrectly, can't we just make it simpler? Like, change it into a set storage? (Current version uses list structure, along with tons of codes to ensure the list is always in order. But failed. And that's why the issue happens IMO.)

This will be my current goal of investigation. I'd let you know if I succeed.

[markomeara]

@samuelchou Any success with the investigation?

[samuelchou]

Nope. Been busy since then. 😢

[WessimBetclic]

Hello ! I reckon this is the same issue as this one, is it on the todo list yet ? Thanks !

[samuelchou]

I did a little bit more research on it.

• It turns out that, when managing index list (of StickyHeaders), it is hard to maintain all of their accuracy.

For example, consider an index list

2, 3, 5 ,7, 11

when inserting a new (non-SH) item at index 4, it have to be modified into

2, 3, 6, 8, 12

and situation become more complex when including SH and more :

2, 3, 5, 7, 11
- insert SH[5]: -> 2, 3, 5, 6, 8, 12
- remove range [4-5]: -> 2, 3, 4, 6, 10
- insert SH[1], SH[5] -> 1, 3, 4, 5, 6, 7, 12

• And, changing List to Set doesn't reduce the risks.
• So...I'm now considering other solution.

So far, I think Set<SH-ID> might be a solution. It only turns into Set<SH-index> (dynamically) when using. But I haven't fully-researched this solution yet.

Any suggestion or idea is welcome. (I'm also looking forward to someone else fixing this problem 😆 )

[MoonRiser]

I just have a try,problem seems to be solved. just found that the class ListAdapter with a function named onCurrentListChanged.it is a nice time to trigger HeaderPositionsAdapterDataObserver invoke the onChanged methord,because the HeaderPositionsAdapterDataObserver did nothing when the Adapter with diff-util data has changed, we should use the other way to know whether the data of ListAdapter change has finished. the onCurrentListChanged just is the one. After a simple work, crash never happened again