Status Update
No update yet.
Comments
il...@google.com
ap...@google.com
This is actually happening in a lot of cases beyond just stable IDs; that just happens to be a particularly easy case to repro it on. Running the RecyclerView test suite on the commit in aosp/2345032 (which validates the intended invariants around temp-detached ViewHolders) will reveal the full extent (thousands of failing tests).
There are two possible ways to fix this:
-
When binding a temporarily detached ViewHolder, temporarily re-attach it, perform the bind operation, then immediately temporarily detach it again. This is implemented as aosp/2366713 and passes all tests. I favor this solution, as it's very, very simple. There's not much in the way of potential to go wrong, confuse a third-party LayoutManager, have incorrect bookkeeping, etc. The only reason not to do this is that it performs more temp detaches/reattaches than is strictly necessary, though this is a sufficiently lightweight operation that we currently temp-detach every view on every layout. If this were a bottleneck, I'd think that'd be a place to look at as well.
-
When binding a temporarily detached ViewHolder, temporarily re-attach it, marking it as being temporarily reattached for binding. If the LayoutManager attaches the View manually, simply clear the flag. At the end of layout, go through the scrap views and properly handle these. I do not favor this approach, as it has many possible edge cases (for example, the LayoutManager might add the View somewhere other than the end, which isn't yet handled in the implementation). I've implemented the basics of this as aosp/2371868 as a proof of concept (though one test is currently failing—I am a bit surprised it's not more). I don't want to spend much more time on it, though, as I favor approach #1 absent a reason not to do that.
After discussion, we've decided on approach 1.
co...@protonmail.com
Branch: androidx-main
commit ed0e0d25ef93ae87ee6e5910364d902d755b9330
Author: Ryan Mentley <ryanmentley@google.com>
Date: Tue Dec 13 00:20:00 2022
Temporarily re-attach temp-detached views for binding, add validation of various intended invariants
Test: Existing test suite should not fail with added invalidation (it does fail very hard without the fix)
Fixes: 258144648
Fixes: 265347515
Change-Id: I7244f2c749238c7241f36e57fdec155b1bea77cf
M recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/RecyclerView.java
jb...@google.com
For AndroidX, the Design assumption violated. in
As of 2023-03-06,
@Override
public final void onBindViewHolder(final @NonNull FragmentViewHolder holder, int position) {
final long itemId = holder.getItemId();
final int viewHolderId = holder.getContainer().getId();
final Long boundItemId = itemForViewHolder(viewHolderId); // item currently bound to the VH
if (boundItemId != null && boundItemId != itemId) {
removeFragment(boundItemId);
mItemIdToViewHolder.remove(boundItemId);
}
mItemIdToViewHolder.put(itemId, viewHolderId); // this might overwrite an existing entry
ensureFragment(position);
/** Special case when {@link RecyclerView} decides to keep the {@link container}
* attached to the window, but not to the view hierarchy (i.e. parent is null) */
final FrameLayout container = holder.getContainer();
if (ViewCompat.isAttachedToWindow(container)) {
if (container.getParent() != null) {
throw new IllegalStateException("Design assumption violated."); // <================ FAILS HERE
}
container.addOnLayoutChangeListener(new View.OnLayoutChangeListener() {
@Override
public void onLayoutChange(View v, int left, int top, int right, int bottom,
int oldLeft, int oldTop, int oldRight, int oldBottom) {
if (container.getParent() != null) {
container.removeOnLayoutChangeListener(this);
placeFragmentInViewHolder(holder);
}
}
});
}
gcFragments();
}
The error has shown up
I don't know whether the original AndroidX was making bad assumptions on Android, or that
Note that this is impacting Chrome autoroll for AndroidX (
co...@protonmail.com
Hmm I cannot CC jgielzak@ (but I can still add comment?!). Pinging him directly instead.
co...@protonmail.com
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)
jb...@google.com
Thanks for the explanation!
I'm wondering if there will be versioning issues re. Android platform support vs. AndroidX? (NVM, this is all AndroidX).
jb...@google.com
co...@protonmail.com
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
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
ma...@deutschebahn.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.recyclerview:recyclerview:1.3.1-rc01
cj...@gmail.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.recyclerview:recyclerview:1.4.0-alpha01
jb...@google.com
Please file an issue with a minimal sample project against the
ma...@deutschebahn.com
might you have unintentionally updated your navigation dependencies
yes, typically I update the nav_version field as proposed documented
I updated AGP from 7.0(.3) to 7.1.0 which forced me to then update navigation stack including safeargs (to 2.5.0alpha01) - you know why, and anyhow I updated updated kotlin from 1.5.30 to 1.6.10 - because this changed too in new AGP databinding deps - before or after, I ran into this compiler mess someone (and I doubt it is the new safeargs) somehow looks for the OnConfigurationChangeProvider which is not released with 2.4.0
It would be more helpful to me if s.o. can validate (or INvalidate) if safeArgs has it's finger in fire or somehow it's transitive dependencies.. yes, perhaps safeargs is not the guilty one in this case just the one I can blame on to be in the front row today. Sorry for that
Feedback you might take with to your next teams of teams: main purpose of this rant is to document the pain your uncoordinated releases in the androidx universe causes in the real world aside some unrealistic, "minimal sample projects" noone has the time to provide. Why don't you provide - "Eat your own dogfood" - US some sample projects that integrate all androidx components without dep problems on an active stable AGP and IntelliJ/Studio on a regular schedule when some androidx team releases a new version. Sounds unrealistic? - so it is to us! The more components your teams release with each transitive-cross dependencies, it's unlikely up to impossible to do regular project maintenance without spending nightmare hours in dependency management!
do...@gmail.com
^
class file for androidx.core.content.OnConfigurationChangedProvider not found
got the same error as you, i tried upgrading agp to 7.1 but it doesn't seem to work
jb...@google.com
Navigation 2.5.0 depends on the newest Activity, which depends on the newest core version were those APIs exist.
If you don't wish to upgrade your core version, you can use the Navigation 2.4.1 release which fixes this bug directly without any other included changes.
Description
Component used: Navigation/SafeArgs Kotlin Version used:
2.4.0-rc01AGP:7.1.0-rc01Kotlin:1.6.10When calling
gw :app:assembleDebugI see the following error (plus stacktrace):Here's my plugins block in
app/b.g.kts:Obviously the error message is incorrect -- KGP is invoked right there at the top 🤔
Happy to provide more info (Gradle Scan, other repo info) privately; just ping me!