Status Update
Comments
yb...@google.com <yb...@google.com>
da...@gmail.com <da...@gmail.com> #2
Maybe related issue with onViewDetachedFromWindow method, early without Concat Adapter I could use viewHolder.position (which is now global position in CA) to get the view holder type. But now we need to use viewHolder.bindingAdapterPosition, the only thing it is NO_POSITION again. I debugged sources and it seems another issue not with mBinderLookup, but before that RecyclerView#getAdapterPositionInRecyclerView gives NO_POSITION every time. But the problem remains the same I can't get the viewHolder position in those 3 methods due to Concat Adapter mechanics. I hope you will check this as well.
yb...@google.com <yb...@google.com> #3
yea i think they are related. Right now CA updates its own state before calling callbacks. I'll try changing that order though it might have other consequences of inconsistent state. Will investigate further and hopefully solve both here.
yb...@google.com <yb...@google.com> #4
#2, I'm not able to reproduce your issue :/.
More specifically, i created a test case where I scroll and assert that adapter position is accessible when view is detached and it is working fine.
Now there is still an issue for detach for recycle, maybe that is the case you are hitting? (which is the main bug that still needs fixing anyways)
da...@gmail.com <da...@gmail.com> #6
ok, I investigated the #2 issue again, seems like it is not really connected to concat adapter itself, but it could be a problem in my case with different view types. You are right when you scroll items it works fine. But the problem may occur even with simple RV adapter. The case here:
- You have some data in recycler view
- You clear the list and call notifyDataSetChanged()
- In onViewDetachedFromWindow you can use holder.position and find an actual position (if you do not use CA)
- In onViewDetachedFromWindow you can use holder.bindingAdapterPosition and find NO_POSITION Seems like it happens because of this part
int globalPosition = mOwnerRecyclerView.getAdapterPositionInRecyclerView(this);
if (globalPosition == NO_POSITION) {
return NO_POSITION;
}
where:
int getAdapterPositionInRecyclerView(ViewHolder viewHolder) {
if (viewHolder.hasAnyOfTheFlags(ViewHolder.FLAG_INVALID
| ViewHolder.FLAG_REMOVED | ViewHolder.FLAG_ADAPTER_POSITION_UNKNOWN)
|| !viewHolder.isBound()) {
return RecyclerView.NO_POSITION;
}
return mAdapterHelper.applyPendingUpdatesToPosition(viewHolder.mPosition);
}
seems ViewHolder like having some issues with flags. Idk, maybe it suppose to work like this, but for me it is a problem. Because in the case when I'm using CA and can't access correct position by using holder.position.
I attached a test project here, which I prepared very quickly, first I tried to use it with paging 3 and concat adapter (because it is actually what I'm trying to achieve on my real project with Adapter Delegates library, trying to run it with Paging 3 and CA 'PagingDelegationAdapter'). But then I found that it works same way with simple adapters, so you may check 'SampleAdapter'. If you run app and press 'Refresh' button it will clear the list in recycler view, then you will have an exception.
ap...@google.com <ap...@google.com> #7
Branch: androidx-main
commit 0ac1672c1c5568431e87317a8c77430e463c377e
Author: Yigit Boyar <yboyar@google.com>
Date: Tue May 18 14:18:51 2021
Ensure adapter position APIs work in ConcatAdapter for recycled VHs
This CL fixes a bug in ConcatAdapter where it was changing its internal
structure before calling onViewRecycled / onFailedToRecycle APIs. This
breaks documented API in onViewRecycled / onFailedToRecycle APIs where
it should be able to retrieve adapter position if the item is not
removed from the adapter.
I've moved callbacks before changing internal structure to fix the
issue.
Bug: 187339376
Test: ConcatAdapterTest
Change-Id: If5c9c182abed6d85df3b35e86ef79beed497c901
M recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/ConcatAdapterTest.kt
M recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/ConcatAdapterController.java
yb...@google.com <yb...@google.com> #8
When you call notifyDataSetChanged, all items become invalid so you cannot get the position out of it (you can get layout position but NOT adapter position since adapter has changed).
The difference between layout position and adapter position is that layout position is the position when it was last laid out vs adapter position is guaranteed to match adapter contents. For notifyDataSetChanged, because all of the adapter contents have changed, we have no way of calculating the adapter position anymore.
Does that make sense?
da...@gmail.com <da...@gmail.com> #9
Yes, it make sense. But basically that means, I don't have a chance to know the local view type in onViewDetachedFromWindow method, because the position itself is already invalid at the moment, and holder.itemViewType contains global view type in case of Concat adapter. Can ViewHolder store its local view type the same way it handles global view type? So we don't need to calculate either position or view type every time we need correct view type?
yb...@google.com <yb...@google.com> #10
hmm, i see. We would need a new API for that but it might feel very weird (ViewHolder.getBindingAdapterViewType()) :/
Does NO_ISOLATION not work for your use case?
da...@gmail.com <da...@gmail.com> #11
I could use NO_ISOLATION or find another way to handle view types, maybe. I just wanted to do as less changes to the paging and recycler view classes and APIs as possible. Thank you for original fix, though. It would help a bit.
Also about ViewHolder.getBindingAdapterViewType(), yeah it sounds a bit off, but maybe Concat adapter should have the method like getBindingAdapterViewType(globalViewType). I haven't dig much into Concat adapter API, but I saw that you have some kind of map between local and global types and I suppose ViewTypeLookup could convert types for both strategies (Isolation and Shared) (ViewTypeStorage in API). Also ViewTypeStorage have the method like
NestedAdapterWrapper getWrapperForGlobalType(int globalViewType)
So seems like you can have adapter wrapper without using a position, like in getItemViewType(int position) in the original RV API. Maybe you can add open method to get local view type by using global.
The ConcatAdapterController already uses similar feature when it creates view holder (or triggers VH creation)
public ViewHolder onCreateViewHolder(ViewGroup parent, int globalViewType) {
NestedAdapterWrapper wrapper = mViewTypeStorage.getWrapperForGlobalType(globalViewType);
return wrapper.onCreateViewHolder(parent, globalViewType);
}
I may read the code wrong, but at a glimpse there is could be possibility to get local view type without using position, just global view type which I can get from VH.
yb...@google.com <yb...@google.com> #12
yea that could be as well. I've been trying to set up concat adapter to be as seemless as possible to the nested adapter but it does hit these cases where an explicit API becomes necessary :/.
If we were to provide this API in concat adapter, then the container adapter still needs to know that it is part of a concat adapter (and a reference to it) so I'm not sure how useful it will be (compared to also keeping a map or field in the VH yourself). Which is why i like the NO_ISOLATION solution better (and if you use layout ids as item types, that is usually good enough unless you have different VHs with the same layout)
But i guess you cannot use no isolation w/ the paging adapter :/ (though paging adapter is actually a very simple wrapper:
Description
Component used: ConcatAdapter
Version used: 1.2.0
Devices/Android versions reproduced on: any
The case is about two methods onViewRecycled and onFailedToRecycleView in ConcatAdapter. If you see the documentation of the base recycler view adapter it said that I can get a view holder position before it will be recycled by using ViewHolder#getBindingAdapterPosition() inside those two methods. But now comes ConcatAdapter who intercept those calls and execute its own code before it re-route them to nested adapters. The concat adapter executes the line which is removes viewHolder from mBinderLookup.
Concat Adapter triggers ConcatAdapterController.onViewRecycled:
The problem then comes out in nested adapter when I'm trying to get a view type of the view holder which is about to recycle:
I use holder.getBindingAdapterPosition() and receive NO_POSITION, of course. Because the Concat Adapter throws my call through ConcatAdapterController again by triggering getLocalAdapterPosition:
This method using mBinderLookup to search for the NestedAdapterWrapper for this view holder (mBinderLookup.get(viewHolder)), but the thing is that it is already gone, it was removed a moment ago in ConcatAdapterController.onViewRecycled. So now I do not see a possibility to get local view type using just a view holder in my nested adapter onViewRecycled method. And I can't use global view type from the view holder itself, because I'm using isolateViewTypes = true.
Even though the current implementation does not match the documentation of recycler view, I don't see how to use multiple view types for the nested adapter in isolation mode.