From 9d0732be9e16c328ac960f5a8d64417b4d6a1c68 Mon Sep 17 00:00:00 2001 From: hunterstich Date: Thu, 2 Mar 2023 13:32:31 +0000 Subject: [PATCH] [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 --- .../carousel/MultiBrowseDemoFragment.java | 4 +- .../cat_carousel_multi_browse_fragment.xml | 2 +- .../catalog/carousel/res/values/strings.xml | 2 +- .../carousel/CarouselLayoutManager.java | 92 ++++++++++++++++--- .../material/carousel/CarouselHelper.java | 24 +++++ .../CarouselLayoutManagerRtlTest.java | 10 ++ .../carousel/CarouselLayoutManagerTest.java | 27 ++++++ 7 files changed, 145 insertions(+), 16 deletions(-) diff --git a/catalog/java/io/material/catalog/carousel/MultiBrowseDemoFragment.java b/catalog/java/io/material/catalog/carousel/MultiBrowseDemoFragment.java index d3c63663e5e..248a302527f 100644 --- a/catalog/java/io/material/catalog/carousel/MultiBrowseDemoFragment.java +++ b/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); }); diff --git a/catalog/java/io/material/catalog/carousel/res/layout/cat_carousel_multi_browse_fragment.xml b/catalog/java/io/material/catalog/carousel/res/layout/cat_carousel_multi_browse_fragment.xml index 505a0bc3f1d..02bbfdbb65a 100644 --- a/catalog/java/io/material/catalog/carousel/res/layout/cat_carousel_multi_browse_fragment.xml +++ b/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"/> - Draw debug lines + Debug mode Force compact arrangement Draw dividers Item count diff --git a/lib/java/com/google/android/material/carousel/CarouselLayoutManager.java b/lib/java/com/google/android/material/carousel/CarouselLayoutManager.java index 568dad2c433..c0f6a99a594 100644 --- a/lib/java/com/google/android/material/carousel/CarouselLayoutManager.java +++ b/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. + * + *

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. + * + *

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); diff --git a/lib/javatests/com/google/android/material/carousel/CarouselHelper.java b/lib/javatests/com/google/android/material/carousel/CarouselHelper.java index 770adc8e72b..6a002dc31c3 100644 --- a/lib/javatests/com/google/android/material/carousel/CarouselHelper.java +++ b/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. * diff --git a/lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerRtlTest.java b/lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerRtlTest.java index a6960591750..1607b611a11 100644 --- a/lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerRtlTest.java +++ b/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. * diff --git a/lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerTest.java b/lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerTest.java index 8678e93479e..82b2859b1e4 100644 --- a/lib/javatests/com/google/android/material/carousel/CarouselLayoutManagerTest.java +++ b/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. *