Skip to content

Commit 220415d

Browse files
hunterstichdsn5ft
hunterstich
authored and
dsn5ft
committed
[Carousel] Fixed scroll offset calculation error when last focal keyline comes after the last child
Calculating end scroll offset was using the distance between the last child and the last focal keyline. When the last child came before the last keyline, an unexpected scroll offset was returned. This also adds a dropdown to the catalog demo to allow setting the number of items in the carousel. PiperOrigin-RevId: 508739119
1 parent 795979c commit 220415d

File tree

7 files changed

+188
-70
lines changed

7 files changed

+188
-70
lines changed

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import android.view.LayoutInflater;
2424
import android.view.View;
2525
import android.view.ViewGroup;
26+
import android.widget.AutoCompleteTextView;
2627
import androidx.annotation.NonNull;
2728
import androidx.annotation.Nullable;
2829
import com.google.android.material.carousel.CarouselLayoutManager;
@@ -58,6 +59,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle bundle) {
5859
MaterialSwitch debugSwitch = view.findViewById(R.id.debug_switch);
5960
MaterialSwitch forceCompactSwitch = view.findViewById(R.id.force_compact_arrangement_switch);
6061
MaterialSwitch drawDividers = view.findViewById(R.id.draw_dividers_switch);
62+
AutoCompleteTextView itemCountDropdown = view.findViewById(R.id.item_count_dropdown);
6163

6264
// A start-aligned multi-browse carousel
6365
RecyclerView multiBrowseStartRecyclerView =
@@ -94,6 +96,10 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle bundle) {
9496
new CarouselAdapter(
9597
(item, position) -> multiBrowseStartRecyclerView.scrollToPosition(position));
9698

99+
itemCountDropdown.setOnItemClickListener(
100+
(parent, view1, position, id) ->
101+
adapter.submitList(CarouselData.createItems().subList(0, position)));
102+
97103
multiBrowseStartRecyclerView.setAdapter(adapter);
98104
adapter.submitList(CarouselData.createItems());
99105
}

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

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,24 @@
2121
android:layout_width="match_parent"
2222
android:orientation="vertical">
2323

24+
<TextView
25+
android:id="@+id/multi_browse_title_text_view"
26+
android:layout_width="wrap_content"
27+
android:layout_height="wrap_content"
28+
android:layout_marginHorizontal="16dp"
29+
android:layout_marginVertical="4dp"
30+
android:text="@string/cat_carousel_multi_browse_title"
31+
android:textAppearance="?attr/textAppearanceTitleMedium" />
32+
33+
<TextView
34+
android:id="@+id/multi_browse_desc_text_view"
35+
android:layout_width="wrap_content"
36+
android:layout_height="wrap_content"
37+
android:layout_marginHorizontal="16dp"
38+
android:layout_marginVertical="4dp"
39+
android:text="@string/cat_carousel_multi_browse_desc"
40+
android:textAppearance="?attr/textAppearanceBodyMedium" />
41+
2442
<com.google.android.material.materialswitch.MaterialSwitch
2543
android:id="@+id/debug_switch"
2644
android:layout_width="match_parent"
@@ -51,32 +69,31 @@
5169
android:textAppearance="?attr/textAppearanceBodyLarge"
5270
android:text="@string/cat_carousel_draw_dividers_label"/>
5371

54-
<TextView
55-
android:id="@+id/multi_browse_title_text_view"
56-
android:layout_width="wrap_content"
72+
<com.google.android.material.textfield.TextInputLayout
73+
style="?attr/textInputFilledExposedDropdownMenuStyle"
74+
android:layout_width="match_parent"
5775
android:layout_height="wrap_content"
76+
android:layout_gravity="center_vertical"
5877
android:layout_marginHorizontal="16dp"
59-
android:layout_marginVertical="4dp"
60-
android:text="@string/cat_carousel_multi_browse_title"
61-
android:textAppearance="?attr/textAppearanceTitleMedium" />
78+
android:layout_marginVertical="8dp"
79+
app:helperTextEnabled="false">
6280

63-
<TextView
64-
android:id="@+id/multi_browse_desc_text_view"
65-
android:layout_width="wrap_content"
66-
android:layout_height="wrap_content"
67-
android:layout_marginHorizontal="16dp"
68-
android:layout_marginVertical="4dp"
69-
android:text="@string/cat_carousel_multi_browse_desc"
70-
android:textAppearance="?attr/textAppearanceBodyMedium" />
81+
<AutoCompleteTextView
82+
android:id="@+id/item_count_dropdown"
83+
android:layout_width="match_parent"
84+
android:layout_height="wrap_content"
85+
android:inputType="none"
86+
android:hint="@string/cat_carousel_adapter_item_count_hint_label"
87+
app:simpleItems="@array/cat_carousel_adapter_count_content"/>
88+
89+
</com.google.android.material.textfield.TextInputLayout>
7190

7291
<androidx.recyclerview.widget.RecyclerView
7392
android:id="@+id/multi_browse_start_carousel_recycler_view"
7493
android:layout_width="match_parent"
7594
android:layout_height="196dp"
7695
android:layout_marginHorizontal="16dp"
7796
android:layout_marginVertical="16dp"
78-
android:paddingStart="70dp"
79-
android:paddingEnd="24dp"
8097
android:clipChildren="false"
8198
android:clipToPadding="false" />
8299

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,22 @@
1919
<string name="cat_carousel_draw_debug_switch_label" translatable="false">Draw debug lines</string>
2020
<string name="cat_carousel_force_compact_arrangement_label" translatable="false">Force compact arrangement</string>
2121
<string name="cat_carousel_draw_dividers_label" translatable="false">Draw dividers</string>
22+
<string name="cat_carousel_adapter_item_count_hint_label" translatable="false">Item count</string>
23+
24+
<string-array name="cat_carousel_adapter_count_content" translatable="false">
25+
<item>0</item>
26+
<item>1</item>
27+
<item>2</item>
28+
<item>3</item>
29+
<item>4</item>
30+
<item>5</item>
31+
<item>6</item>
32+
<item>7</item>
33+
<item>8</item>
34+
<item>9</item>
35+
<item>10</item>
36+
</string-array>
37+
2238

2339
<string name="cat_carousel_title" translatable="false">Carousel</string>
2440
<string name="cat_carousel_description" translatable="false">Carousels are stylized versions of lists that provide a unique viewing and behavior that suit large imagery and other visually rich content.</string>

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

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ public void setCarouselStrategy(@NonNull CarouselStrategy carouselStrategy) {
130130
public void onLayoutChildren(Recycler recycler, State state) {
131131
if (state.getItemCount() <= 0) {
132132
removeAndRecycleAllViews(recycler);
133+
currentFillStartPosition = 0;
133134
return;
134135
}
135136

@@ -166,6 +167,10 @@ public void onLayoutChildren(Recycler recycler, State state) {
166167
calculateShouldHorizontallyScrollBy(
167168
0, horizontalScrollOffset, minHorizontalScroll, maxHorizontalScroll);
168169
}
170+
171+
// Ensure currentFillStartPosition is valid if the number of items in the adapter has changed.
172+
currentFillStartPosition = MathUtils.clamp(currentFillStartPosition, 0, state.getItemCount());
173+
169174
updateCurrentKeylineStateForScrollOffset();
170175

171176
detachAndScrapAttachedViews(recycler);
@@ -506,9 +511,16 @@ private static KeylineRange getSurroundingKeylineRange(
506511
* <p>This method should be called any time a change in the scroll offset occurs.
507512
*/
508513
private void updateCurrentKeylineStateForScrollOffset() {
509-
this.currentKeylineState =
510-
keylineStateList.getShiftedState(
511-
horizontalScrollOffset, minHorizontalScroll, maxHorizontalScroll);
514+
if (maxHorizontalScroll <= minHorizontalScroll) {
515+
// We don't have enough items in the list to scroll and we should use the keyline state
516+
// that aligns items to the start of the container.
517+
this.currentKeylineState =
518+
isLayoutRtl() ? keylineStateList.getRightState() : keylineStateList.getLeftState();
519+
} else {
520+
this.currentKeylineState =
521+
keylineStateList.getShiftedState(
522+
horizontalScrollOffset, minHorizontalScroll, maxHorizontalScroll);
523+
}
512524
debugItemDecoration.setKeylines(currentKeylineState.getKeylines());
513525
}
514526

@@ -569,6 +581,12 @@ private int calculateEndHorizontalScroll(State state, KeylineStateList stateList
569581
// item hit the center of the end focal keyline.
570582
float endFocalLocDistanceFromStart = endFocalKeyline.loc - getParentStart();
571583
float endFocalLocDistanceFromEnd = getParentEnd() - endFocalKeyline.loc;
584+
if (abs(endFocalLocDistanceFromStart) > abs(lastItemDistanceFromFirstItem)) {
585+
// The last item comes before the last focal keyline which means all items should be within
586+
// the focal range and there is nowhere to scroll.
587+
return 0;
588+
}
589+
572590
return (int)
573591
(lastItemDistanceFromFirstItem - endFocalLocDistanceFromStart + endFocalLocDistanceFromEnd);
574592
}

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

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,14 @@ static void scrollToPosition(
8484
layoutManager.waitForLayout(3L);
8585
}
8686

87+
static void scrollHorizontallyBy(
88+
RecyclerView recyclerView, WrappedCarouselLayoutManager layoutManager, int dx)
89+
throws Throwable {
90+
layoutManager.expectScrolls(1);
91+
recyclerView.scrollBy(dx, 0);
92+
layoutManager.waitForScroll(3L);
93+
}
94+
8795
/**
8896
* Handles setting the items of the adapter and waiting until the recycler view has made a layout
8997
* pass.
@@ -194,6 +202,7 @@ static class WrappedCarouselLayoutManager extends CarouselLayoutManager {
194202

195203
WrappedCarouselLayoutManager() {}
196204

205+
CountDownLatch scrollLatch;
197206
CountDownLatch layoutLatch;
198207

199208
/**
@@ -206,14 +215,30 @@ void expectLayouts(int count) {
206215
layoutLatch = new CountDownLatch(count);
207216
}
208217

218+
void expectScrolls(int count) {
219+
scrollLatch = new CountDownLatch(count);
220+
}
221+
209222
/**
210223
* Tells an active layout {@link CountDownLatch} to wait a number of seconds for its release
211224
* until throwing.
212225
*/
213226
void waitForLayout(long seconds) throws Throwable {
214-
layoutLatch.await(seconds, SECONDS);
227+
waitForLatch(layoutLatch, seconds, "layout");
228+
}
229+
230+
/**
231+
* Tells an active scroll {@link CountDownLatch} to wait a number of seconds for its release
232+
* until throwing.
233+
*/
234+
void waitForScroll(long seconds) throws Throwable {
235+
waitForLatch(scrollLatch, seconds, "scroll");
236+
}
237+
238+
private void waitForLatch(CountDownLatch latch, long seconds, String tag) throws Throwable {
239+
latch.await(seconds, SECONDS);
215240
MatcherAssert.assertThat(
216-
"all layouts should complete on time", layoutLatch.getCount(), CoreMatchers.is(0L));
241+
"all " + tag + "s should complete on time", latch.getCount(), CoreMatchers.is(0L));
217242
// use a runnable to ensure RV layout is finished
218243
InstrumentationRegistry.getInstrumentation()
219244
.runOnMainSync(
@@ -228,5 +253,12 @@ public void onLayoutChildren(Recycler recycler, State state) {
228253
super.onLayoutChildren(recycler, state);
229254
layoutLatch.countDown();
230255
}
256+
257+
@Override
258+
public int scrollHorizontallyBy(int dx, Recycler recycler, State state) {
259+
int scroll = super.scrollHorizontallyBy(dx, recycler, state);
260+
scrollLatch.countDown();
261+
return scroll;
262+
}
231263
}
232264
}

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

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.google.android.material.test.R;
1919

2020
import static com.google.android.material.carousel.CarouselHelper.createDataSetWithSize;
21+
import static com.google.android.material.carousel.CarouselHelper.scrollHorizontallyBy;
2122
import static com.google.android.material.carousel.CarouselHelper.scrollToPosition;
2223
import static com.google.android.material.carousel.CarouselHelper.setAdapterItems;
2324
import static com.google.android.material.carousel.CarouselHelper.setViewSize;
@@ -66,10 +67,6 @@ public void setUp() {
6667
checkAppSupportsRtl();
6768
applyRtlPseudoLocale();
6869
createAndSetFixtures(DEFAULT_RECYCLER_VIEW_WIDTH, DEFAULT_ITEM_WIDTH);
69-
}
70-
71-
@Test
72-
public void testFirstAdapterItem_isDrawnAtRightOfContainer() throws Throwable {
7370
layoutManager.setCarouselStrategy(
7471
new CarouselStrategy() {
7572
@Override
@@ -78,6 +75,10 @@ KeylineState onFirstChildMeasuredWithMargins(
7875
return getTestCenteredKeylineState();
7976
}
8077
});
78+
}
79+
80+
@Test
81+
public void testFirstAdapterItem_isDrawnAtRightOfContainer() throws Throwable {
8182
setAdapterItems(recyclerView, layoutManager, adapter, createDataSetWithSize(10));
8283

8384
MaskableFrameLayout firstChild = (MaskableFrameLayout) recyclerView.getChildAt(0);
@@ -109,6 +110,40 @@ KeylineState onFirstChildMeasuredWithMargins(
109110
assertThat(childCenterX).isEqualTo(leftState.getFirstFocalKeyline().locOffset);
110111
}
111112

113+
@Test
114+
public void testSingleItem_shouldBeInFocalRange() throws Throwable {
115+
setAdapterItems(recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(1));
116+
117+
assertThat(((Maskable) recyclerView.getChildAt(0)).getMaskXPercentage()).isEqualTo(0F);
118+
}
119+
120+
@Test
121+
public void testSingleItem_shouldNotScrollLeft() throws Throwable {
122+
setAdapterItems(recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(1));
123+
scrollHorizontallyBy(recyclerView, layoutManager, 100);
124+
125+
assertThat(recyclerView.getChildAt(0).getRight()).isEqualTo(DEFAULT_RECYCLER_VIEW_WIDTH);
126+
}
127+
128+
@Test
129+
public void testSingleItem_shouldNotScrollRight() throws Throwable {
130+
setAdapterItems(recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(1));
131+
scrollHorizontallyBy(recyclerView, layoutManager, -100);
132+
133+
assertThat(recyclerView.getChildAt(0).getRight()).isEqualTo(DEFAULT_RECYCLER_VIEW_WIDTH);
134+
}
135+
136+
@Test
137+
public void testChangeAdapterItemCount_shouldAlignFirstItemToStart() throws Throwable {
138+
setAdapterItems(
139+
recyclerView, layoutManager, adapter, CarouselHelper.createDataSetWithSize(200));
140+
scrollToPosition(recyclerView, layoutManager, 100);
141+
setAdapterItems(recyclerView, layoutManager, adapter, createDataSetWithSize(1));
142+
143+
assertThat(recyclerView.getChildCount()).isEqualTo(1);
144+
assertThat(recyclerView.getChildAt(0).getRight()).isEqualTo(DEFAULT_RECYCLER_VIEW_WIDTH);
145+
}
146+
112147
/**
113148
* Assigns explicit sizes to fixtures being used to construct the testing environment.
114149
*

0 commit comments

Comments
 (0)