RecyclerView invokes adapter onBind while target view to be bound is temporarily detached [265347515]

Fixed

Bug

Status Update

No update yet.

Description

ry...@google.com

created issue #1

Jan 12, 2023 11:02PM

Quoting Adam Powell on an internal issue:

When we investigated [a crash with Compose in a Google app] it looks like something between RecyclerView/LinearLayoutManager is leaving the view in a temporarily detached state via ViewGroup.detachViewFromParent during the bind operation, which will disrupt the functioning of many Jetpack libraries that use ViewTreeFoo service locators to discover dependencies, Compose included. We should definitely work to fix this so that we only invoke binding operations from adapters while the view is either fully attached or fully detached.

Comments

ry...@google.com <ry...@google.com> #2Jan 12, 2023 11:03PM

apologies, i meant "navigate(To...)"

ap...@google.com <ap...@google.com> #3Mar 2, 2023 03:35AM

Marked as fixed.

Yep, it appears that navigateUp() returns true, even when nothing visually changes on the screen. This seems to be caused by popBackStack(), which is always returning true even when popping the root Fragment.

hu...@google.com <hu...@google.com> #4Mar 6, 2023 08:06PM

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

commit 77f6f126b51ef6c880368ed5b4ac900c7b187165
Author: Ian Lake <ilake@google.com>
Date: Wed Feb 06 10:03:44 2019

Fix popBackStack logic when popping the root destination

popBackStack() and by extension navigateUp() should
only return true if the user was successfully navigated
to a new destination. In cases where the entire stack
was popped, the methods should return false so that
additional logic, such as finishing the activity, can
be employed.

This fixes a regression introduced in

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

Test: updated tests
BUG: 123933201
Change-Id: I756481f14b2c7d9b8f401bea05c4dbd16d28f614

M navigation/runtime/src/androidTest/java/androidx/navigation/NavControllerTest.kt
M navigation/runtime/src/main/java/androidx/navigation/NavController.java

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

https://goto.google.com/android-sha1/77f6f126b51ef6c880368ed5b4ac900c7b187165

hu...@google.com <hu...@google.com> #5Mar 6, 2023 08:08PM

This is fixed and will be available in Navigation 1.0.0-beta02.

ry...@google.com <ry...@google.com> #6Mar 7, 2023 01:09AM

It looks like this code in ViewPager2 is a workaround for the bug that's fixed in this change.

Specifically, it looks to be working around the fact that onBindViewHolder was previously called with temporarily detached views, and using this listener to wait for it to be properly attached so that it can perform operations that rely on it being properly attached.

The previous state was bad precisely because it forced hacky workarounds like this due to the lack of a proper way to listen for changes in temp-detached state (there are listeners, but they don't work, and would likely require changes in both RecyclerView and framework to make them work...it's a very flawed design). Until seeing this workaround, I wasn't aware it was even possible at all to figure it out (and even this has some limitations that probably aren't really relevant to VP2's use-case).

I think the correct solution here is to remove this workaround in ViewPager2 and release new RecyclerView and ViewPager2 versions in parallel to address this, with release notes advising users to upgrade. Jakub, what do you think?

(also, I think I vaguely recall hearing that it's possible to do some sort of Gradle module metadata thing where we can also add the information that VP2 should be at least a certain version if being used with a given RV version? I'm not sure if/how we could do that, though)

Message last modified on Mar 7, 2023 01:14AM

hu...@google.com <hu...@google.com> #7Mar 7, 2023 03:36PM

Thanks for the explanation!

~~I'm wondering if there will be versioning issues re. Android platform support vs. AndroidX?~~ (NVM, this is all AndroidX).

Message last modified on Mar 7, 2023 03:47PM

ap...@google.com <ap...@google.com> #8Apr 27, 2023 06:58PM

Project: platform/frameworks/support
Branch: androidx-main

commit e8d8836df99cf5e0d1d4bc318947d89d4103c569
Author: Ryan Mentley <ryanmentley@google.com>
Date: Wed Mar 08 18:26:56 2023

Ensure that ViewPager2 is on the latest version when used with an updated recyclerview

As discussed at

https://issuetracker.google.com/issues/265347515#comment4,
ViewPager2 had a workaround for that bug that now crashes when the bug is
not present. To fix this, we need to ensure that ViewPager2 is on a
version containing the corresponding fix in ViewPager2.

Bug: 265347515
Bug: 271618925
Test: None (TBD?)
Change-Id: I18a5140755c15a0c93e2647207152b679e463233

M recyclerview/recyclerview/build.gradle

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

ju...@google.com <ju...@google.com> #9May 24, 2023 07:37PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.recyclerview:recyclerview:1.3.1-rc01

pr...@google.com <pr...@google.com> #10Oct 18, 2023 08:42PM