Concat Adapter: ViewHolde.getBindingAdapterPosition is always NO_POSITION in onViewRecycled method in Nested Adapters [187339376]

Assigned

Feature

Status Update

No update yet.

Description

da...@gmail.com

created issue #1

May 6, 2021 11:22AM

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:

public void onViewRecycled(ViewHolder holder) {
        NestedAdapterWrapper wrapper = mBinderLookup.remove(holder);
        if (wrapper == null) {
            throw new IllegalStateException("Cannot find wrapper for " + holder
                    + ", seems like it is not bound by this adapter: " + this);
        }
        wrapper.adapter.onViewRecycled(holder);
    }

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:

override fun onViewRecycled(holder: RecyclerView.ViewHolder) {
    // delegatesManager will decide to which delegate to send onViewRecycled event using view type
    // but for Concat adapter holder.itemViewType will give me global view type which is useless in isolation mode
    delegatesManager.onViewRecycled(holder, getItemViewType(holder.bindingAdapterPosition))
  }

I use holder.getBindingAdapterPosition() and receive NO_POSITION, of course. Because the Concat Adapter throws my call through ConcatAdapterController again by triggering getLocalAdapterPosition:

public int getLocalAdapterPosition(
            Adapter<? extends ViewHolder> adapter,
            ViewHolder viewHolder,
            int globalPosition
    ) {
        NestedAdapterWrapper wrapper = mBinderLookup.get(viewHolder);
        if (wrapper == null) {
            return NO_POSITION;
        }
        int itemsBefore = countItemsBefore(wrapper);
        // local position is globalPosition - itemsBefore
        int localPosition = globalPosition - itemsBefore;
        // Early error detection:
        int itemCount = wrapper.adapter.getItemCount();
        if (localPosition < 0 || localPosition >= itemCount) {
            throw new IllegalStateException("Detected inconsistent adapter updates. The"
                    + " local position of the view holder maps to " + localPosition + " which"
                    + " is out of bounds for the adapter with size "
                    + itemCount + "."
                    + "Make sure to immediately call notify methods in your adapter when you "
                    + "change the backing data"
                    + "viewHolder:" + viewHolder
                    + "adapter:" + adapter);
        }
        return wrapper.adapter.findRelativeAdapterPositionIn(adapter, viewHolder, localPosition);
    }

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.

Comments

yb...@google.com <yb...@google.com> May 6, 2021 02:29PM

Reassigned to yb...@google.com.

da...@gmail.com <da...@gmail.com> #2May 18, 2021 01:43PM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6552735953321984/#/history/all)
Created: 2020-03-24 00:08:58.917870+00:00
Changes: aosp/1266484

yb...@google.com <yb...@google.com> #3May 18, 2021 04:31PM

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

commit 41c9e54ce4ab51f95d19a55fa92a56b6bd018c62
Author: Ian Lake <ilake@google.com>
Date: Mon Mar 23 16:07:32 2020

Allow obfuscation of kept Fragments

Fragments that are kept solely by class
can safely be obfuscated.

By switching to keepclasseswithmembers,
only classes with a default constructor
will qualify to be kept. This also allows
shrinking of classes that are constructed
through a custom FragmentFactory.

Test: tested in sample app at

bug 148181315

#comment4
BUG: 151605338
Change-Id: I68ea8779bce80f33f27658cf87a94ccede5376b0

M fragment/fragment/proguard-rules.pro

https://android-review.googlesource.com/1266484

yb...@google.com <yb...@google.com> #4May 18, 2021 09:16PM

While -keepclassmembers caused a regression in the original bug, we've added allowsObfuscation to allow even kept fragments to be obfuscated.

This will be available in the upcoming Fragment 1.2.4 and 1.3.0-alpha03.

yb...@google.com <yb...@google.com> #5May 19, 2021 01:52AM

The new release 1.2.4 now leads to a direct crash on app-start, i.e. when Android tries to inflate any Fragment referenced in the main navigation graph.

da...@gmail.com <da...@gmail.com> #6May 19, 2021 09:06AM

Re #5 - please file a new bug with a sample project that reproduces your issue.

Message last modified on May 19, 2021 09:07AM

MyApplication.zip

1020 KB

Download

ap...@google.com <ap...@google.com> #7May 19, 2021 04:01PM

This change in 1.2.4 also leads to ClassNotFoundException when a fragment is created by providing a name attribute to a FragmentContainerView if the referenced fragment class name is not explicitly kept and obfuscation is enabled:

<androidx.fragment.app.FragmentContainerView
                ...
                android:name="fullClassName"
                ... />

This, of course, is the expected behavior, but I think it is too much of an API-breaking change for a patch release.

yb...@google.com <yb...@google.com> #8May 19, 2021 04:09PM

Re #7 - please make sure you've followed the instructions in b/142601969 as otherwise no ProGuard rules are generated for FragmentContainerView referenced classes as per the Fragment 1.2.0 Known Issues.

If you're using Android Gradle Plugin 4.1 Canary 4 or higher and you're not getting your classes properly kept and non-obfuscated when using FragmentContainerView, please file a bug against AAPT.

da...@gmail.com <da...@gmail.com> #9May 19, 2021 04:41PM

Re #6: It's essentially https://issuetracker.google.com/issues/142601969 and indeed an API-breaking change. Unfortunately, there's no proper fix for AGP 3.6 nor 4.0, thus one can't update until those are stable. It's unfortunate as other AndroidX libraries start depending on 1.3.4 (such as preferences).

Message last modified on May 19, 2021 04:43PM

yb...@google.com <yb...@google.com> #10May 19, 2021 04:59PM

Thanks everyone

da...@gmail.com <da...@gmail.com> #11May 19, 2021 05:57PM

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> #12May 19, 2021 06:37PM

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: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:paging/runtime/src/main/java/androidx/paging/PagingDataAdapter.kt;l=296?q=LoadStateAdapter&ss=androidx%2Fplatform%2Fframeworks%2Fsupport so you can also construct it and use no isolation.