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