Status Update
Comments
su...@google.com <su...@google.com>
ap...@google.com <ap...@google.com> #2
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.
su...@google.com <su...@google.com> #3
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
ne...@gmail.com <ne...@gmail.com> #4
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 (
Description
Version used: Android 9 Pie
Add a test facility to reduce minimal work interval from 15 mins to 1 min or less.
Rationale: We must test our app end to end to make sure scheduled periodic jobs to upload business-critical data to back-end are executed correctly. The current minimal periodic work schedule interval is 15 mins, which means rounds of regressions usually take 30-60 mins for just a couple of scenarios. Testing will significantly improve if testers could tune this minimal 15 mins interval to something shorter than 2 mins.
Examples of possible acceptable options to solve this:
- a new option under the Android developer-options menu (just like animations speed)
- a setting in WorkManager's lib to create builds with customized interval