[Carousel] Fixed child index bug causing items to be ordered incorrectly.

When filling the RecyclerView, views need to be added at the correct index (either begginning or end) depending on the direction of fill.

PiperOrigin-RevId: 513510079

Author: hunterstich

catalog/java/io/material/catalog/carousel/MultiBrowseDemoFragment.java

@@ -68,7 +68,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle bundle) {
     RecyclerView multiBrowseStartRecyclerView =
         view.findViewById(R.id.multi_browse_start_carousel_recycler_view);
     CarouselLayoutManager multiBrowseStartCarouselLayoutManager = new CarouselLayoutManager();
-    multiBrowseStartCarouselLayoutManager.setDrawDebugEnabled(
+    multiBrowseStartCarouselLayoutManager.setDebuggingEnabled(
         multiBrowseStartRecyclerView, debugSwitch.isChecked());
     multiBrowseStartRecyclerView.setLayoutManager(multiBrowseStartCarouselLayoutManager);
     multiBrowseStartRecyclerView.setNestedScrollingEnabled(false);
@@ -77,7 +77,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle bundle) {
         (buttonView, isChecked) -> {
           multiBrowseStartRecyclerView.setBackgroundResource(
               isChecked ? R.drawable.dashed_outline_rectangle : 0);
-          multiBrowseStartCarouselLayoutManager.setDrawDebugEnabled(
+          multiBrowseStartCarouselLayoutManager.setDebuggingEnabled(
               multiBrowseStartRecyclerView, isChecked);
         });
 

catalog/java/io/material/catalog/carousel/res/layout/cat_carousel_multi_browse_fragment.xml

@@ -47,7 +47,7 @@
       android:layout_marginHorizontal="16dp"
       android:layout_marginVertical="8dp"
       android:textAppearance="?attr/textAppearanceBodyLarge"
-      android:text="@string/cat_carousel_draw_debug_switch_label"/>
+      android:text="@string/cat_carousel_debug_mode_label"/>
 
     <com.google.android.material.materialswitch.MaterialSwitch
       android:id="@+id/force_compact_arrangement_switch"

catalog/java/io/material/catalog/carousel/res/values/strings.xml

@@ -16,7 +16,7 @@
   -->
 
 <resources>
-  <string name="cat_carousel_draw_debug_switch_label" translatable="false">Draw debug lines</string>
+  <string name="cat_carousel_debug_mode_label" translatable="false">Debug mode</string>
   <string name="cat_carousel_force_compact_arrangement_label" translatable="false">Force compact arrangement</string>
   <string name="cat_carousel_draw_dividers_label" translatable="false">Draw dividers</string>
   <string name="cat_carousel_adapter_item_count_hint_label" translatable="false">Item count</string>

lib/java/com/google/android/material/carousel/CarouselLayoutManager.java

@@ -33,6 +33,7 @@
 import androidx.recyclerview.widget.RecyclerView.LayoutParams;
 import androidx.recyclerview.widget.RecyclerView.Recycler;
 import androidx.recyclerview.widget.RecyclerView.State;
+import android.util.Log;
 import android.view.View;
 import android.view.ViewGroup;
 import android.view.accessibility.AccessibilityEvent;
@@ -62,6 +63,8 @@
  */
 public class CarouselLayoutManager extends LayoutManager implements Carousel {
 
+  private static final String TAG = "CarouselLayoutManager";
+
   private int horizontalScrollOffset;
 
   // Min scroll is the offset number that offsets the list to the right/bottom as much as possible.
@@ -73,6 +76,7 @@ public class CarouselLayoutManager extends LayoutManager implements Carousel {
   // will move the list to the start of the container.
   private int maxHorizontalScroll;
 
+  private boolean isDebuggingEnabled = false;
   private final DebugItemDecoration debugItemDecoration = new DebugItemDecoration();
   @NonNull private CarouselStrategy carouselStrategy;
   @Nullable private KeylineStateList keylineStateList;
@@ -214,6 +218,8 @@ private void fill(Recycler recycler, State state) {
       addViewsStart(recycler, firstPosition - 1);
       addViewsEnd(recycler, state, lastPosition + 1);
     }
+
+    validateChildOrderIfDebugging();
   }
 
   @Override
@@ -224,6 +230,8 @@ public void onLayoutCompleted(State state) {
     } else {
       currentFillStartPosition = getPosition(getChildAt(0));
     }
+
+    validateChildOrderIfDebugging();
   }
 
   /**
@@ -247,7 +255,8 @@ private void addViewsStart(Recycler recycler, int startPosition) {
       if (isLocOffsetOutOfFillBoundsEnd(calculations.locOffset, calculations.range)) {
         continue;
       }
-      addAndLayoutView(calculations.child, calculations.locOffset);
+      // Add this child to the first index of the RecyclerView.
+      addAndLayoutView(calculations.child, /* index= */ 0, calculations.locOffset);
     }
   }
 
@@ -273,7 +282,58 @@ private void addViewsEnd(Recycler recycler, State state, int startPosition) {
       if (isLocOffsetOutOfFillBoundsStart(calculations.locOffset, calculations.range)) {
         continue;
       }
-      addAndLayoutView(calculations.child, calculations.locOffset);
+      // Add this child to the last index of the RecyclerView
+      addAndLayoutView(calculations.child, /* index= */ -1, calculations.locOffset);
+    }
+  }
+
+  /** Used for debugging. Logs the internal representation of children to default logger. */
+  private void logChildrenIfDebugging() {
+    if (!isDebuggingEnabled) {
+      return;
+    }
+
+    if (Log.isLoggable(TAG, Log.DEBUG)) {
+      Log.d(TAG, "internal representation of views on the screen");
+      for (int i = 0; i < getChildCount(); i++) {
+        View child = getChildAt(i);
+        float centerX = getDecoratedCenterXWithMargins(child);
+        Log.d(
+            TAG,
+            "item position " + getPosition(child) + ", center:" + centerX + ", child index:" + i);
+      }
+      Log.d(TAG, "==============");
+    }
+  }
+
+  /**
+   * Used for debugging. Validates that child views are laid out in correct order. This is important
+   * because rest of the algorithm relies on this constraint.
+   *
+   * <p>Child 0 should be closest to adapter position 0 and last child should be closest to the last
+   * adapter position.
+   */
+  private void validateChildOrderIfDebugging() {
+    if (!isDebuggingEnabled || getChildCount() < 1) {
+      return;
+    }
+
+    for (int i = 0; i < getChildCount() - 1; i++) {
+      int currPos = getPosition(getChildAt(i));
+      int nextPos = getPosition(getChildAt(i + 1));
+      if (currPos > nextPos) {
+        logChildrenIfDebugging();
+        throw new IllegalStateException(
+            "Detected invalid child order. Child at index ["
+                + i
+                + "] had adapter position ["
+                + currPos
+                + "] and child at index ["
+                + (i + 1)
+                + "] had adapter position ["
+                + nextPos
+                + "].");
+      }
     }
   }
 
@@ -294,7 +354,7 @@ private ChildCalculations makeChildCalculations(Recycler recycler, float start,
     View child = recycler.getViewForPosition(position);
     measureChildWithMargins(child, 0, 0);
 
-    float centerX = addEnd((int) start, (int) halfItemSize);
+    int centerX = addEnd((int) start, (int) halfItemSize);
     KeylineRange range =
         getSurroundingKeylineRange(currentKeylineState.getKeylines(), centerX, false);
 
@@ -309,11 +369,13 @@ private ChildCalculations makeChildCalculations(Recycler recycler, float start,
    * scrolling axis.
    *
    * @param child the child view to add and lay out
+   * @param index the index at which to add the child to the RecyclerView. Use 0 for adding to the
+   *     start of the list and -1 for adding to the end.
    * @param offsetCx where the center of the masked child should be placed along the scrolling axis
    */
-  private void addAndLayoutView(View child, float offsetCx) {
+  private void addAndLayoutView(View child, int index, float offsetCx) {
     float halfItemSize = currentKeylineState.getItemSize() / 2F;
-    addView(child);
+    addView(child, index);
     layoutDecoratedWithMargins(
         child,
         /* left= */ (int) (offsetCx - halfItemSize),
@@ -336,7 +398,7 @@ private void addAndLayoutView(View child, float offsetCx) {
    */
   private boolean isLocOffsetOutOfFillBoundsStart(float locOffset, KeylineRange range) {
     float maskedSize = getMaskedItemSizeForLocOffset(locOffset, range);
-    int maskedEnd = addEnd((int) locOffset, (int) (maskedSize / 2));
+    int maskedEnd = addEnd((int) locOffset, (int) (maskedSize / 2F));
     return isLayoutRtl() ? maskedEnd > getContainerWidth() : maskedEnd < 0;
   }
 
@@ -354,7 +416,7 @@ private boolean isLocOffsetOutOfFillBoundsStart(float locOffset, KeylineRange ra
    */
   private boolean isLocOffsetOutOfFillBoundsEnd(float locOffset, KeylineRange range) {
     float maskedSize = getMaskedItemSizeForLocOffset(locOffset, range);
-    int maskedStart = addStart((int) locOffset, (int) (maskedSize / 2));
+    int maskedStart = addStart((int) locOffset, (int) (maskedSize / 2F));
     return isLayoutRtl() ? maskedStart < 0 : maskedStart > getContainerWidth();
   }
 
@@ -560,7 +622,7 @@ private int calculateStartHorizontalScroll(KeylineStateList stateList) {
     Keyline startFocalKeyline =
         isRtl ? startState.getLastFocalKeyline() : startState.getFirstFocalKeyline();
     float firstItemDistanceFromStart = getPaddingStart() * (isRtl ? 1 : -1);
-    float firstItemStart =
+    int firstItemStart =
         addStart((int) startFocalKeyline.loc, (int) (startState.getItemSize() / 2F));
     return (int) (firstItemDistanceFromStart + getParentStart() - firstItemStart);
   }
@@ -922,7 +984,7 @@ private int scrollBy(int distance, Recycler recycler, State state) {
    */
   private void offsetChildLeftAndRight(
       View child, float startOffset, float halfItemSize, Rect boundsRect) {
-    float centerX = addEnd((int) startOffset, (int) halfItemSize);
+    int centerX = addEnd((int) startOffset, (int) halfItemSize);
     KeylineRange range =
         getSurroundingKeylineRange(currentKeylineState.getKeylines(), centerX, false);
 
@@ -975,14 +1037,20 @@ public int computeHorizontalScrollRange(@NonNull State state) {
   }
 
   /**
-   * Enables drawing that illustrates keylines and other internal concepts to help debug strategy.
+   * Enables features to help debug keylines and other internal layout manager logic.
+   *
+   * <p>This will draw lines on top of the RecyclerView that show where keylines are placed for the
+   * current {@link CarouselStrategy}. Enabling debugging will also throw an exception when an
+   * invalid child order is detected (child index and adapter position are incorrectly ordered). See
+   * {@link #validateChildOrderIfDebugging()} ()} ()} for more details.
    *
    * @param recyclerView The {@link RecyclerView} this layout manager is attached to.
-   * @param enabled Whether to draw debug lines.
+   * @param enabled Whether to draw debug lines and throw on state errors.
    * @hide
    */
   @RestrictTo(Scope.LIBRARY_GROUP)
-  public void setDrawDebugEnabled(@NonNull RecyclerView recyclerView, boolean enabled) {
+  public void setDebuggingEnabled(@NonNull RecyclerView recyclerView, boolean enabled) {
+    this.isDebuggingEnabled = enabled;
     recyclerView.removeItemDecoration(debugItemDecoration);
     if (enabled) {
       recyclerView.addItemDecoration(debugItemDecoration);

lib/javatests/com/google/android/material/carousel/CarouselHelper.java

@@ -15,6 +15,7 @@
  */
 package com.google.android.material.carousel;
 
+import static com.google.common.truth.Truth.assertWithMessage;
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 import android.graphics.Color;
@@ -39,6 +40,29 @@ class CarouselHelper {
 
   private CarouselHelper() {}
 
+
+  /** Ensure that as child index increases, adapter position also increases. */
+  static void assertChildrenHaveValidOrder(WrappedCarouselLayoutManager layoutManager) {
+    // CarouselLayoutManager keeps track of internal start position state and should always have
+    // an accurate ordering where adapter position increases as child index increases.
+    for (int i = 0; i < layoutManager.getChildCount() - 1; i++) {
+      int currentAdapterPosition = layoutManager.getPosition(layoutManager.getChildAt(i));
+      int nextAdapterPosition = layoutManager.getPosition(layoutManager.getChildAt(i + 1));
+      assertWithMessage(
+          "Child at index "
+              + i
+              + " had a greater adapter position ["
+              + currentAdapterPosition
+              + "] than child at index "
+              + (i + 1)
+              + " ["
+              + nextAdapterPosition
+              + "]")
+          .that(currentAdapterPosition)
+          .isLessThan(nextAdapterPosition);
+    }
+  }
+
   /**
    * Explicitly set a view's size.
    *

lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerRtlTest.java

@@ -144,6 +144,16 @@ public void testChangeAdapterItemCount_shouldAlignFirstItemToStart() throws Thro
     assertThat(recyclerView.getChildAt(0).getRight()).isEqualTo(DEFAULT_RECYCLER_VIEW_WIDTH);
   }
 
+  @Test
+  public void testScrollToEndThenToStart_childrenHaveValidOrder() throws Throwable {
+    // TODO(b/271293808): Refactor to use parameterized tests.
+    setAdapterItems(recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(10));
+    scrollToPosition(recyclerView, layoutManager, 9);
+    scrollToPosition(recyclerView, layoutManager, 2);
+
+    CarouselHelper.assertChildrenHaveValidOrder(layoutManager);
+  }
+
   /**
    * Assigns explicit sizes to fixtures being used to construct the testing environment.
    *

lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerTest.java

@@ -15,6 +15,7 @@
  */
 package com.google.android.material.carousel;
 
+import static com.google.android.material.carousel.CarouselHelper.assertChildrenHaveValidOrder;
 import static com.google.android.material.carousel.CarouselHelper.createDataSetWithSize;
 import static com.google.android.material.carousel.CarouselHelper.scrollHorizontallyBy;
 import static com.google.android.material.carousel.CarouselHelper.scrollToPosition;
@@ -232,6 +233,32 @@ public void testChangeAdapterItemCount_shouldAlignFirstItemToStart() throws Thro
     assertThat(recyclerView.getChildAt(0).getLeft()).isEqualTo(0);
   }
 
+  @Test
+  public void testScrollToEnd_childrenHaveValidOrder() throws Throwable {
+    setAdapterItems(recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(10));
+    scrollToPosition(recyclerView, layoutManager, 9);
+
+    assertChildrenHaveValidOrder(layoutManager);
+  }
+
+  @Test
+  public void testScrollToMiddle_childrenHaveValidOrder() throws Throwable {
+    setAdapterItems(
+        recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(200));
+    scrollToPosition(recyclerView, layoutManager, 99);
+
+    assertChildrenHaveValidOrder(layoutManager);
+  }
+
+  @Test
+  public void testScrollToEndThenToStart_childrenHaveValidOrder() throws Throwable {
+    setAdapterItems(recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(10));
+    scrollToPosition(recyclerView, layoutManager, 9);
+    scrollToPosition(recyclerView, layoutManager, 2);
+
+    assertChildrenHaveValidOrder(layoutManager);
+  }
+
   /**
    * Assigns explicit sizes to fixtures being used to construct the testing environment.
    *